713226 - Assertion failure: (static_cast<Cell *>(thing)->isMarked()), at jsgc.cpp:3556

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test asserts on larch branch (incremental GC) revision c5b90ea7e475 (options -m -n -a):


gczeal(4);
var optionNames = options().split(',');
  for (var i = 0; i < optionNames.length; i++)
    var optionName = optionNames[i];
      options(optionName);
evaluate("\
function addDebug(g, id) {\
    var debuggerGlobal = newGlobal('new-compartment');\
    debuggerGlobal.debuggee = g;\
    debuggerGlobal.id = id;\
    debuggerGlobal.print = function (s) { (g) += s; };\
    debuggerGlobal.eval('var dbg = new Debugger(debuggee);dbg.onDebuggerStatement = function () { print(id); debugger; };');\
    return debuggerGlobal;\
}\
var base = newGlobal('new-compartment');\
var top = base;\
for (var i = 0; i < 8; i++ )\
    top = addDebug(top, i);\
base.eval('debugger;');\
");

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I can't find this rev on the larch branch. Is it a typo?

I tried this testcase on different revisions. I found one where it asserts with the |started| assertion that was fixed in bug 713214. I couldn't get an isMarked assertion. Can you look at this one again, Christian?

Comment 2

[Christian Holler (:decoder)]

The revision is a mozilla-central revision so I checked this again and the assertion is indeed on mozilla-central. So the mistake I made is specifying the wrong branch. I confirmed this being on m-c revision d98fbf3cbd71.

Comment 3

[Christian Holler (:decoder)]

This also involves a debugger, Ccing jsdbg2 devs.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm pretty sure this is unrelated to the debugger. It has to do with TI and call objects.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

I guess we don't have type information for JSOP_SETNAME opcodes.

Comment 6

[Brian Hackett [Laid off!]]

Comment on attachment 592892 [details] [diff] [review]
patch

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

::: js/src/methodjit/Compiler.cpp
@@ +5635,4 @@
>  #ifdef JSGC_INCREMENTAL_MJ
> +    /* Write barrier. We only have type information for JSOP_SETPROP. */
> +    if (cx->compartment->needsBarrier() &&
> +        (!types || op != JSOP_SETPROP || types->propertyNeedsBarrier(cx, id)))

This should test for op == JSOP_SETNAME, so it does not slow path JSOP_SETMETHOD.  (I think SETMETHOD will always hit propertyNeedsBarrier anyways, but that's a little subtle).

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f623a11c6cb

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/71f5bf4df2f6 - one of the six in that push was crashing in js::gc::Mark<JSString>

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3500272283ed

After I pushed I noticed that I forgot to address Brian's review comment. When I made the obvious change, the test case started to fail again. I'll look into this now.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24bf37c95906

Never mind, it works now.

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/3500272283ed
https://hg.mozilla.org/mozilla-central/rev/24bf37c95906

Comment 12

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug713226.js.