583777 - Assertion [@ JS_BeginRequest] "!cx->prevRequestContext" when accessing remote tabs

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Josh Matthews [:jdm]]

Attachment: Backtrace

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

See attached stack.

Comment 1

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

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?

Comment 2

[Igor Bukanov]

(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

Comment 3

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

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

Comment 4

[Igor Bukanov]

(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.

Comment 5

[Igor Bukanov]

(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.

Comment 6

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

Attachment: like this?

Comment 7

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

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

Comment 8

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

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

Comment 9

[Igor Bukanov]

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 11

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

Attachment: patch

Comment 12

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

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

jdm, could you test whether this actually fixes the bug ;)

Comment 13

[Igor Bukanov]

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.

Comment 14

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

Attachment: patch

Comment 15

[Josh Matthews [:jdm]]

The crash is gone with this patch.  r+!

Comment 16

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

I'll push this soon

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

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?