STR: - run fennectrolysis tip - set up weave account - click on remote tabs link from start page See attached stack.

Bug 552266 seems to have changed how JSContexts are handled in the JSEng. I'm not sure what should be done in this case. Igor, any hints?

(In reply to comment #1) > Bug 552266 seems to have changed how JSContexts are handled in the JSEng. > I'm not sure what should be done in this case. > Igor, any hints? The bug indicate that the code violates the rule that BeginRequest/EndRequest for different contexts should follow LIFO ordering. I.e. something like JS_BeginRequest(cx2); JS_BeginRequest(cx1); JS_EndRequest(cx1); JS_EndRequest(cx2); is ok while something like: JS_BeginRequest(cx2); JSAutoRequest req(cx1); JS_EndRequest(cx2); is not since the destructor for req that calls JS_EndRequest(cx1) will run after JS_EndRequest(cx2) leading to something like: JS_BeginRequest(cx2); JS_BeginRequest(cx1); JS_EndRequest(cx2); JS_EndRequest(cx1); Hopefully the stack trace should be sufficient to locate and fix the place

Probably http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsInProcessTabChildGlobal.cpp#346

(In reply to comment #3) > Probably > http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsInProcessTabChildGlobal.cpp#346 Yes, this is indeed the bug but the the reason for it is a sequence like: JS_BeginRequest(some_other_cx); JS_BeginRequest(mCx); // from JSAutoRequest ar(mCx); JS_BeginRequest(some_other_cx); // nsContentUtils::ThreadJSContextStack()->Push(mCx); A quick fix would be to replace 347 JSAutoRequest ar(mCx); 348 jsval retval; 349 JSObject* global = nsnull; 350 mGlobal->GetJSObject(&global); 351 if (!global) { 352 return; 353 } 354 355 JSPrincipals* jsprin = nsnull; 356 mPrincipal->GetJSPrincipals(mCx, &jsprin); 357 nsContentUtils::XPConnect()->FlagSystemFilenamePrefix(url.get(), PR_TRUE); 358 nsContentUtils::ThreadJSContextStack()->Push(mCx); with { JSAutoRequest ar(mCx); jsval retval; JSObject* global = nsnull; mGlobal->GetJSObject(&global); if (!global) { return; } JSPrincipals* jsprin = nsnull; mPrincipal->GetJSPrincipals(mCx, &jsprin); nsContentUtils::XPConnect()->FlagSystemFilenamePrefix(url.get(), PR_TRUE); } nsContentUtils::ThreadJSContextStack()->Push(mCx); But this is suboptimal as the request will be entered twice. A better way would be to just call nsContentUtils::ThreadJSContextStack()->Push(mCx). But that may have bad security implications, right? Non-quick fix would be to avoid unnecessary JSAutoRequest in nsContentUtils::ThreadJSContextStack()->Push implementation, but that would imply some non-trivial changes to JS implementation. I plan to do that for later.

(In reply to comment #4) > > But this is suboptimal as the request will be entered twice. A better way would > be to just call nsContentUtils::ThreadJSContextStack()->Push(mCx). But that may > have bad security implications, right? I meant to remove JSAutoRequest and move nsContentUtils::ThreadJSContextStack()->Push(mCx) in place of it.

But that doesn't look right. Then we don't do any "request" thing.

I wish I had a testcase for this (a testcase which run on some of my machines)

Comment on attachment 462356 [details] [diff] [review] like this? >diff --git a/content/base/src/nsInProcessTabChildGlobal.cpp b/content/base/src/nsInProcessTabChildGlobal.cpp >--- a/content/base/src/nsInProcessTabChildGlobal.cpp >+++ b/content/base/src/nsInProcessTabChildGlobal.cpp >@@ -339,28 +339,27 @@ nsInProcessTabChildGlobal::LoadFrameScri > read = 0; > } > } > nsScriptLoader::ConvertToUTF16(channel, (PRUint8*)data.get(), data.Length(), > EmptyString(), nsnull, dataString); > } > > if (!dataString.IsEmpty()) { >- JSAutoRequest ar(mCx); >+ nsContentUtils::ThreadJSContextStack()->Push(mCx); I forgot that Push does not start the request. So the proper thing would be: nsContentUtils::ThreadJSContextStack()->Push(mCx); JSAutoRequest ar(mCx);

Comment on attachment 463129 [details] [diff] [review] patch jdm, could you test whether this actually fixes the bug ;)

Comment on attachment 463129 [details] [diff] [review] patch >diff --git a/content/base/src/nsInProcessTabChildGlobal.cpp b/content/base/src/nsInProcessTabChildGlobal.cpp >--- a/content/base/src/nsInProcessTabChildGlobal.cpp >+++ b/content/base/src/nsInProcessTabChildGlobal.cpp >@@ -339,37 +339,35 @@ nsInProcessTabChildGlobal::LoadFrameScri > read = 0; > } > } > nsScriptLoader::ConvertToUTF16(channel, (PRUint8*)data.get(), data.Length(), > EmptyString(), nsnull, dataString); > } > > if (!dataString.IsEmpty()) { >+ nsContentUtils::ThreadJSContextStack()->Push(mCx); > JSAutoRequest ar(mCx); This will run JSAutoRequest destructor after the pop leading again to the request model violations. So we need an extra block and comments until at least the bug 584673 is fixed.

The crash is gone with this patch. r+!

I'll push this soon

http://hg.mozilla.org/mozilla-central/rev/51d9fb2d7e2d -- sorry for jumping the gun here Olli. I didn't see your comment until just now.

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx/1281044249/ and http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux/1281043944/ are builds that include this fix; can we verify that it doesn't crash?