Last Comment Bug 720580 - InitClassesWithNewWrappedGlobal should only create one global (with JS_NewCompartmentAndGlobal)
: InitClassesWithNewWrappedGlobal should only create one global (with JS_NewCom...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on: 733606 751454
Blocks: cpg 897676
  Show dependency treegraph
 
Reported: 2012-01-23 17:06 PST by Luke Wagner [:luke]
Modified: 2013-09-18 13:17 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
github URL (placeholder) (55 bytes, text/plain)
2012-02-10 17:02 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details
Combined patch v1 (93.92 KB, patch)
2012-02-21 11:11 PST, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-01-23 17:06:31 PST
To have compartment-per-global, every use of JS_NewGlobal needs to be replaced with JS_NewCompartmentAndGlobalObject.  There are only 3 calls in the browser, two are trivial (xpc_CreateGlobalObject, xpc_CreateMTGlobalObject) and one is non-trivial: xpc_NewSystemInheritingJSObject.  This bug is to fix the non-trivial case.  IIUC, it is only used for a few non-content globals so I think this could land ahead of bug 650353.

Lasty, there is one more JS_NewGlobalObject use in xpcshell.cpp that needs to be replaced.
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-01-24 00:26:19 PST
I'm busy with new DOM binding stuff at the moment, but hopefully I can start on this in two weeks or so.
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-01-31 12:22:56 PST
(In reply to Luke Wagner [:luke] from comment #0)
> To have compartment-per-global, every use of JS_NewGlobal needs to be
> replaced with JS_NewCompartmentAndGlobalObject.  There are only 3 calls in
> the browser, two are trivial (xpc_CreateGlobalObject,
> xpc_CreateMTGlobalObject) and one is non-trivial:
> xpc_NewSystemInheritingJSObject.  This bug is to fix the non-trivial case. 

> IIUC, it is only used for a few non-content globals

Digging into it, I don't think this is true. The first few dozen times that code path is hit, it is indeed for the BackstagePass, but that's just because a lot of component loading goes on (via mozJSComponentLoader) long before we do anything with content.

A cursory reading of nsXPConnect::InitClassesWithNewWrappedGlobal would indicate that we're using xpc_CreateGlobalObject for the common path. But the following comment explains that this is contrary to reality:

> // XXX This is not pretty. We make a temporary global object and
> // init it with all the Components object junk just so we have a
> // parent with an xpc scope to use when wrapping the object that will
> // become the 'real' global.

Indeed, looking more closely, the only uses of xpc_Create{,MT}GlobalObject are sandboxes, this temp global stuff, and mSafeJSContext. Everything else goes through the newSystemInheritingJSObject path. :-(
Comment 3 Luke Wagner [:luke] 2012-01-31 14:56:15 PST
(First of all, good news: xpc_NewMTGlobal is going away: bug 722594.)

Hm, you seem to be right; and the clasps are not just BackstagePass, but ChromeWindow, Window, and ContentFrameMessageManager so, like, all global windows.  I wonder if this changed in the last 5 months, because I was fairly certain I had looked at this.  Another interesting thing is that the calls to xpc_NewGlobal and xpc_NewSystemInheritingJSObject almost seemed paired.  Also: loading a browser with a single google.com tab shows 102 system compartments and 7 user compartments (with the c-p-g patch, of course).  Thus, it seems that compartments are being created.  Perhaps xpc_NewSystemInheritingJSObject always (or almost always) creates a global in the compartment created by xpc_NewGlobal and the fix is to undo the hack by creating the (non-temporary) global up-front?
Comment 4 Luke Wagner [:luke] 2012-02-01 12:02:15 PST
After a bit more inspection, it seems that what the bug really wants is for InitClassesWithNewWrappedGlobal to create its global object directly without calling xpc_NewSystemInheritingJSObject.  That way xpc_NewSystemInheritingJSObject could assert that the given clasp was not a global class.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-02-01 14:10:30 PST
(In reply to Luke Wagner [:luke] from comment #4)
> After a bit more inspection, it seems that what the bug really wants is for
> InitClassesWithNewWrappedGlobal to create its global object directly without
> calling xpc_NewSystemInheritingJSObject.  That way
> xpc_NewSystemInheritingJSObject could assert that the given clasp was not a
> global class.

Yeah, that's my (and Blake's) assessment as well.

I got partway through some patches for this, but ran into cross-compartment assertions when trying to set the prototype. Blake and I talked about this today, and I think the solution is to create the global much earlier on, and stop using all the generic XPCScriptable machinery for everything (since we're getting rid of that anyway with the new DOM bindings).

I'll be working on this in the coming days, though I'll be a bit distracted with FOSDEM stuff.
Comment 6 Luke Wagner [:luke] 2012-02-01 14:19:32 PST
On the subject, if there is any way you can see to avoid calling that JS_SplicePrototype madness, that'd be awesome..

IIUC, this patch could land early?  (In particular, with the assert(!(clasp->flags & JSCLASS_IS_GLOBAL)) in xpc_NewSystemInheritingJSObject)?
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2012-02-01 14:22:04 PST
(In reply to Luke Wagner [:luke] from comment #6)
> On the subject, if there is any way you can see to avoid calling that
> JS_SplicePrototype madness, that'd be awesome..

Unlikely. We can't create the prototype first, because there's no global 

> 
> IIUC, this patch could land early?  (In particular, with the
> assert(!(clasp->flags & JSCLASS_IS_GLOBAL)) in
> xpc_NewSystemInheritingJSObject)?
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-02-01 14:24:12 PST
Ooops, hit enter before I meant to.

(In reply to Luke Wagner [:luke] from comment #6)
> On the subject, if there is any way you can see to avoid calling that
> JS_SplicePrototype madness, that'd be awesome..

Unlikely. We can't create the prototype first, because there's no global to parent it to. As I understand it, it's easy to create a global without a prototype, but not the reverse.

> 
> IIUC, this patch could land early?  (In particular, with the
> assert(!(clasp->flags & JSCLASS_IS_GLOBAL)) in
> xpc_NewSystemInheritingJSObject)?

Depends. I don't have a good sense of what the assumptions are before and after c-p-g and whether we need any of those new assumptions in the fix here.
Comment 9 Luke Wagner [:luke] 2012-02-01 15:04:10 PST
(In reply to Bobby Holley (:bholley) from comment #8)
> > On the subject, if there is any way you can see to avoid calling that
> > JS_SplicePrototype madness, that'd be awesome..
> 
> Unlikely. We can't create the prototype first, because there's no global to
> parent it to. As I understand it, it's easy to create a global without a
> prototype, but not the reverse.

True (and, as I say this, I know this is "for some later bug"), but perhaps this is a very reasonable thing to want to do (create your global's prototype before the global) and we could add a JSAPI giving a sane way to do it.  One idea is to allow creating a compartment before the global.  We could do this without breaking assumptions everywhere by specifying that the compartment couldn't be used until its global was created and set.

> > IIUC, this patch could land early?  (In particular, with the
> > assert(!(clasp->flags & JSCLASS_IS_GLOBAL)) in
> > xpc_NewSystemInheritingJSObject)?
> 
> Depends. I don't have a good sense of what the assumptions are before and
> after c-p-g and whether we need any of those new assumptions in the fix here.

I don't think this patch would depend on c-p-g invariants: the task is just to remove the temp-global hack.  As long the new strategy looks roughly like xpc_NewGlobalObject (that is, there is a hash table lookup that conditionally creates a new compartment), then the c-p-g patch can just remove the branch and always create a new compartment.
Comment 10 Blake Kaplan (:mrbkap) 2012-02-02 06:32:36 PST
(In reply to Luke Wagner [:luke] from comment #9)
> True (and, as I say this, I know this is "for some later bug"), but perhaps
> this is a very reasonable thing to want to do (create your global's
> prototype before the global) and we could add a JSAPI giving a sane way to

I don't really like this idea. Especially in the case of windows where we're going to change the prototype at least one or two more times, it doesn't really gain us anything for the most common case. How bad is splicing the prototype once on object creation?

> do it.  One idea is to allow creating a compartment before the global.  We
> could do this without breaking assumptions everywhere by specifying that the
> compartment couldn't be used until its global was created and set.

This complicates the lifetime of compartments quite a bit, though. What happens for the first GC? The second? What if we enter another compartment and hit an exception?
Comment 11 Luke Wagner [:luke] 2012-02-02 08:46:27 PST
(In reply to Blake Kaplan (:mrbkap) from comment #10)
> How bad is splicing the prototype once on object creation?

It used to be analogous to setting __proto__ from script, but that now is heavily deoptimized (which is bad for global objects), so we had to introduce a special set-__proto__-but-don't-be-slow-because-we-know...stuff path.  Blech.

> > do it.  One idea is to allow creating a compartment before the global.  We
> > could do this without breaking assumptions everywhere by specifying that the
> > compartment couldn't be used until its global was created and set.
> 
> This complicates the lifetime of compartments quite a bit, though. What
> happens for the first GC? The second? What if we enter another compartment
> and hit an exception?

With enough asserts (to enforce that a compartment never gets used before it gets initialized with a global) I think there would be no global increase in complexity over just making sure GC doesn't kill un-initialized compartments.

If the reseting-prototypes-during-init problem is systemic and baked into our design, then I agree it doesn't make sense to fix in just one place.  Judging by the six calls to JS_SplicePrototype in mxr, I suspect this is the case.
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-02-09 12:47:36 PST
This is going well. My github branch is here: https://github.com/bholley/mozilla-central/commits/cpg2

I've finally got the browser starting up, running, and shutting down with this stuff applied. For the most part everything goes normally, though there are few small anomalies in the console that I need to look into.

There are also a few other changes I want to make, and a hack commit that I need to write a real fix for. But I'm closing in on it. ;-)
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2012-02-09 19:36:28 PST
So one of the anomalies I ran into was that |window.Window| wasn't being defined properly. As it turned out, we were bailing early in nsDOMClassInfo::PostCreatePrototype, because we tried to get the native associated with the global and QI it to nsPIDOMWindow all at once. The native was null (because we hadn't yet created a wrapper for it), so we were just returning early from all that stuff, assuming that we weren't dealing with a window for a global.

I've fixed that, but now there's a whole host of other issues cropping up with the code in nsDOMClassInfo that assumes we're farther along in creating a window than we really are. They're all tractable problems so far, but it's back to compile-run-crash-fix for now. ;-)
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2012-02-10 12:55:54 PST
I ended up abandoning my efforts to try to make PostCreatePrototype deal with bootstrapping windows, and worked up a solution akin to RefreshPrototype instead. That stuff now works, and the browser builds and passes XPConnect tests.

I've just got a few other things to look into and then it should be ready to push to try.
Comment 15 Bobby Holley (:bholley) (busy with Stylo) 2012-02-10 14:30:55 PST
This is now in good shape. The latest patches are on my gitub branch (see comment 12), and I've pushed it to try here:

https://tbpl.mozilla.org/?tree=Try&rev=8eb4db4f5eec

In the event that it goes green, I'll flag mrbkap for review.
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2012-02-10 17:02:10 PST
Created attachment 596232 [details]
github URL (placeholder)

Looks green! Flagging mrbkap for review.
Comment 17 Blake Kaplan (:mrbkap) 2012-02-17 07:41:58 PST
- In "Stop passing isGlobal everywhere and use the nsIXPCScriptable flags instead. v1"
@@ -1093,7 +1090,7 @@ XPCWrappedNative::Init(XPCCallContext& ccx,
 
     JSClass* jsclazz = si ? si->GetJSClass() : Jsvalify(&XPC_WN_NoHelper_JSClass);
 
-    if (isGlobal) {
+    if (si && si->GetFlags().IsGlobalObject()) {
         // Resolving a global object's class can cause us to create a global's
         // JS class without the proper global flags. Notice that here and fix
         // the problem.

With these changes, this should no longer be possible. So we should be able to assert that IsGlobalObject implies XPCONNECT_GLOBAL_FLAGS.
Comment 18 Bobby Holley (:bholley) (busy with Stylo) 2012-02-21 11:11:18 PST
Created attachment 599272 [details] [diff] [review]
Combined patch v1

Attaching a combined patch, rebased to tip for luke.
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2012-02-22 19:37:47 PST
Patch updated to pass false rather than true for wantXray during global creation. It appears that this is more what we want.
Comment 20 :Ms2ger (⌚ UTC+1/+2) 2012-03-01 04:59:00 PST
*out = foo.forget().get() should be replaced with foo.forget(out); throughout
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-03-05 13:24:48 PST
Pushed this all to try one more time: https://tbpl.mozilla.org/?tree=Try&rev=6bd6af6ccd66
Comment 22 Bobby Holley (:bholley) (busy with Stylo) 2012-03-05 15:25:19 PST
Looks green. Pushed 19 changesets to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/a8115574617b (and ancestors)
Comment 23 Ed Morley [:emorley] 2012-03-06 05:08:52 PST
Was this expected?

Regression :( Dromaeo (jslib) decrease 2.43% on Win7 Mozilla-Inbound-Non-PGO
----------------------------------------------------------------------------
    Previous: avg 145.708 stddev 1.351 of 30 runs up to revision 0601f686e1b8
    New     : avg 142.165 stddev 0.597 of 5 runs since revision a8115574617b
    Change  : -3.543 (2.43% / z=2.622)
    Graph   : http://mzl.la/AoGbEm

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0601f686e1b8&tochange=a8115574617b 

https://groups.google.com/d/msg/mozilla.dev.tree-management/oPLq7LW9iQw/dDiGYhxLGu8J
Comment 24 Bobby Holley (:bholley) (busy with Stylo) 2012-03-06 10:04:48 PST
I'm spinning up a profiling build to investigate this now. I'll check if any of the dromaeo tests are disproportionately affected, and if anything jumps out at me from the profiler.

If I don't find anything though, I think we should probably ignore this:
* It's only on Win7 Non-PGO
* It's not clear from the graph that this is actually the culprit changeset 
* It's a small drop on one-platform, making it very difficult to say anything definitive about
* It's a huge change that blocks compartment-per-global (a Q1 DOM goal).

FWIW, khuey agrees with the above analysis.
Comment 26 Bobby Holley (:bholley) (busy with Stylo) 2012-03-06 18:38:18 PST
InitClassesWithNewWrappedGlobal, which is where the meat of the changes are, seems to be barely called on the dromaeo stuff (which would make sense, since dromaeo doesn't do too much with iframes AFAIK). So I say we ignore the comment 23.
Comment 27 Ed Morley [:emorley] 2012-03-07 01:49:27 PST
wfm :-)
Comment 28 David :Bienvenu 2012-03-14 08:37:58 PDT
we (Thunderbird) are hitting this assertion, !(clasp->flags & (1<<((8 + 8)+2)))", in NewSystemInheritingJSObject -  we're loading a js module from a modal dialog, and calling a method that takes an nsIRequestObserver, which in turn passes a callback to an other js component - that callback calls onStopRequest on the nsIRequestObserver after running an xhr request, and that call to onStopRequest triggers this assertion. Any hints on what we could be doing wrong?
Comment 29 Bobby Holley (:bholley) (busy with Stylo) 2012-03-14 11:34:15 PDT
(In reply to David :Bienvenu from comment #28)
> we (Thunderbird) are hitting this assertion, !(clasp->flags & (1<<((8 +
> 8)+2)))", in NewSystemInheritingJSObject -  we're loading a js module from a
> modal dialog, and calling a method that takes an nsIRequestObserver, which
> in turn passes a callback to an other js component - that callback calls
> onStopRequest on the nsIRequestObserver after running an xhr request, and
> that call to onStopRequest triggers this assertion.

Whoa, that's quite a mouthful! ;-)

Can you file a separate bug and attach a stack?
Comment 30 David :Bienvenu 2012-03-14 14:36:22 PDT
(In reply to Bobby Holley (:bholley) from comment #29)

> Whoa, that's quite a mouthful! ;-)
> 
> Can you file a separate bug and attach a stack?

turned out we had a callback function with a bad "this" pointer by the time it got called back, so the error was most likely correct. Sorry for the false alarm!

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