Remove default compartment objects

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla34
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

Assignee

Description

5 years ago
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.
\o/
Assignee

Updated

5 years ago
Depends on: 989069
Assignee

Updated

5 years ago
Depends on: 1042996
Assignee

Updated

5 years ago
No longer depends on: 989069
Duplicate of this bug: 989069
Assignee

Comment 4

5 years ago
By this point, AutoJSAPI takes care of entering the correct compartment, so this
no longer serves a purpose.
Attachment #8476240 - Flags: review?(bobowencode)

Updated

5 years ago
Attachment #8476243 - Flags: review?(luke) → review+
Comment on attachment 8476245 [details] [diff] [review]
Part 6 - Remove default compartment objects. v1

\o/ \o/ \o/
Attachment #8476245 - Flags: review?(luke) → review+
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.
Assignee

Updated

5 years ago
Depends on: 1056418
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.
Attachment #8476240 - Flags: review?(bobowencode) → review+
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.
Attachment #8476241 - Flags: review?(bobowencode) → review+
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.
Attachment #8476242 - Flags: review?(bobowencode) → review+
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.
Attachment #8476244 - Flags: review?(bobowencode) → review+
Assignee

Comment 16

5 years ago
(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. :-)

Updated

5 years ago
Duplicate of this bug: 604813
Duplicate of this bug: 567361
You need to log in before you can comment on or make changes to this bug.