Closed Bug 865745 Opened 11 years ago Closed 11 years ago

Use the SafeJSContext in frame message manager

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox24 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(13 files, 4 obsolete files)

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
I'll upload a patch to try.


Bobby, could you mark which bug this one is blocking.
Blocks: 860085
Attached patch v2 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=09fba50647be
Attachment #741928 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
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
Attached patch v4 (obsolete) — Splinter Review
v3 is leaking, maybe this is better
https://tbpl.mozilla.org/?tree=Try&rev=ce0514cb0477
Attachment #742086 - Attachment is obsolete: true
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
Depends on: 868122
This is green. :-)
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)
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)
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
AutoSafeJSContext will always push.
Attachment #759210 - Flags: review?(bugs)
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)
Their lifetimes should be the same, and the latter is going away.
Attachment #759216 - Flags: review?(bugs)
Attached patch Full DiffSplinter Review
Attaching full diff, per smaug's request.
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+
Attachment #759201 - Flags: review?(bugs)
Attachment #759203 - Flags: review?(bugs)
Attachment #759204 - Flags: review?(bugs)
Attachment #759206 - Flags: review?(bugs)
Attachment #759207 - Flags: review?(bugs)
Attachment #759210 - Flags: review?(bugs)
Attachment #759212 - Flags: review?(bugs)
Attachment #759215 - Flags: review?(bugs)
Attachment #759216 - Flags: review?(bugs)
Attachment #759218 - Flags: review?(bugs)
Attachment #759220 - Flags: review?(bugs)
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: