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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Jan de Mooij [:jandem]]

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;
       };
    }
);

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

No idea if this makes sense, but it fixes the crashes locally.

Can scope be null there?

Comment 3

[Jan de Mooij [:jandem]]

(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 :/

Comment 4

[Shu-yu Guo [:shu]]

(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.

Comment 5

[Shu-yu Guo [: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.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Keep scope in a slot in WasmFunctionCallObjects.

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.

Comment 7

[Luke Wagner [:luke]]

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 8

[Yury Delendik (:yury)]

Comment on attachment 8827664 [details] [diff] [review]
Keep scope in a slot in WasmFunctionCallObjects.

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

Looks good.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e24ffa06137c
Keep scope in a slot in WasmFunctionCallObjects. (r=yury)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6541dbce2b2
Followup: hazard fix.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e24ffa06137c
https://hg.mozilla.org/mozilla-central/rev/e6541dbce2b2