948459 - [jsdbg2] Assertion failure: isNonEvalFunctionFrame(), at ../vm/Stack.h:834 or Crash [@ IsAboutToBeFinalized<JSObject>] or Crash [@ address]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision df82be9d89a5 (run with --fuzzing-safe):


gcPreserveCode()
gczeal(9, 2);
function test2() {
  var g = newGlobal();
  var dbg = new Debugger;
  var gw = dbg.addDebuggee(g);
  g.eval("function h() { debugger; }");
  dbg.onDebuggerStatement = function (hframe) {
    var env = hframe.older.environment;
  };
  g.eval("(function () { h(); return []; })();");
} test2();
function test() {
  var TZ_DIFF_RAW = GetRawTimezoneOffset();
  var TIME_NOW = now.valueOf();
  function GetRawTimezoneOffset() {
    var t1 = new Date(2000, 1, 1).getTimezoneOffset();
    var t2 = new Date(2000, 1 + 6, 1).getTimezoneOffset();
  }
} test();

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Needinfo from jimb because this is Debugger-related.

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/f5eca934fa16
user:        Jan de Mooij
date:        Fri Jun 21 08:28:06 2013 +0200
summary:     Bug 881902 - Remove ContextStack and StackSpace. r=luke,njn

This iteration took 2.239 seconds to run.

Comment 4

[Jim Blandy :jimb]

OOH LURVLEY, looks like it could be related to bug 947773.

Comment 5

[Jim Blandy :jimb]

It seems that a scope object is being associated with a given stack frame in the debugger's liveScopes table, but that the liveScopes entry isn't being cleared when the stack frame is popped.

The crash occurs because DebugScopes::sweep calls maybeSuspendedGenerator on a StackFrame * abstract frame pointer, and the frame claims to be a generator but not a function frame, which is bogus. This is because the 'flags' field of the alleged stack frame is actually the integer constant 2000, being pushed by the 'test' function in the test case.

Not s-s, because it only occurs when Debugger is active, and Debugger isn't exposed to content.

Comment 6

[Jim Blandy :jimb]

The problem here is that DebugScopes::onPopCall only removes an entry from liveScopes mapping a synthesized CallObject to a non-heavyweight frame if that frame has an entry in missingScopes. If the DebugScopeObject is GC'd but its synthesized CallObject is not, then DebugScopes::sweep will remove the missingScopes entry, but not the liveScopes entry. Then, onPopCall can't find the liveScopes entry to remove it, so the entry's AbstractFramePtr becomes a dangling pointer. The next DebugScopes::sweep call tries to use it, and (if we're lucky) we crash.

Comment 7

[Jim Blandy :jimb]

(I don't know why a DebugScopeObject would be GC'd, but its synthesized CallObject not. I had thought the DebugScopeObject was the only thing that ever referred to the CallObject.)

Comment 8

[Luke Wagner [:luke]]

Wow, nice detectiving!  It seems like the fix is to add a 'liveScopes.remove(&debugScope.scope())' right before we remove the about-to-be-finalized debug scope from missingScopes in DebugScopes::sweep.  Does that make sense to you too?

Comment 9

[Luke Wagner [:luke]]

I checked and indeed

     for (MissingScopeMap::Enum e(missingScopes); !e.empty(); e.popFront()) {
-        if (IsObjectAboutToBeFinalized(e.front().value().unsafeGet()))
+        DebugScopeObject **debugScope = e.front().value().unsafeGet();
+        if (IsObjectAboutToBeFinalized(debugScope)) {
+            liveScopes.remove(&(*debugScope)->scope());
             e.removeFront();
+        }
     }

fixes the crash.  However, part 4 in bug 948459 actually removes all this special-case generator junk (by making all generator variables 'aliased') and removes the cycle that necessitates the sweeping here.  Instead, we could have missingScopes mark its values which would remove this sweeping code, the ReadBarriered, and make this all simpler to reason about.

Comment 10

[Jim Blandy :jimb]

(In reply to Luke Wagner [:luke] from comment #8)
> Wow, nice detectiving!  It seems like the fix is to add a
> 'liveScopes.remove(&debugScope.scope())' right before we remove the
> about-to-be-finalized debug scope from missingScopes in DebugScopes::sweep. 
> Does that make sense to you too?

Yes, I was thinking that might be a fix - if we know that the synthetic call object won't be used once the DebugScopeObject for which it was created is gone. But the synthetic call object is marked, while the DebugScopeObject isn't; perhaps that's just a fluke, but if it's not, then it does suggest that that condition doesn't hold.

I'll spend another hour to see if I can understand why the call object is marked. But then let's land this fix, until bug 948459 lands.

Comment 11

[Jim Blandy :jimb]

(In reply to Luke Wagner [:luke] from comment #9)
> fixes the crash.  However, part 4 in bug 948459 actually removes all this

You meant bug 927782, right?

Comment 12

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #10)
> I'll spend another hour to see if I can understand why the call object is
> marked. But then let's land this fix, until bug 948459 lands.

Conservative marker. !@#$%^&*

Comment 13

[Luke Wagner [:luke]]

lol.  I hate that thing.  Btw, here's a shorter-running testcase that reproduces the assertion:

gcPreserveCode()
var g = newGlobal();
var dbg = new Debugger;
var gw = dbg.addDebuggee(g);
g.eval("function h() { debugger; }");
dbg.onDebuggerStatement = function (hframe) {
    hframe.older.environment;
};
gczeal(9, 2);
g.eval("(function () { h(); return []; })();");
(function() {
  var a = new Date(2000).getTimezoneOffset();
  var b = new Date(2000).getTimezoneOffset();
})();

Comment 14

[Jim Blandy :jimb]

Attachment: Reliably clean up DebugScopes::liveScopes when DebugScopeObjects are finalized.

I haven't included the test case in this patch, because:
1) it takes seven seconds to run, because it depends on gczeal
2) the bug depends on the conservative scanner marking one particular object but not another, so it will have very limited value as a regression test
3) there isn't another way to get the one object marked (that's kind of the reason the bug was present in the first place - nobody was expecting that)
4) bug 927782 is going to remove all this code anyway.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=038595afcc5e

Comment 15

[Jim Blandy :jimb]

(In reply to Luke Wagner [:luke] from comment #13)
> lol.  I hate that thing.  Btw, here's a shorter-running testcase that
> reproduces the assertion:

That gets it down to two seconds; better! I still would argue that it's not very valuable, because it depends on conservative GC finding a particular pointer.

Comment 17

[Luke Wagner [:luke]]

Comment on attachment 8346096 [details] [diff] [review]
Reliably clean up DebugScopes::liveScopes when DebugScopeObjects are finalized.

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

Great!  Thanks again for the digging into this.

::: js/src/vm/ScopeObject.cpp
@@ +1835,5 @@
> +#if DEBUG
> +    /* Now this frame should not appear in liveScopes. */
> +    for (LiveScopeMap::Enum e(scopes->liveScopes); !e.empty(); e.popFront())
> +        JS_ASSERT(e.front().value() != frame);
> +#endif

This could produce pretty massive O(n^2) runtime behavior (for deep stacks) so I'd be a little worried about this seriously increasing mochtest times.

Comment 18

[Luke Wagner [:luke]]

Note, bug 927782 will just frob the comment but otherwise leave the weak behavior of missingScopes intact so we'd want to remove it in a followup bug.

Comment 19

[Jim Blandy :jimb]

Hmm, that assertion I added tripped up a bunch of stuff with generational GC enabled.

Comment 20

[Luke Wagner [:luke]]

Oh, hah, e.front().value() will trigger a read barrier which I get is bad during sweep :)

Comment 21

[Jim Blandy :jimb]

Pushed without the assertion, as discussed on IRC:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52e299230d26

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/52e299230d26

Comment 25

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.