Closed Bug 720580 Opened 13 years ago Closed 13 years ago

InitClassesWithNewWrappedGlobal should only create one global (with JS_NewCompartmentAndGlobal)

Categories

(Core :: XPConnect, defect)

Product:
Core
Component:
XPConnect
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla13

People

(Reporter: luke, Assigned: bholley)

References

Details

Attachments

(2 files)

github URL (placeholder)
55 bytes, text/plain
mrbkap
: review+
Details
Combined patch v1
93.92 KB, patch
Details | Diff | Splinter Review 
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.
I'm busy with new DOM binding stuff at the moment, but hopefully I can start on this in two weeks or so.
(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. :-(
(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?
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.
Summary: s/JS_NewGlobalObject/JS_NewCompartmentAndGlobalObject/ in xpc_NewSystemInheritingJSObject → InitClassesWithNewWrappedGlobal should only create one global (with JS_NewCompartmentAndGlobal)
(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.
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)?
(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)?
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.
(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.
(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?
(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.
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. ;-)
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. ;-)
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.
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.
Looks green! Flagging mrbkap for review.
Attachment #596232 - Flags: review?(mrbkap)
- 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.
Attaching a combined patch, rebased to tip for luke.
Patch updated to pass false rather than true for wantXray during global creation. It appears that this is more what we want.
*out = foo.forget().get() should be replaced with foo.forget(out); throughout
Attachment #596232 - Flags: review?(mrbkap) → review+
Pushed this all to try one more time: https://tbpl.mozilla.org/?tree=Try&rev=6bd6af6ccd66
Looks green. Pushed 19 changesets to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/a8115574617b (and ancestors)
Target Milestone: --- → mozilla13
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
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.
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.
wfm :-)
Depends on: 733606
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?
(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?
(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!
Depends on: 751454
Depends on: 897676
Blocks: 897676
No longer depends on: 897676
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: