Closed Bug 979481 Opened 6 years ago Closed 6 years ago

Give the SafeJSContext a null default compartment so that AutoJSContext won't silently operate in the dummy compartment

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bzbarsky, Assigned: bholley)

References

Details

Attachments

(5 files, 1 obsolete file)

This would solve the problem we have where people AutoJSContext off a runnable, and then start creating JS objects in a random compartment.  Then they hand them to a web page and stuff breaks, in ways that are non-obvious to most people.

So Bobby and I were thinking to basically put the cx in a null compartment (as JS_SaveFrameChain would, for example).  Then add an assert to ExclusiveContext::global() (instead of just having it crash with no good explanation) in the case when compartment_ is null.

And possibly make assertSameCompartment actually assert if the cx is not in a compartment, for good measure.
Surprisingly, this is green on local xpconnect tests with only a few tweaks:

https://tbpl.mozilla.org/?tree=Try&rev=5b04020ecd2f
Summary: AutoJSContext should complain loudly if it's used without entering a compartment when not reusing the on-stack context → Give the SafeJSContext a null default compartment so that AutoJSContext won't silently operate in the dummy compartment
Depends on: 979951
Assignee: nobody → bobbyholley
Comment on attachment 8386296 [details] [diff] [review]
Part 5 - Add a helpful assertion indicating that the caller probably needs a JSAutoCompartment. v1

So friendly.
Attachment #8386296 - Flags: review?(luke) → review+
Comment on attachment 8386291 [details] [diff] [review]
Part 1 - Separate the lifetime of the SafeJSContext global from that of the SafeJSContext itself. v1

Are we guaranteed that script can't hold a ref to the XPCJSContextStack?

r=me if so.  Otherwise, it seems like we can leak via a cycle through mSafeJSContextGlobal...
Attachment #8386291 - Flags: review?(bzbarsky) → review+
Comment on attachment 8386292 [details] [diff] [review]
Part 2 - Prepare the cx stack machinery for a world where a cx has no default context. v1

s/default context/default compartment/ or "default global" in the commit message?

r=me
Attachment #8386292 - Flags: review?(bzbarsky) → review+
Comment on attachment 8386294 [details] [diff] [review]
Part 3 - Don't depend on the default global for the SafeJSContext in nsJSUtils::ReportPendingException. v1

Hmm.  Can we simplify this to calling GetDefaultScopeFromJSContext() and then if that returns null asserting NS_IsMainThread() && aContext == nsContentUtils::GetSafeJSContext() and getting xpc::GetSafeJSContextGlobal()?

r=me either way.
Attachment #8386294 - Flags: review?(bzbarsky) → review+
Comment on attachment 8386295 [details] [diff] [review]
Part 4 - Make the SafeJSContext default to a null compartment, and have AutoSafeJSContext enter the compartment instead. v1

r=me
Attachment #8386295 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #9)
> Comment on attachment 8386291 [details] [diff] [review]
> Part 1 - Separate the lifetime of the SafeJSContext global from that of the
> SafeJSContext itself. v1
> 
> Are we guaranteed that script can't hold a ref to the XPCJSContextStack?

Correct. Its lifetime is bounded to that of the XPCJSRuntime.

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #11)
> Comment on attachment 8386294 [details] [diff] [review]
> Part 3 - Don't depend on the default global for the SafeJSContext in
> nsJSUtils::ReportPendingException. v1
> 
> Hmm.  Can we simplify this to calling GetDefaultScopeFromJSContext() and
> then if that returns null asserting NS_IsMainThread() && aContext ==
> nsContentUtils::GetSafeJSContext() and getting xpc::GetSafeJSContextGlobal()?

Good idea. I'll do that.
Updated patches locally. This is ready to land modulo bug 979951, which Kyle can hopefully fix.
Full try push with Kyle's patch: https://tbpl.mozilla.org/?tree=Try&rev=55d3b0ada798
Blocks: 981218
Depends on: 981202
backed out for causing https://bugzilla.mozilla.org/show_bug.cgi?id=981202#c3 from all branches
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hi, can't apply patches in bug 979481 on to m-c tip.  Need rebase.
Flags: needinfo?(bobbyholley)
Rebased part 1. The rest of them don't seem to need rebasing.
Attachment #8386291 - Attachment is obsolete: true
Attachment #8394863 - Flags: review+
Thanks for taking the time to test this, vicamo. Let me know if there's anything else you need.
Flags: needinfo?(bobbyholley) → needinfo?(vyang)
(In reply to Bobby Holley (:bholley) from comment #21)
> Thanks for taking the time to test this, vicamo. Let me know if there's
> anything else you need.

Hi, back from weekend & team building.  Maybe due to the three days delay, I can't still apply part 3/4/5 on to m-c tip without manually edit the changed files.  Could you also point out which revision are these patches based?  However, with some manual fixes, these patch along with patches from bug 981202 (with one manual fix as well) passes all MobileMessage marionette test cases.
Flags: needinfo?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #22)
> Hi, back from weekend & team building.  Maybe due to the three days delay, I
> can't still apply part 3/4/5 on to m-c tip without manually edit the changed
> files.  Could you also point out which revision are these patches based? 

Part 1 was the only patch that had merge conflicts when I rebased the series forward with git, which is why that was the only patch I re-uploaded. But I guess git is generally better at automatic merging than hg (especially given that our convention for hg patches is -U8).

> However, with some manual fixes, these patch along with patches from bug
> 981202 (with one manual fix as well) passes all MobileMessage marionette
> test cases.

Awesome! That's probably good enough. I've flagged gregor for review on the other bug, and then we can land all of these together.

Thanks for your help. :-)
Depends on: 990090
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.