493366 - Assertion failure: (cx)->requestDepth || (cx)->thread == (cx)->runtime->gcThread, at mozilla/js/src/jsapi.cpp:5196

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

I get the assertion when running mochitest on a x86_64 linux debug build.
#7  0x00002aaaaadcd79c in JS_Assert (s=<value optimized out>, file=<value optimized out>, ln=<value optimized out>)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/js/src/jsutil.cpp:69
#8  0x00002aaaaad26be6 in JS_CallFunctionValue (cx=0x33af790, obj=0x4054740, fval=67461760, argc=0, argv=0x0, 
    rval=0x7fff8f095780) at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/js/src/jsapi.cpp:5196
#9  0x00002aaab0af0a61 in nsXBLProtoImplAnonymousMethod::Execute (this=0x17ee2b0, aBoundElement=0x2a4eb40)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/content/xbl/src/nsXBLProtoImplMethod.cpp:332
#10 0x00002aaab0afe9d5 in nsBindingManager::ProcessAttachedQueue (this=0x328c380, aSkipSize=0)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/content/xbl/src/nsBindingManager.cpp:1015
#11 0x00002aaab0afea6e in nsBindingManager::EndOutermostUpdate (this=0x328c380)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/content/xbl/src/nsBindingManager.cpp:1677
#12 0x00002aaab097abde in nsDocument::EndUpdate (this=0x3539960, aUpdateType=<value optimized out>)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/content/base/src/nsDocument.cpp:3709
#13 0x00002aaab0a7492f in nsHTMLDocument::EndUpdate (this=0x3ec9, aUpdateType=16073)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/content/html/document/src/nsHTMLDocument.cpp:3045

Comment 1

[Igor Bukanov]

Can you attach the full stack trace?

Comment 2

[Mats Palmgren (inactive)]

Attachment: Full stack trace

It crashes consistently by loading the mochitest http://localhost:8888/tests/browser/components/feeds/test/test_bug408328.html

Comment 3

[Igor Bukanov]

This bug is a regression from the bug 413774. Here is a relevant source of nsXBLProtoImplAnonymousMethod::Execute that triggers this bug, http://hg.mozilla.org/mozilla-central/file/998d6f6e95b7/content/xbl/src/nsXBLProtoImplMethod.cpp#l266

   309   JSAutoRequest ar(cx);
...
   320   // Use nsCxPusher to make sure we call ScriptEvaluated when we're done.
   321   nsCxPusher pusher;
   322   NS_ENSURE_STATE(pusher.Push(aBoundElement));
   ...
   331     ok = ::JS_CallFunctionValue(cx, thisObject, OBJECT_TO_JSVAL(method),

   332                                 0 /* argc */, nsnull /* argv */, &retval);


What happens here is that pusher.Push(aBoundElement) pushes a JSContext instance that is different from the one stored in the cx variable. Since the bug 413774 that lead to suspending the previous top of the stack, that is, the cx.

Now, this bug may not be a true regression from the bug 413774 as the latter could just expose some latent problem. For example, shouldn't the code use the pushed context by pusher.Push(aBoundElement), not the cx locals, when doing JS_CallFunctionValue?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Uh... how do cx and the context the pusher pushes end up different?  Both look like they go through the owner document, then one uses the script global object, the other the script handling object.  Is that the issue?  After that they're identical again.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

bug 413774 did land long time ago. This assertion is something recent.

(In reply to comment #4)
> Uh... how do cx and the context the pusher pushes end up different? 
Yeah, I can't see how that could happen.

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> bug 413774 did land long time ago. This assertion is something recent.
> 
> (In reply to comment #4)
> > Uh... how do cx and the context the pusher pushes end up different? 
> Yeah, I can't see how that could happen.

Right, the context instance is the same, I was too quick to assume that. So the issue is unrelated to the bug 413774.

The real cause is the following code in  XPCJSContextStack::Push, http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcthreadcontext.cpp#134 :

141         XPCJSContextInfo & e = mStack[mStack.Length() - 2];
...
149                     nsIPrincipal* globalObjectPrincipal =
150                         GetPrincipalFromCx(cx);
...
153                         nsIPrincipal* subjectPrincipal = ssm->GetCxSubjectPrincipal(cx);
154                         PRBool equals = PR_FALSE;
155                         globalObjectPrincipal->Equals(subjectPrincipal, &equals);
156                         if(equals)
157                         {
158                             return NS_OK;
159                         }
...
162             }
163 
164             e.frame = JS_SaveFrameChain(e.cx);
165 
166             if(JS_GetContextThread(e.cx))
167                 e.requestDepth = JS_SuspendRequest(e.cx);

Here the check globalObjectPrincipal->Equals returns false leading to fallthrough to the code suspending the e.cx and the current cx. That points directly to the bug 461861.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Ah, fun, I caused this.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: not tested

Comment 9

[Boris Zbarsky [:bzbarsky]]

This needs to block 1.9.1.

Comment 10

[Igor Bukanov]

Comment on attachment 378421 [details] [diff] [review]
not tested

>-            if(JS_GetContextThread(e.cx))
>+            if(JS_GetContextThread(e.cx) && e.cx != cx)

I suspect that JS_GetContextThread returning null here would be a bug as no code in the FF codebase calls JS_ClearContextThread. But even if it is possible it is better to avoid this function call that a compiler cannot optimize when e.cx == cx.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

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

Sorry, should have spotted this when I reviewed the last patch.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/0f4de606acd7

Thanks Igor!

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/98f526c4eb51

Comment 15

[Daniel Veditz [:dveditz]]

Needed if we take bug 461861

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Olli, I think this works for trunk and 1.9.1 for you now?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Yes, I pushed the patch to trunk and 1.9.1

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Based on comment 17 everything should be fine now. I can't test on my own due to the lack of a 64bit Linux system. Marking as verified.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

This was 1.9.0.12fixed in Bug 461861

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Marking verified for 1.9.0.12 based on Bug 461861.