Use the SafeJSContext in frame message manager

RESOLVED FIXED in Firefox 24

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 1 bug)

unspecified
mozilla25
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 fixed)

Details

Attachments

(13 attachments, 4 obsolete attachments)

1.04 KB, patch
Details | Diff | Splinter Review
3.54 KB, patch
Details | Diff | Splinter Review
2.67 KB, patch
Details | Diff | Splinter Review
4.03 KB, patch
Details | Diff | Splinter Review
6.18 KB, patch
Details | Diff | Splinter Review
1.03 KB, patch
Details | Diff | Splinter Review
1.23 KB, patch
Details | Diff | Splinter Review
13.66 KB, patch
Details | Diff | Splinter Review
4.67 KB, patch
Details | Diff | Splinter Review
1.93 KB, patch
Details | Diff | Splinter Review
13.42 KB, patch
Details | Diff | Splinter Review
3.38 KB, patch
Details | Diff | Splinter Review
28.07 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
I'll upload a patch to try.


Bobby, could you mark which bug this one is blocking.
Blocks: 860085
(Assignee)

Comment 3

5 years ago
Created attachment 742086 [details] [diff] [review]
v3

GC was still causing problems, but only during shutdown.
Maybe this helps ... or leaks.
https://tbpl.mozilla.org/?tree=Try&rev=af8f1f9b7752
Attachment #742024 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 742127 [details] [diff] [review]
v4

v3 is leaking, maybe this is better
https://tbpl.mozilla.org/?tree=Try&rev=ce0514cb0477
Attachment #742086 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Comment on attachment 742127 [details] [diff] [review]
v4

Keeping things alive while calling callbacks.
The hackish part is in nsInProcessTabChildGlobal::DelayedDisconnect
Attachment #742127 - Flags: review?(bobbyholley+bmo)
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.
Attachment #742127 - Flags: review?(bobbyholley+bmo) → review+
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.
Summary: Try to not use nsIXPConnect::ReleaseJSContext in frame message manager → Use the SafeJSContext in frame message manager
Depends on: 868110
(Assignee)

Updated

5 years ago
Depends on: 868122
This is green. :-)
Created attachment 759201 [details] [diff] [review]
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).
Attachment #759201 - Flags: review?(bugs)
Created attachment 759203 [details] [diff] [review]
Part 2 - Stop depending on mCx during frame script execution. v1
Attachment #759203 - Flags: review?(bugs)
Created attachment 759204 [details] [diff] [review]
Part 3 - Rename nsFrameScriptExecutor::DidCreateCx. v1
Attachment #759204 - Flags: review?(bugs)
Created attachment 759206 [details] [diff] [review]
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.
Attachment #759206 - Flags: review?(bugs)
Created attachment 759207 [details] [diff] [review]
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.
Attachment #759207 - Flags: review?(bugs)
Attachment #742127 - Attachment is obsolete: true
Created attachment 759210 [details] [diff] [review]
Part 6 - Remove redundant cx push. v1

AutoSafeJSContext will always push.
Attachment #759210 - Flags: review?(bugs)
Created attachment 759212 [details] [diff] [review]
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.
Attachment #759212 - Flags: review?(bugs)
Created attachment 759215 [details] [diff] [review]
Part 8 - Remove mContext from nsFrameMessageManager. v1
Attachment #759215 - Flags: review?(bugs)
Created attachment 759216 [details] [diff] [review]
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.
Attachment #759216 - Flags: review?(bugs)
Created attachment 759218 [details] [diff] [review]
Part 10 - Return the safe JSContext for Tab Children in GetJSContextForEventHandlers. v1
Attachment #759218 - Flags: review?(bugs)
Created attachment 759220 [details] [diff] [review]
Part 12 - Remove mCx from nsFrameScriptExecutor. v1
Attachment #759220 - Flags: review?(bugs)
Created attachment 759222 [details] [diff] [review]
Part 13 - Remove nsFrameScriptExecutor::DestroyCx. v1
Attachment #759222 - Flags: review?(bugs)
Created attachment 759232 [details] [diff] [review]
Full Diff

Attaching full diff, per smaug's request.
(Assignee)

Comment 24

5 years ago
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
Attachment #759232 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #759201 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #759203 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #759204 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #759206 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #759207 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #759210 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #759212 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #759215 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #759216 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #759218 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #759220 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #759222 - Flags: review?(bugs)
Depends on: 880917
Depends on: 882106
Blocks: 882106
No longer depends on: 882106
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)
Blocks: 888682
(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.
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
Theory confirmed. Filed bug 888736 and relanding with the exception squelched:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=3f5669a7cd14
You need to log in before you can comment on or make changes to this bug.