722594 - can we remove xpc_CreateMTGlobalObject and mMTCompartmentMap?

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Luke Wagner [:luke]]

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.

Comment 1

[Luke Wagner [:luke]]

Oops, forgot to CC those being asked.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

(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.

Comment 3

[Luke Wagner [:luke]]

Attachment: rm

Awesome, thanks for the explanation.  Try's green too.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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|?

Comment 5

[Luke Wagner [:luke]]

Attachment: rm more

Actually, the rippage can go farther and deeper.  Ah, the sweet rewards of bug 650411.

Comment 6

[Luke Wagner [:luke]]

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

I do not understand this question.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 8

[Luke Wagner [:luke]]

(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.

Comment 9

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2f1bf0353c2

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f2f1bf0353c2