981218 - Remove default compartment objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

Now that bug 979481 is landed, we have a null default compartment object for both DOMWindow JSContexts and the SafeJSContext. At that point, we really don't have a use for default compartment objects. Their primary purpose was to specify the compartment that the JSContext should be restored to after JS_SaveFrameChain (which happens during pushing). But I think we can get to a point where we only ever push DOMWindowJSContexts and the SafeJSContext.

I think we can probably do the following:
(1) Remove the special JSContexts we have for the component loader, sandboxes, etc, and replace them with the SafeJSContext.
(2) Fix up any jsapi-tests and whatnot that are sloppy about entering a compartment.
(3) Remove default compartment objects, and their associated goop.

Comment 1

[Luke Wagner [:luke]]

\o/

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove the JSAutoCompartment from cx pushing. v1

By this point, AutoJSAPI takes care of entering the correct compartment, so this
no longer serves a purpose.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Stop using default compartment objects in nsJSUtils.cpp. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Stop using default compartment objects in workers. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove default compartment object usage in js shell and tests. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 5 - Stop using a default compartment object in the IndexedDB and ProxyAutoConfig JSRuntimes. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 6 - Remove default compartment objects. v1

\o/

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8476245 [details] [diff] [review]
Part 6 - Remove default compartment objects. v1

\o/ \o/ \o/

Comment 11

[Luke Wagner [:luke]]

ISTR some sort of cruft (my memory is not more specific than that) we had to add to deal with the case where you start in compartment A, auto-enter compartment B, auto-leave compartment B, but since then the default-compartment-object has been changed to C.  ExclusiveContext::leaveCompartment seems pretty reasonable nowadays, so maybe it's all gone.

Comment 12

[Bob Owen (:bobowen)]

Comment on attachment 8476240 [details] [diff] [review]
Part 1 - Remove the JSAutoCompartment from cx pushing. v1

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

Right, we only construct the AutoCxPusher in AutoJSAPI::InitInternal and we always enter a compartment directly afterwards, making this redundant.

Comment 13

[Bob Owen (:bobowen)]

Comment on attachment 8476241 [details] [diff] [review]
Part 2 - Stop using default compartment objects in nsJSUtils.cpp. v1

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

I've convinced myself that all the callers of nsJSUtils::ReportPendingException will either pass a JSContext that has a script context or pass the SafeJSContext.
Again, GetDefaultScopeFromJSContext is only called during an actual push as part of AutoJSAPI::InitInternal, so the JSContext must have a script context or be the SafeJSContext.

So in both cases either we would never call js::DefaultObjectForContextOrNull or it would return null anyway.

Comment 14

[Bob Owen (:bobowen)]

Comment on attachment 8476242 [details] [diff] [review]
Part 3 - Stop using default compartment objects in workers. v1

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

GlobalScope gets set just after the global is created in WorkerPrivate::CreateGlobalScope and (currently until part 6) set as the default compartment object for the context.

Comment 15

[Bob Owen (:bobowen)]

Comment on attachment 8476244 [details] [diff] [review]
Part 5 - Stop using a default compartment object in the IndexedDB and ProxyAutoConfig JSRuntimes. v1

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

OK, after part 4 it looks like the only place still using DefaultObjectForContextOrNull is JSRuntime::initSelfHosting.
This is only called as part of JS_NewContext, which is clearly before we're setting it here.

Comment 16

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #13)

> I've convinced myself that all the callers of
> nsJSUtils::ReportPendingException will either pass a JSContext that has a
> script context or pass the SafeJSContext.

Yep! Those are now the only 2 contexts left on the main thread. :-)

Comment 17

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/32f80f97a321
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/18ffee30e06a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0d911a34415d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fa5dfae27e49
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f4c2b44cc493
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ff8835b816c9

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/32f80f97a321
https://hg.mozilla.org/mozilla-central/rev/18ffee30e06a
https://hg.mozilla.org/mozilla-central/rev/0d911a34415d
https://hg.mozilla.org/mozilla-central/rev/fa5dfae27e49
https://hg.mozilla.org/mozilla-central/rev/f4c2b44cc493
https://hg.mozilla.org/mozilla-central/rev/ff8835b816c9