Last Comment Bug 722594 - can we remove xpc_CreateMTGlobalObject and mMTCompartmentMap?
: can we remove xpc_CreateMTGlobalObject and mMTCompartmentMap?
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks: cpg
  Show dependency treegraph
 
Reported: 2012-01-30 18:11 PST by Luke Wagner [:luke]
Modified: 2012-02-03 10:55 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
rm (10.83 KB, patch)
2012-01-31 08:29 PST, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
rm more (17.83 KB, patch)
2012-01-31 09:20 PST, Luke Wagner [:luke]
mrbkap: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-01-30 18:11:38 PST
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] 2012-01-30 18:13:33 PST
Oops, forgot to CC those being asked.
Comment 2 Blake Kaplan (:mrbkap) 2012-01-31 05:50:11 PST
(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] 2012-01-31 08:29:36 PST
Created attachment 593110 [details] [diff] [review]
rm

Awesome, thanks for the explanation.  Try's green too.
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-01-31 08:55:07 PST
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] 2012-01-31 09:20:27 PST
Created attachment 593129 [details] [diff] [review]
rm more

Actually, the rippage can go farther and deeper.  Ah, the sweet rewards of bug 650411.
Comment 6 Luke Wagner [:luke] 2012-01-31 10:07:13 PST
(In reply to Ms2ger from comment #4)
> Could you keep the scope |current|?

I do not understand this question.
Comment 7 Blake Kaplan (:mrbkap) 2012-02-01 06:10:12 PST
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] 2012-02-01 08:57:51 PST
(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 10 Ed Morley [:emorley] 2012-02-03 10:55:16 PST
https://hg.mozilla.org/mozilla-central/rev/f2f1bf0353c2

Note You need to log in before you can comment on or make changes to this bug.