Closed Bug 1331452 Opened 3 years ago Closed 3 years ago

jit-test/tests/debug/wasm-06.js failures with CGC builds

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jandem, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

I got this on a m-i push. I can reproduce it reliably like this:

JS_GC_ZEAL=14,5 ../jit-test/jit_test.py dist/bin/js wasm-06.js --tbpl

It crashes all test runs, even with --no-baseline --no-ion.
Flags: needinfo?(ydelendik)
I can reproduce it when I run the test below:

$ JS_GC_ZEAL=14,5 dist/bin/js --no-baseline test.js
Bus error: 10

We crash under DebugEnvironments::checkHashTablesAfterMovingGC, when checking missingEnvs.

function runWasmWithDebugger(wast, lib, init) {
    let g = newGlobal('');
    let dbg = new Debugger(g);
    g.eval(`
var wasm = wasmTextToBinary('${wast}');
var lib = ${lib || 'undefined'};
var m = new WebAssembly.Instance(new WebAssembly.Module(wasm), lib);`);
    init(dbg, g);
    let result = g.eval("m.exports.test()");
}

var onEnterFrameCalled, onLeaveFrameCalled, onExceptionUnwindCalled, testComplete;
runWasmWithDebugger(
    '(module (func (result i32) (i32.const 42)) (export "test" 0))', undefined,
    function (dbg) {
        var wasmScript = dbg.findScripts().filter(s => s.format == 'wasm')[0];
        var evalFrame;
        dbg.onEnterFrame = function (frame) {
            let env = frame.environment;
       };
    }
);
Flags: needinfo?(shu)
Attached patch Patch (obsolete) — Splinter Review
No idea if this makes sense, but it fixes the crashes locally.

Can scope be null there?
Attachment #8827263 - Flags: review?(shu)
Blocks: 1326067
(In reply to Jan de Mooij [:jandem] from comment #0)
> I got this on a m-i push.

And I had that push backed out because of this. My patch somehow makes this fail more reliably :/
Blocks: 1286948
(In reply to Jan de Mooij [:jandem] from comment #2)
> Created attachment 8827263 [details] [diff] [review]
> Patch
> 
> No idea if this makes sense, but it fixes the crashes locally.
> 
> Can scope be null there?

That patch doesn't look right to me. Which patch makes the code in comment 1 crash reliably? I can take a look.
Flags: needinfo?(shu)
(In reply to Shu-yu Guo [:shu] from comment #4)
> (In reply to Jan de Mooij [:jandem] from comment #2)
> > Created attachment 8827263 [details] [diff] [review]
> > Patch
> > 
> > No idea if this makes sense, but it fixes the crashes locally.
> > 
> > Can scope be null there?
> 
> That patch doesn't look right to me. Which patch makes the code in comment 1
> crash reliably? I can take a look.

Ah, bug 1326067.
The bug is that environment objects in the missingEnvs maps expects their
scopes to be kept alive. For other environment objects (e.g. CallObjects,
LexicalEnvironmentObjects, etc), they have strong references to their scopes
directly or indirectly. WasmFunctionCallObjects didn't, so its scopes were
getting collected, causing the crash when compacting the missingEnvs map.
Attachment #8827664 - Flags: review?(ydelendik)
Attachment #8827263 - Attachment is obsolete: true
Attachment #8827263 - Flags: review?(shu)
Comment on attachment 8827664 [details] [diff] [review]
Keep scope in a slot in WasmFunctionCallObjects.

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

::: js/src/vm/EnvironmentObject.h
@@ +427,5 @@
>  typedef MutableHandle<ModuleEnvironmentObject*> MutableHandleModuleEnvironmentObject;
>  
>  class WasmFunctionCallObject : public EnvironmentObject
>  {
> +    static const unsigned SCOPE_SLOT = 1;

Thanks for the fix!  It'd be nice to add a comment explaining why this otherwise dead-looking edge is necessary.
Comment on attachment 8827664 [details] [diff] [review]
Keep scope in a slot in WasmFunctionCallObjects.

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

Looks good.
Attachment #8827664 - Flags: review?(ydelendik) → review+
Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e24ffa06137c
Keep scope in a slot in WasmFunctionCallObjects. (r=yury)
https://hg.mozilla.org/mozilla-central/rev/e24ffa06137c
https://hg.mozilla.org/mozilla-central/rev/e6541dbce2b2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Flags: needinfo?(ydelendik)
You need to log in before you can comment on or make changes to this bug.