619565 - "Assertion failure: scopeChain_ == calleeValue().toObject().getParent()"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: scopeChain_ == calleeValue().toObject().getParent(), at js/src/jsinterpinlines.h:122

Only happens when the method jit is enabled.

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Jesse Ruderman]

Doesn't crash in opt, fwiw.

Comment 3

[Makoto Kato [:m_kato]]

Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitests-2/5 on 2011/02/19 00:52:54
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1298105574.1298106333.15139.gz

Assertion failure: scopeChain_ == calleeValue().toObject().getParent(), at /builds/slave/cen-osx64-dbg/build/js/src/jsinterpinlines.h:124
NEXT ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/ajax/prototype/test_Prototype.html | Exited with code 1 during test run
INFO | automation.py | Application ran for: 0:11:13.645725
INFO | automation.py | Reading PID log: /var/folders/H5/H5TD8hgwEqKq9hgKlayjWU+++TM/-Tmp-/tmp9pZb8Ypidlog
PROCESS-CRASH | /tests/dom/tests/mochitest/ajax/prototype/test_Prototype.html | application crashed (minidump found)
Operating system: Mac OS X
                  10.6.6 10J567
CPU: amd64
     family 6 model 23 stepping 10
     2 CPUs

Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash address: 0x0

Thread 0 (crashed)
 0  XUL!JS_Assert [jsutil.cpp:657c2a92ee2b : 80 + 0x5]
    rbx = 0x294f9a90   r12 = 0x27351f78   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0x057abf08   rip = 0x01ab2aa3   rsp = 0x5fbfaf30   rbp = 0x5fbfaf50
    Found by: given as instruction pointer in context
 1  XUL!JSStackFrame::resetInvokeCallFrame [jsinterpinlines.h:657c2a92ee2b : 124 + 0x4d]
    rbx = 0x294f9a90   r12 = 0x27351f78   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0x057abf08   rip = 0x01a966c1   rsp = 0x5fbfaf60   rbp = 0x5fbfaf80
    Found by: call frame info
 2  XUL!js::InvokeSessionGuard::invoke [jsinterpinlines.h:657c2a92ee2b : 601 + 0x8]
    rbx = 0x057abf20   r12 = 0x27351f78   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0x057abf08   rip = 0x0193f271   rsp = 0x5fbfaf90   rbp = 0x5fbfb000
    Found by: call frame info
 3  XUL!array_extra [jsarray.cpp:657c2a92ee2b : 2851 + 0x12]
    rbx = 0x057abf20   r12 = 0x27351f78   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0x057abf08   rip = 0x01938ad1   rsp = 0x5fbfb010   rbp = 0x5fbfb1d0
    Found by: call frame info
 4  XUL!array_forEach [jsarray.cpp:657c2a92ee2b : 2908 + 0x14]
    rbx = 0x057abe90   r12 = 0x27351f78   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0x057abf08   rip = 0x01938e72   rsp = 0x5fbfb1e0   rbp = 0x5fbfb200
    Found by: call frame info
 5  0x119a94ff2
    rbx = 0x057abe90   r12 = 0x27351f78   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0x057abf08   rip = 0x19a94ff3   rsp = 0x5fbfb210   rbp = 0x5fbfb290
    Found by: call frame info

Comment 4

[Daniel Holbert [:dholbert]]

I filed Bug 635462 on the apparently new randomorange with this assertion-failure described in Comment 3.

Comment 5

[Jeff Walden [:Waldo]]

100 inline void
101 JSStackFrame::resetInvokeCallFrame()
102 {
103     /* Undo changes to frame made during execution; see initCallFrame */
104 
105     if (hasArgsObj())
106         args.nactual = argsObj().getArgsInitialLength();
107 
108     JS_ASSERT(!(flags_ & ~(JSFRAME_FUNCTION |
109                            JSFRAME_OVERFLOW_ARGS |
110                            JSFRAME_UNDERFLOW_ARGS |
111                            JSFRAME_HAS_CALL_OBJ |
112                            JSFRAME_HAS_ARGS_OBJ |
113                            JSFRAME_OVERRIDE_ARGS |
114                            JSFRAME_HAS_PREVPC |
115                            JSFRAME_HAS_RVAL |
116                            JSFRAME_HAS_SCOPECHAIN |
117                            JSFRAME_HAS_ANNOTATION |
118                            JSFRAME_FINISHED_IN_INTERPRETER)));
119     flags_ &= JSFRAME_FUNCTION |
120               JSFRAME_OVERFLOW_ARGS |
121               JSFRAME_HAS_PREVPC |
122               JSFRAME_UNDERFLOW_ARGS;
123 
124     JS_ASSERT_IF(!hasCallObj(), scopeChain_ == calleeValue().toObject().getParent());
125     JS_ASSERT_IF(hasCallObj(), scopeChain_ == callObj().getParent());
126     if (hasCallObj())
127         scopeChain_ = callObj().getParent();
128 
129     JS_ASSERT(exec.fun == calleeValue().toObject().getFunctionPrivate());
130     JS_ASSERT(!hasImacropc());
131     JS_ASSERT(!hasHookData());
132     JS_ASSERT(annotation() == NULL);
133     JS_ASSERT(!hasCallObj());
134 }

The |flags_&= ...| assignment clears out the JSFRAME_HAS_CALL_OBJ bit, so it must be the case that |!hasCallObj()|.  So the first assertion is the only one that can be hit.  Right?

Now the second assertion.  If the bit actually got cleared, isn't it the case that the second assertion's consequent is actually the correct assertion to make, if it's correct?

Now the guarded assignment after it.  Since |!hasCallObj()| it seems impossible to hit this.  But supposing it were true: wouldn't it just duplicate the consequent of the second assertion?

My thought is that the assertions and assignment should all move above the flag-clearing assignment, and the assignment should change to assign what the !hasCallObj() assertion requires it to be (which is what initCallFrame sets it to be as well).  I think that fixes the logic.  But right now it looks very dodgy, and I think it's the cause of the orange with bug 631135 (although why that would have triggered it I'm not 100% sure).

But even beyond that I think there are problems here, because with those changes I can still sometimes hit the assertion that caused bug 635462 to be filed.  I'll investigate more on Tuesday, likely.

Comment 6

[Luke Wagner [:luke]]

You're right that something is wrong, but I think the story is a bit different:

After every execution of the callee by the optimized() path in InvokeSessionGuard::invoke, PutActivationObjects is called, which will unset JSFRAME_HAS_{ARGS,CALL}_OBJ.  Thus, resetInvokeCallFrame could actually assert !has{Args,Call}Obj() at entry (or, equivalently, remove their flags from the big list).  This also means the "if (hasArgsObj())" on entry is dead code.

However, even after PutCallObject, the call objects remains on fp->scopeChain_ so, even though the condition is the wrong one, the then-branch of the "if (hasCallObj())" is wanted.  One option is to replace "if (hasCallObj())" with "if (scopeChain_->isCall())", but I think the "righter" fix is to fix clearCallObj:

  void JSStackFrame::clearCallObj() {
    JS_ASSERT(hasCallObj());
    JS_ASSERT(scopeChain_->getPrivate() == this);
    scopeChain_ = scopeChain_->getParent();
    flags_ ^= JSFRAME_HAS_CALL_OBJ;
  }

(And switch the order of callobj.setPrivate(NULL) in js_PutCallObject.)  This is analogous to what JSSStackFrame::clearArgsObj() already does and avoids code from observing a frame that says it doesn't have a call object but actually does.

Comment 7

[Luke Wagner [:luke]]

(In reply to comment #6)
So another problem with the ideal world in comment 6 is that, in between InvokeSessionGuard::invoke calling PutActivationObjects and the next call to InvokeSessionGuard::invoke (or ~InvokeSessionGuard), some random code could ask for the scope chain which would create a call object for the frame (which would dangle, causing call_trace crashes a la bug 634542).

These bugs pop up because InvokeSessionGuard is rather unnatural... makes me want to rip it out.

Comment 8

[Jeff Walden [:Waldo]]

Looked more closely at comment 6, this does sound nice to me, although the extra store plausibly could impact perf, and you couldn't really get rid of it in JITs either.  Still, it seems a less edge-cased world, so I'm cool with it.

We talked over comment 7 and the idea of ripping out ISG entirely.  I hit several hundred or so uses of it in startup, and the ones I was hitting here were all in tabbrowser.xml or somesuch.  So that's a fair number of uses right there which would be best sped up.  And given how easy it is to use [].forEach and friends, particularly even in old browsers not supporting them (because we publicize a drop-in replacement), I expect this use will only increase.  So while I'm generally sympathetic to removing complexity, this bit seems well-warranted to me, and I think to Luke after we discussed this.

Luke said he'd take this, seems best since this is his code (even if I reviewed it the first time through).

Comment 9

[Luke Wagner [:luke]]

Attachment: remove no-debug-mode restriction (get assert) [Checked in: Comment 14]

To make a shell testcase, I want to use evalInFrame which requires debugmode which will turn off the InvokeSessionGuard.  After bug 612329 we don't cache the code ptr so it seems like this restriction isn't necessary so this patch removes it.  Patch with fix and testcase next.

Comment 10

[Luke Wagner [:luke]]

Attachment: fix [Checked in: Comment 14]

Also fixes comment 0.

Comment 11

[Luke Wagner [:luke]]

I'm nominating because the patch is relatively simple and it seems like this could hit for any Array.{forEach,map,reduce,etc} that touches the DOM (and calls GetFrameDocument).

Comment 12

[Luke Wagner [:luke]]

Green on try; approval still pending.

Comment 13

[Luke Wagner [:luke]]

Attachment: fix part 2 [Checked in: Comment 14]

I forgot to address the non-crash issue found in comment 7 (which is trivial to fix).  The test case fails on TM tip and passes with the patch.

Comment 14

[Luke Wagner [:luke]]

http://hg.mozilla.org/mozilla-central/rev/032e7061575a