can we remove xpc_CreateMTGlobalObject and mMTCompartmentMap?

RESOLVED FIXED in mozilla13

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
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.
Posted patch rm (obsolete) — Splinter Review
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|?
Posted patch rm moreSplinter Review
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)
(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+
(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.
https://hg.mozilla.org/mozilla-central/rev/f2f1bf0353c2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.