408301 - XPConnect wrappers w/o XPConnect proto don't share JS scopes with their protos.

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Johnny Stenback (:jst)]

This came out of bug 408143. There, we made all XPConnect wrappers that have a prototype (i.e. they're wrappers for natives with classinfo) share their JS scope with their prototype. But wrappers for natives that have no classinfo get not XPConnect prototype, and thus there's no proto that the scope can be shared with. This should be fairly easily fixable by introducing a prototype object per scope that the wrappers could share their JS object scope with. All we'd need is a prototype that has the same object ops and magic class bits (see js_NewObject() where it decides whether to share scopes (maps) or create a new one)...

Comment 1

[Johnny Stenback (:jst)]

Attachment: Enable more scope sharing.

This lets instances of XPC_WN_NoHelper_JSClass share scopes (maps) as well. There's two types of XPC_WN_NoHelper_JSClass instances, ones that have no prototype at all, and ones that use XPC_WN_NoMods_NoCall_Proto_JSClass. For the first case, I introduced a new JSClass to use as their prototype, XPC_WN_NoHelper_Proto_JSClass. This class basically does nothing, except have the right bits set in its flags (reserved slots through JSCLASS_HAS_PRIVATE), and it exposes the right JSObjectOps.

The large hunk (first hunk) in XPCWrappedNativeScope::SetGlobal() is only a removal of an unnecessary null check, the GetScopeOfObject() change is a fix for an assert that's not really related, and the rest are real changes.

This patch cuts the numer of scopes created in the JS engine when starting up and then shutting down right away from ~2300 to ~1400 scopes.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 293638 [details] [diff] [review]
Enable more scope sharing.

>@@ -1320,6 +1329,7 @@ private:
>     // constructor).
>     JSObject*                        mGlobalJSObject;
>     JSObject*                        mPrototypeJSObject;
>+    JSObject*                        mNoHelperJSPrototype;

Do we still want mPrototypeJSObject? Is that causing unwanted js_NewScope from js_NewObject action?

>@@ -191,25 +228,24 @@ XPCWrappedNativeScope::SetGlobal(XPCCallContext& ccx, JSObject* aGlobal)
> #ifndef XPCONNECT_STANDALONE
>     mScriptObjectPrincipal = nsnull;
>     // Now init our script object principal, if the new global has one
>-    if (aGlobal)
>+
>+    JSContext* cx = ccx.GetJSContext();
>+    const JSClass* jsClass = JS_GetClass(cx, aGlobal);
>+    if(jsClass && !(~jsClass->flags & (JSCLASS_HAS_PRIVATE |
>+                                       JSCLASS_PRIVATE_IS_NSISUPPORTS)))

No need for jsClass non-null test. Note !(~foo & (BAR|BAZ)) goodness for later:

>@@ -637,10 +701,11 @@ GetScopeOfObject(JSContext* cx, JSObject* obj)
> #ifdef DEBUG
>         {
>             if(clazz->flags & JSCLASS_HAS_PRIVATE &&
>-               clazz->flags & JSCLASS_PRIVATE_IS_NSISUPPORTS)
>+               clazz->flags & JSCLASS_PRIVATE_IS_NSISUPPORTS &&
>+               (supports = (nsISupports*) JS_GetPrivate(cx, obj)))

Use that bit-magic here too.

Still wondering about other cases where XPConnect uses a prototype with different ops and/or reserved slots and forces eager new scope creation.

/be

Comment 3

[Eivind Eklund]

> Use that bit-magic here too.

Please don't.  I am normally reasonably good at bit magic, and I still had to spend about a minute and a half to be sure what that magic did. It's better to keep the code readable and leave the magic to the compiler.

Eivind.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> > Use that bit-magic here too.
> 
> Please don't.  I am normally reasonably good at bit magic, and I still had to
> spend about a minute and a half to be sure what that magic did. It's better to
> keep the code readable and leave the magic to the compiler.

That's an idiom we use already. It's not going away, and now that you've learned it, we are all better off. Idioms (linguistically this is more of a cliché) are like that.

/be

Comment 5

[Johnny Stenback (:jst)]

(In reply to comment #2)
> (From update of attachment 293638 [details] [diff] [review])
> >@@ -1320,6 +1329,7 @@ private:
> >     // constructor).
> >     JSObject*                        mGlobalJSObject;
> >     JSObject*                        mPrototypeJSObject;
> >+    JSObject*                        mNoHelperJSPrototype;
> 
> Do we still want mPrototypeJSObject? Is that causing unwanted js_NewScope from
> js_NewObject action?

mPrototypeJSObject is simply Object.prototype, and that's used as the proto for other XPConnect prototypes and doesn't hurt noticeably here.

[...]

> Still wondering about other cases where XPConnect uses a prototype with
> different ops and/or reserved slots and forces eager new scope creation.

With this patch we're down to sharing scopes for all wrapped natives AFAICT. The only other cases I see where we don't share scopes is in XPCNativeWrapper n' friends, and also in the actual prototypes, of which there's very few per scope so I don't think that's worth fixing.

Comment 6

[Johnny Stenback (:jst)]

Attachment: Updated diff

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 294764 [details] [diff] [review]
Updated diff

>@@ -1226,6 +1230,11 @@ public:
>     JSObject*
>     GetPrototypeJSObject() const {return mPrototypeJSObject;}
> 
>+    // Getter for the prototype that we use for wrappers that have no
>+    // helper.
>+    JSObject*
>+    GetNoHelperJSPrototype(XPCCallContext& ccx);

[snip 1]

>@@ -1320,6 +1329,7 @@ private:
>     // constructor).
>     JSObject*                        mGlobalJSObject;
>     JSObject*                        mPrototypeJSObject;
>+    JSObject*                        mNoHelperJSPrototype;
>     JSObject*                        mPrototypeJSFunction;

Wonder if adding the new member after mPrototypeJSFunction is better, just cuz the first three are global, Object.prototype, and Function.prototype, then we get into extension (to ECMA) territory? Also, could comment the mPrototypeJS* ones with their in-language property-path names, e.g. Object.prototype).

[snip 2]

>+++ b/js/src/xpconnect/src/xpcwrappednative.cpp
>@@ -874,7 +874,11 @@ XPCWrappedNative::Init(XPCCallContext& ccx, JSObject* parent, JSBool isGlobal,
> 
>     JSObject* protoJSObject = HasProto() ?
>                                 GetProto()->GetJSProtoObject() :
>-                                GetScope()->GetPrototypeJSObject();
>+                                GetScope()->GetNoHelperJSPrototype(ccx);

Ok, I see why I was confused -- the naming convention was mPrototypeJSFoo and GetPrototypeJSFoo, but you've changed it for the NoHelper class variant to mFooJSPrototype and GetFooJSPrototype. Fix that to match and maybe comment a bit, and sr=me.

/be

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 294764 [details] [diff] [review]
Updated diff

>diff --git a/js/src/xpconnect/src/xpcwrappednativescope.cpp b/js/src/xpconnect/src/xpcwrappednativescope.cpp
>index e9938a5..6d35a9d 100644
>--- a/js/src/xpconnect/src/xpcwrappednativescope.cpp
>+++ b/js/src/xpconnect/src/xpcwrappednativescope.cpp

>@@ -119,11 +119,12 @@ XPCWrappedNativeScope::GetNewOrUsed(XPCCallContext& ccx, 

>+        // We need to call SetGlobal in order to refresh our cached
>+        // mPrototypeJSObject and mPrototypeJSFunction and to clear
>+        // mNoHelperJSPrototype (so we get a new on if requested in

... a new one ...

>@@ -191,25 +228,24 @@ XPCWrappedNativeScope::SetGlobal(XPCCallContext& ccx, JSObject* 

>+    const JSClass* jsClass = JS_GetClass(cx, aGlobal);
>+    if(!(~jsClass->flags & (JSCLASS_HAS_PRIVATE |
>+                            JSCLASS_PRIVATE_IS_NSISUPPORTS)))
>     {
>-        JSContext* cx = ccx.GetJSContext();
>-        const JSClass* jsClass = JS_GetClass(cx, aGlobal);
>-        if (jsClass && !(~jsClass->flags & (JSCLASS_HAS_PRIVATE |
>-                                            JSCLASS_PRIVATE_IS_NSISUPPORTS)))

Is removing that null-check for jsClass really safe?

>+XPCWrappedNativeScope::GetNoHelperJSPrototype(XPCCallContext& ccx)
>+{
>+    // We could create this prototype in SetGlobal(), but all scopes
>+    // don't need one, so we save ourselves a bit of space if we
>+    // create these are they're needed.

... these when they're needed.

Comment 9

[Johnny Stenback (:jst)]

(In reply to comment #8)
> >+    const JSClass* jsClass = JS_GetClass(cx, aGlobal);
> >+    if(!(~jsClass->flags & (JSCLASS_HAS_PRIVATE |
> >+                            JSCLASS_PRIVATE_IS_NSISUPPORTS)))
> >     {
> >-        JSContext* cx = ccx.GetJSContext();
> >-        const JSClass* jsClass = JS_GetClass(cx, aGlobal);
> >-        if (jsClass && !(~jsClass->flags & (JSCLASS_HAS_PRIVATE |
> >-                                            JSCLASS_PRIVATE_IS_NSISUPPORTS)))
> 
> Is removing that null-check for jsClass really safe?

Yes, a JSObject w/o a JSClass is just broken and should never come out of the JS engine. I checked this with brendan too, and he confirmed. Caps and other code relies on this too.

Comment 10

[Johnny Stenback (:jst)]

Attachment: Updated fix for checkin.

This fixes the issues pointed out by brendan and peterv. Thanks for the reviews!

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I backed this out to see if it will fix 3 mochitests on winxp-01 that started failing with this as the only checkin in the window.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backing out this patch seems to have fixed that problem.

Comment 13

[Johnny Stenback (:jst)]

It seems unclear whether this patch really did have anything to do with the problem, and after running through these tests on two different computers I've still not seen the same failure. After talking with sayrer we decided to try to land this again to see if this really did have anything to do with the orange or not. I'll probably do that either later today or tomorrow when I have time to watch the tree...

Comment 14

[Ryan VanderMeulen [:RyanVM]]

This fix broke Adblock Plus with the following error:
Error: abpPrefs is undefined
Source File: chrome://adblockplus/content/overlay.js
Line: 143

It also called the Google Calendar at the top of the Fx Tinderbox to not work with this error:
Error: uncaught exception: Permission denied to call method Location.toString

Local backout confirmed that all the problems went away.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Oh, this also seems to have caused some major red & orange on the unit test boxes as well.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Johnny asked me to back him out if this caused problems, and since it appears that it has, I've done so.

Checking in js/src/xpconnect/src/xpcwrappednativescope.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp,v  <--  xpcwrappednativescope.cpp
new revision: 1.68; previous revision: 1.67
done
Checking in js/src/xpconnect/src/xpcwrappednativejsops.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,v  <--  xpcwrappednativejsops.cpp
new revision: 1.82; previous revision: 1.81
done
Checking in js/src/xpconnect/src/xpcwrappednative.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,v  <--  xpcwrappednative.cpp
new revision: 1.181; previous revision: 1.180
done
Checking in js/src/xpconnect/src/xpcprivate.h;
/cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v  <--  xpcprivate.h
new revision: 1.265; previous revision: 1.264
done

Comment 17

[Ryan VanderMeulen [:RyanVM]]

It appears the problems in comment #14 and comment #15 were due to a buggy jsnum.c checkin (3.97). The fixed one (3.98) caused all the problems I was seeing to go away.

Sorry for the bad info and subsequent bugspam.

Comment 18

[jag (Peter Annema)]

So ... this can be backed in then? :-)

Comment 19

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #18)
> So ... this can be backed in then? :-)

Yes, and I'm happy to check it back in, but I think it'd make the most sense for jst to do that.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Just make sure it's at a quiet time on the tree so there are no more false positives :)

Comment 21

[Johnny Stenback (:jst)]

Yes, I will re-land this once the tree clears up...

Comment 22

[Johnny Stenback (:jst)]

Attachment: Unbitrotted patch.

Same as above but applies cleanly to current trunk.

Comment 23

[Johnny Stenback (:jst)]

I checked this in earlier today and it looks like it stuck this time. Marking bug FIXED.