865745 - Use the SafeJSContext in frame message manager

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

I'll upload a patch to try.


Bobby, could you mark which bug this one is blocking.

Comment 1

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

Attachment: wip

https://tbpl.mozilla.org/?tree=Try&rev=210dd2a5c61f

Comment 2

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

Attachment: v2

https://tbpl.mozilla.org/?tree=Try&rev=09fba50647be

Comment 3

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

Attachment: v3

GC was still causing problems, but only during shutdown.
Maybe this helps ... or leaks.
https://tbpl.mozilla.org/?tree=Try&rev=af8f1f9b7752

Comment 4

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

Attachment: v4

v3 is leaking, maybe this is better
https://tbpl.mozilla.org/?tree=Try&rev=ce0514cb0477

Comment 5

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

https://tbpl.mozilla.org/?tree=Try&rev=ce0514cb0477 looks good,
now https://tbpl.mozilla.org/?tree=Try&rev=861318fd1e2d with more testing

Comment 6

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

Comment on attachment 742127 [details] [diff] [review]
v4

Keeping things alive while calling callbacks.
The hackish part is in nsInProcessTabChildGlobal::DelayedDisconnect

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 742127 [details] [diff] [review]
v4

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

I'm sorry this took so long. I'm not really familiar with this code, and despite my efforts to brush up on it, I'm still pretty shaky on it.

In particular, I don't really understand what the nsFrameScriptCx was doing (it's certainly not documented). I still have a number of questions, but the new setup looks saner than the old one. It's your code, so I'll let you decide whether that constitutes sufficient review or not.

::: content/base/src/nsFrameMessageManager.cpp
@@ +926,5 @@
>  nsScriptCacheCleaner* nsFrameScriptExecutor::sScriptCacheCleaner = nullptr;
>  
> +nsFrameScriptExecutor::nsFrameScriptExecutor() : mCx(nullptr)
> +{
> +  nsLayoutStatics::AddRef();

Hm, I'm willing to believe that this might now be necessary, but can you explain exactly why, preferably in a comment?

::: content/base/src/nsInProcessTabChildGlobal.cpp
@@ +250,5 @@
>      nsContentUtils::ReleaseWrapper(static_cast<EventTarget*>(this), this);
>      if (mCx) {
> +      JSAutoRequest ar(mCx);
> +      JS_SetGlobalObject(mCx, nullptr);
> +      mGlobal = nullptr;

Hm, so why can't we just destroy the cx here? If someone's still going to use it, then presumably they want the default compartment object. And if they're not, then it seems like we should be able to just kill it here...

::: dom/ipc/TabChild.cpp
@@ +1413,5 @@
>          cloneData.mDataLength = buffer.nbytes();
>      }
>  
> +    nsRefPtr<TabChild> tabChild = this;
> +    nsRefPtr<TabChildGlobal> tabChildGlobal = tabChild->mTabChildGlobal;

So, is the former now necessary as a strong ref to keep ourselves from being destroyed? I could see that, maybe, but I'm having trouble groking how the latter could be necessary. Can you add a comment explaining exactly what's going on?

@@ +1954,3 @@
>      StructuredCloneData cloneData = UnpackClonedMessageDataForChild(aData);
> +    nsRefPtr<TabChild> tabChild = this;
> +    nsRefPtr<TabChildGlobal> tabChildGlobal = mTabChildGlobal;

Same.

Comment 8

[Bobby Holley (:bholley)]

Smaug and I discussed this on IRC, and decided that it makes the most sense to try to just transition into using the SafeJSContext here, since that's what we're eventually moving towards for everyone. I'm going to file a few blocking bugs.

Comment 9

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=ec616a823711

Comment 10

[Bobby Holley (:bholley)]

This is green. :-)

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 1 - Make the SafeJSContext accept JSVERSION_LATEST. v1

We'll need this once we start compiling frame scripts with it. This might seem
kind of scary, but I don't think the safe JSContext is actually used for
compilation much if at all (it's usually just used for JSAPI stuff).

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 2 - Stop depending on mCx during frame script execution. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 3 - Rename nsFrameScriptExecutor::DidCreateCx. v1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 4 - Hold a strong ref to the global for the duration that each nsFrameScriptCx is on the stack. v1

These things currently do a complicated refcounting dance to avoid destroying the
cx until all the consumers of it are gone. That stuff can mostly go away now that
we're just using the SafeJSContext, but DestroyCx also nulls out the global, so
we should make sure to keep that alive for anyone that might be using it.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 5 - Use an AutoSafeJSContext in nsFrameMessageManager::ReceiveMessage. v1

This function proceeds to push its cx and enters a compartment, so it can't be
depending on any compartment or callstack state of the cx it's using. The only
potential issue would then be reporting the error to the correct DOM window, but
this stuff is used only for chrome, where that doesn't matter. The safe JSContext
uses the same error reporter as JSMs and such, which is probably fine.

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 6 - Remove redundant cx push. v1

AutoSafeJSContext will always push.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 7 - Use a SafeJSContext in TabChildParent. v1

This is just used for rooting, but happens to be a consumer of
nsFrameMessageManager::GetJSContext, which we're about to remove.

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 8 - Remove mContext from nsFrameMessageManager. v1

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 9 - Check mGlobal instead of mCx to see if we're in a functional state. v1

Their lifetimes should be the same, and the latter is going away.

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 10 - Return the safe JSContext for Tab Children in GetJSContextForEventHandlers. v1

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 12 - Remove mCx from nsFrameScriptExecutor. v1

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 13 - Remove nsFrameScriptExecutor::DestroyCx. v1

Comment 23

[Bobby Holley (:bholley)]

Attachment: Full Diff

Attaching full diff, per smaug's request.

Comment 24

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

Comment on attachment 759232 [details] [diff] [review]
Full Diff

>--- a/js/xpconnect/src/XPCJSContextStack.cpp
>+++ b/js/xpconnect/src/XPCJSContextStack.cpp
>@@ -159,6 +159,7 @@ XPCJSContextStack::GetSafeJSContext()
>     mSafeJSContext = JS_NewContext(rt, 8192);
>     if (!mSafeJSContext)
>         return NULL;
>+    JS_SetVersion(mSafeJSContext, JSVERSION_LATEST);
> 
>     JS::RootedObject glob(mSafeJSContext);
>     {

Otherwise looking good, but this feels risky. Should be ok from message manager, but may break other stuff.
The versioning should go to compartment.
Could you please fix that first, and then land this patch without the change to XPCJSContextStack.cpp

Comment 25

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=6263d14e2460

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 759220 [details] [diff] [review]
Part 12 - Remove mCx from nsFrameScriptExecutor. v1

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

::: content/base/src/nsFrameMessageManager.cpp
@@ +1034,5 @@
>  // static
>  void
>  nsFrameScriptExecutor::Unlink(nsFrameScriptExecutor* aTmp)
>  {
> +  aTmp->mGlobal = nullptr;

This doesn't make sense. This is only called from nsInProcessTabChildGlobal's unlink, after

NS_IMPL_CYCLE_COLLECTION_UNLINK(mGlobal)

so mGlobal is always null here. Why not inline these two functions? That'll give...

NS_IMPL_CYCLE_COLLECTION_INHERITED_2(nsInProcessTabChildGlobal,
                                     nsDOMEventTargetHelper,
                                     mMessageManager,
                                     mGlobal)

Comment 27

[Bobby Holley (:bholley)]

(In reply to :Ms2ger from comment #26)
> so mGlobal is always null here. Why not inline these two functions?

I filed bug 888682 and will do this in a followup.

Comment 28

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=9a57cd3e5a93

Comment 29

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/89035681fdd1 because http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochiperf/browser_msgmgr_01.js, a perf mochitest filled with magic number timeouts, hit unexpected sessionstore exceptions, https://tbpl.mozilla.org/php/getParsedLog.php?id=24753259&tree=Mozilla-Inbound.

Comment 30

[Bobby Holley (:bholley)]

Ugh. I'd imagine that this always threw, and the JSContext changes here just cause the dice to land a bit differently as to whether we report the exception from SessionStore to onerror or not.

I've pushed some diagnostics to test this theory, which try/catch and dump whether we threw or not.

Without the JSContext changes:
https://tbpl.mozilla.org/?tree=Try&rev=d0d7d9cabe88

With the JSContext changes:
https://tbpl.mozilla.org/?tree=Try&rev=97f6732f7f45

Comment 31

[Bobby Holley (:bholley)]

Theory confirmed. Filed bug 888736 and relanding with the exception squelched:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=3f5669a7cd14

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/72f3191b2f57
https://hg.mozilla.org/mozilla-central/rev/2433717d83cd
https://hg.mozilla.org/mozilla-central/rev/c3a8e4da1cce
https://hg.mozilla.org/mozilla-central/rev/329b420eb34b
https://hg.mozilla.org/mozilla-central/rev/4b027ffa665a
https://hg.mozilla.org/mozilla-central/rev/895af605aeb8
https://hg.mozilla.org/mozilla-central/rev/0916ee0101e0
https://hg.mozilla.org/mozilla-central/rev/878459c6f483
https://hg.mozilla.org/mozilla-central/rev/a0517aaf9011
https://hg.mozilla.org/mozilla-central/rev/f68b47d85c48
https://hg.mozilla.org/mozilla-central/rev/4c6755c92b09
https://hg.mozilla.org/mozilla-central/rev/3f5669a7cd14

Comment 33

[Bobby Holley (:bholley)]

Uplifted to 24 as a dep of bug 860085:

https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=dd08f5dd367c