can we remove xpc_CreateMTGlobalObject and mMTCompartmentMap?

RESOLVED FIXED in mozilla13

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
There seem to be two callers of xpc_CreateMTGlobalObject: GetSafeJSContext and InitClassesWithNewWrappedGlobal.

GetSafeJSContext says that it uses xpc_CreateMTGlobalObject because of multi-threading but I think with a single-threaded JSRuntime (and XPConnect), this can only happen on the main thread so we can use xpc_CreateGlobalObject.

InitClassesWithNewWrappedGlobal only uses xpc_CreateMTGlobalObject if !aPrincipal.  On first glance, mxr shows all callers passing a non-null aPrincipal.  Try server will tell.

Without xpc_CreateMTGlobalObject, we can kill mMTCompartmentMap.  Note that this map is quite asymmetric with mCompartmentMap; the latter seems to be used in a lot of places during cycle collection while the former does not at all, so something smells fishy.
(Assignee)

Comment 1

5 years ago
Oops, forgot to CC those being asked.
(In reply to Luke Wagner [:luke] from comment #0)
> GetSafeJSContext says that it uses xpc_CreateMTGlobalObject because of
> multi-threading but I think with a single-threaded JSRuntime (and
> XPConnect), this can only happen on the main thread so we can use
> xpc_CreateGlobalObject.

This looks correct.

> InitClassesWithNewWrappedGlobal only uses xpc_CreateMTGlobalObject if
> !aPrincipal.  On first glance, mxr shows all callers passing a non-null
> aPrincipal.  Try server will tell.

That back door was for workers, which no longer use XPConnect at all. This should also be safe.

> Without xpc_CreateMTGlobalObject, we can kill mMTCompartmentMap.  Note that
> this map is quite asymmetric with mCompartmentMap; the latter seems to be
> used in a lot of places during cycle collection while the former does not at
> all, so something smells fishy.

The cycle collector can't collect cycles that involve objects off the main thread, so any cycles that involve the MT map won't be collectible anyway.
(Assignee)

Comment 3

5 years ago
Created attachment 593110 [details] [diff] [review]
rm

Awesome, thanks for the explanation.  Try's green too.
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #593110 - Flags: review?(mrbkap)
Comment on attachment 593110 [details] [diff] [review]
rm

>--- a/js/xpconnect/src/XPCJSRuntime.cpp
>+++ b/js/xpconnect/src/XPCJSRuntime.cpp
> #ifdef DEBUG
>-        {
>-            JSCompartment *current = NULL;
>-            NS_ASSERTION(map.Get(key, &current), "no compartment?");
>-            NS_ASSERTION(current == compartment, "compartment mismatch");
>-        }
>+    JSCompartment *current = NULL;
>+    NS_ASSERTION(map.Get(key, &current), "no compartment?");
>+    NS_ASSERTION(current == compartment, "compartment mismatch");
> #endif

Could you keep the scope |current|?
(Assignee)

Comment 5

5 years ago
Created attachment 593129 [details] [diff] [review]
rm more

Actually, the rippage can go farther and deeper.  Ah, the sweet rewards of bug 650411.
Attachment #593110 - Attachment is obsolete: true
Attachment #593110 - Flags: review?(mrbkap)
Attachment #593129 - Flags: review?(mrbkap)
(Assignee)

Comment 6

5 years ago
(In reply to Ms2ger from comment #4)
> Could you keep the scope |current|?

I do not understand this question.
Comment on attachment 593129 [details] [diff] [review]
rm more

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +276,4 @@
>  #ifdef DEBUG
> +    JSCompartment *current = NULL;
> +    NS_ASSERTION(map.Get(key, &current), "no compartment?");
> +    NS_ASSERTION(current == compartment, "compartment mismatch");

I think Ms2ger was saying that you can scope |current| to the #ifdef block to prevent trying to use it outside.

::: js/xpconnect/src/XPCThreadContext.cpp
@@ +176,5 @@
>      "global_for_XPCJSContextStack_SafeJSContext",
>      XPCONNECT_GLOBAL_FLAGS,
>      JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_StrictPropertyStub,
>      JS_EnumerateStub, SafeGlobalResolve, JS_ConvertStub, SafeFinalize,
> +    NULL, NULL, NULL, NULL, NULL, NULL, TraceXPCGlobal

This seems like a good fix, but I don't understand the relationship with this bug.
Attachment #593129 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 8

5 years ago
(In reply to Blake Kaplan (:mrbkap) from comment #7)
> This seems like a good fix, but I don't understand the relationship with
> this bug.

That assert at the end of xpc_NewGlobalObject seems to require it.
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2f1bf0353c2
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/f2f1bf0353c2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.