Christian Holler (:decoder) Reporter
	Description • 10 years ago

The following testcase crashes on mozilla-central revision 5ecd532a167e (run with --fuzzing-safe):


var lfcode = new Array();
lfcode.push = loadFile;
lfcode.push("");
lfcode.push("");
lfcode.push("3");
lfcode.push("");
lfcode.push("");
lfcode.push("");
lfcode.push("");
lfcode.push("4");
lfcode.push("");
lfcode.push("0");
lfcode.push("gczeal(2);");
lfcode.push("\
var g = newGlobal();\
g.parent = this;\
g.eval('new Debugger(parent).onExceptionUnwind = f;');\
");
function loadFile(lfVarx) {
    evaluate(lfVarx);
}

	Christian Holler (:decoder) Reporter
	Comment 1 • 10 years ago

Marking as a fuzzblocker, as this crashes on the heap with no reasonable signatures that could be used to distinguish it from other failures. Also it seems highly frequent.

	Till Schneidereit [:till]
	Comment 2 • 10 years ago

NI'ing shu based on bug 716647 comment 116.

	Christian Holler (:decoder) Reporter
	Comment 3 • 10 years ago

Here's another test (requested by shu), that also crashes on the heap:


var lfcode = new Array();
lfcode.push("");
lfcode.push("0");
lfcode.push("newGlobal(\"1\").eval(\"(new Debugger).addAllGlobalsAsDebuggees();\");\n");
while (true) {
    var file = lfcode.shift(); if (file == undefined) { break; }
    loadFile(file)
}
function loadFile(lfVarx) {
    try {
      if (lfVarx.substr(-3) != ".js" && lfVarx.length != 1) {
        evaluate(lfVarx); 
      } else if (!isNaN(lfVarx)) {}
    } catch (lfVare) {    }
}

	Christian Holler (:decoder) Reporter
	Comment 4 • 10 years ago

JSBugMon: Bisection requested, result:
=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140424014842" and the hash "25dd3d17a19a".
The "bad" changeset has the timestamp "20140424020245" and the hash "079f4f0ed6a6".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=25dd3d17a19a&tochange=079f4f0ed6a6

	Shu-yu Guo [:shu] Assignee
	Comment 5 • 10 years ago

This is caused by me completely missing handling of fake exit frames, as I thought Baseline didn't generate them.

The current debug mode OSR code depends on the exit frame always being a different ABI frame (i.e. pushed ip) from the stub frame. For fake exit frames generated by ICCall_Native, this is not true, so we are getting unpredictable crashes there. Closing bug since this is technically s-s, though a low rating because there is currently no frontend way to in the browser to attach a Debugger with scripts live on the stack -- and should be that way until the fuzzers calm down.

A fix here is going to be tricky. For normal callVMs, the wrapper persists outside of a particular BaselineScript, so VM calls always have a place to return to. It is the exit frame that the wrapper pushes that debug mode OSR patches. For ICCall_Native's optimized stubs, the fake exit frame is a per-BaselineScript jitcode, so if we destroy the original BaselineScript at debug mode OSR time, there will be no code for the VM call to return to.

I believe the cleanest way, at the cost of a little perf, is to have a real exit frame for ICCall_Native stubs. ICCall_Fallback would compile a little stub that expects a callee already loaded in a particular register (R1 for 32bit, ExtractTemp0 for 64bit), pushes a real exit frame, and does the callABI. When debug mode OSR happens, we then have persistent piece of jitcode to return to and a real exit frame to patch. This comes at the cost of an extra call, and an extra real ABI frame. My hope is that the perf hti from this will be negligible, since this is in Baseline.

	Shu-yu Guo [:shu] Assignee
	Comment 6 • 10 years ago

Push a real frame in ICCall_Native. This incidentally lets us save a little
memory by reusing a generic native wrapper.

	Shu-yu Guo [:shu] Assignee
	Comment 7 • 10 years ago

Good news is this patch doesn't really affect perf.

SunSpider, WITH patch:

============================================
RESULTS (means and 95% confidence intervals)
--------------------------------------------
Total:                 149.0ms +/- 4.7%
--------------------------------------------

  3d:                   25.2ms +/- 1.6%
    cube:                9.2ms +/- 2.7%
    morph:               4.1ms +/- 3.9%
    raytrace:           12.0ms +/- 3.5%

  access:               14.0ms +/- 39.3%
    binary-trees:        2.0ms +/- 2.1%
    fannkuch:            7.4ms +/- 73.5%
    nbody:               2.3ms +/- 1.5%
    nsieve:              2.3ms +/- 2.2%

  bitops:                7.2ms +/- 1.0%
    3bit-bits-in-byte:   0.9ms +/- 2.2%
    bits-in-byte:        1.8ms +/- 3.7%
    bitwise-and:         1.6ms +/- 1.9%
    nsieve-bits:         2.9ms +/- 1.1%

  controlflow:           1.9ms +/- 1.7%
    recursive:           1.9ms +/- 1.7%

  crypto:               12.5ms +/- 2.8%
    aes:                 6.2ms +/- 4.7%
    md5:                 3.5ms +/- 1.8%
    sha1:                2.9ms +/- 0.9%

  date:                 18.9ms +/- 1.3%
    format-tofte:        9.7ms +/- 1.7%
    format-xparb:        9.2ms +/- 1.8%

  math:                  9.8ms +/- 1.4%
    cordic:              2.1ms +/- 1.8%
    partial-sums:        6.2ms +/- 1.5%
    spectral-norm:       1.5ms +/- 1.9%

  regexp:               10.7ms +/- 1.5%
    dna:                10.7ms +/- 1.5%

  string:               48.8ms +/- 1.7%
    base64:              4.7ms +/- 0.9%
    fasta:               5.8ms +/- 1.6%
    tagcloud:           14.4ms +/- 1.9%
    unpack-code:        16.5ms +/- 3.3%
    validate-input:      7.4ms +/- 0.9%

SunSpider, WITHOUT patch:


============================================
RESULTS (means and 95% confidence intervals)
--------------------------------------------
Total:                 149.7ms +/- 4.1%
--------------------------------------------

  3d:                   25.6ms +/- 2.3%
    cube:                9.4ms +/- 3.4%
    morph:               4.0ms +/- 1.9%
    raytrace:           12.2ms +/- 4.3%

  access:               14.1ms +/- 36.8%
    binary-trees:        2.0ms +/- 1.7%
    fannkuch:            7.5ms +/- 69.1%
    nbody:               2.3ms +/- 1.5%
    nsieve:              2.3ms +/- 2.0%

  bitops:                7.2ms +/- 1.3%
    3bit-bits-in-byte:   0.9ms +/- 2.2%
    bits-in-byte:        1.8ms +/- 3.7%
    bitwise-and:         1.7ms +/- 2.6%
    nsieve-bits:         2.9ms +/- 1.0%

  controlflow:           1.9ms +/- 2.0%
    recursive:           1.9ms +/- 2.0%

  crypto:               12.6ms +/- 2.2%
    aes:                 6.2ms +/- 4.4%
    md5:                 3.5ms +/- 1.9%
    sha1:                2.9ms +/- 2.0%

  date:                 19.3ms +/- 1.1%
    format-tofte:        9.8ms +/- 1.5%
    format-xparb:        9.5ms +/- 1.4%

  math:                  9.7ms +/- 1.3%
    cordic:              2.1ms +/- 3.1%
    partial-sums:        6.1ms +/- 1.3%
    spectral-norm:       1.4ms +/- 2.8%

  regexp:               10.7ms +/- 1.6%
    dna:                10.7ms +/- 1.6%

  string:               48.7ms +/- 1.8%
    base64:              4.8ms +/- 1.7%
    fasta:               5.8ms +/- 1.0%
    tagcloud:           14.5ms +/- 2.7%
    unpack-code:        16.4ms +/- 3.9%
    validate-input:      7.3ms +/- 1.1%

Octane, WITH patch:

Richards: 21866
DeltaBlue: 30467
Crypto: 26340
RayTrace: 52021
EarleyBoyer: 32509
RegExp: 2855
Splay: 15687
SplayLatency: 10531
NavierStokes: 31398
PdfJS: 15762
Mandreel: 30692
MandreelLatency: 38348
Gameboy: 48635
CodeLoad: 19178
Box2D: 32532
zlib: 81501
Typescript: 21180
----
Score (version 9): 24549

Octane, WITHOUT patch:

Richards: 21887
DeltaBlue: 29575
Crypto: 26542
RayTrace: 51281
EarleyBoyer: 32580
RegExp: 2793
Splay: 16323
SplayLatency: 11431
NavierStokes: 31335
PdfJS: 15851
Mandreel: 30177
MandreelLatency: 37483
Gameboy: 45719
CodeLoad: 19183
Box2D: 32877
zlib: 81112
Typescript: 20746
----
Score (version 9): 24482

	Shu-yu Guo [:shu] Assignee
	Comment 8 • 10 years ago

I'll also add assertions that, while patching return addresses for debug mode OSR, each frame's return address is different.

	Jan de Mooij [:jandem]
	Comment 9 • 10 years ago

I don't really understand the patch. AFAICS, the problem is this:

(1) There's a Call_Native stub for the call to evaluate(..)
(2) Debug mode OSR patches the stub pointer in the stub frame to the Call_Fallback stub.
(3) The callWithABI returns to the Call_Native stub, where it crashes in the code emitted for EmitEnterTypeMonitorIC(masm), because the stub pointer is not a Call_Native stub but a Call_Fallback stub.

What happens with this patch instead of (3)? Will we return to the returnFromStubOffset_ code in Call_Fallback? The code there looks wrong for native calls.

Could the debug OSR code attach a Call_Native stub in this case, based on the old one? Then (3) wouldn't crash and it's zero overhead in the no-debug-mode case.

(In reply to Shu-yu Guo [:shu] from comment #6)
> Push a real frame in ICCall_Native. This incidentally lets us save a little
> memory by reusing a generic native wrapper.

There's only a single copy of the Call_Native stub code so this doesn't matter.

	Shu-yu Guo [:shu] Assignee
	Comment 10 • 10 years ago

This patch reimplements how stub frames are patched. Instead of going to some
special fixer like the Ion bailout fixer, this patch clones optimized stubs on
the stack and uses the old stub's JitCode, thus keeping it alive. Fallback
stubs don't need to be cloned, since they always exist.

This should be cleaner than the old approach and easier to reason about, since
we will always return to the same stub code.

	Shu-yu Guo [:shu] Assignee
	Comment 11 • 10 years ago

Part 1 obviates the need for a special return-from-stub fixer originally added
to patch stub frames, so I reverted the change.

	Shu-yu Guo [:shu] Assignee
	Comment 13 • 10 years ago

Comment on attachment 8414085 [details] [diff] [review]
Part 1: Clone on-stack optimized stubs when doing debug mode OSR

Review of attachment 8414085 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/BaselineJIT.cpp
@@ +638,5 @@
>  
>  void
> +BaselineScript::adoptFallbackStubsForDebugModeOSR(BaselineScript *other)
> +{
> +}

Oops, forgot to remove this.

::: js/src/jit/BaselineJIT.h
@@ +304,5 @@
>      }
>  
>      void copyICEntries(JSScript *script, const ICEntry *entries, MacroAssembler &masm);
>      void adoptFallbackStubs(FallbackICStubSpace *stubSpace);
> +    void adoptFallbackStubsForDebugModeOSR(BaselineScript *other);

And this. Ignore these please.

	Jan de Mooij [:jandem]
	Comment 14 • 10 years ago

Comment on attachment 8414085 [details] [diff] [review]
Part 1: Clone on-stack optimized stubs when doing debug mode OSR

Review of attachment 8414085 [details] [diff] [review]:
-----------------------------------------------------------------

Great, r=me with nits addressed and changes to BaselineJIT.* reverted.

::: js/src/jit/BaselineDebugModeOSR.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include <malloc.h>

Is this include necessary?

@@ +581,5 @@
>      for (size_t i = 0; i < entries.length(); i++) {
>          JSScript *script = entries[i].script;
> +
> +        if (!RecompileBaselineScriptForDebugMode(cx, script) ||
> +            !CloneOldBaselineStub(cx, entries[i]))

What happens if a single stub is on the stack multiple times, will we clone it multiple times? It's harmless but would be better to avoid it.

::: js/src/jit/BaselineIC.h
@@ +3166,5 @@
>      }
> +
> +    static ICGetElem_NativePrototypeCallNative *Clone(
> +             JSContext *cx, ICStubSpace *space, ICStub *firstMonitorStub,
> +             ICGetElem_NativePrototypeCallNative &other)

The indentation here looks unusual. Could you also move the definition to the cpp file? There's too much code in the header file (same for MIR.h), I've been trying to add more to the cpp file. This also doesn't need inlining etc.

Same for the methods below.

	Jan de Mooij [:jandem]
	Comment 15 • 10 years ago

Comment on attachment 8414086 [details] [diff] [review]
Part 2: Revert bailout return offset changes in Baseline can-call fallbacks

Review of attachment 8414086 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

	Shu-yu Guo [:shu] Assignee
	Comment 16 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e3480e87428d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/caad9ce31941
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c7a4b24fbe

	Carsten Book [:Tomcat]
	Comment 17 • 10 years ago

landed on m-c 
https://hg.mozilla.org/mozilla-central/rev/e3480e87428d
https://hg.mozilla.org/mozilla-central/rev/caad9ce31941
https://hg.mozilla.org/mozilla-central/rev/c3c7a4b24fbe

	Christian Holler (:decoder) Reporter
	Comment 18 • 10 years ago

JSBugMon: This bug has been automatically verified fixed.