396487 - Make GCF_SYSTEM immutable per object, set at birth

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

Attachment: fix

Patch separated from bug 157334 attached.

/be

Comment 1

[Brendan Eich [:brendan]]

This supports sharing thing flags using gc pages.

/be

Comment 2

[Brendan Eich [:brendan]]

Sicking said a=him on the DOM part, via IRC.

/be

Comment 3

[Brendan Eich [:brendan]]

Fixed:

js/src/jscntxt.h 3.164
js/src/jsdbgapi.c 3.105
js/src/jsdbgapi.h 3.39
js/src/jsgc.c 3.245
js/src/jsobj.c 3.382
js/src/xpconnect/loader/mozJSComponentLoader.cpp 1.144
js/src/xpconnect/src/nsXPConnect.cpp 1.133
js/src/xpconnect/src/xpcwrappednative.cpp 1.155
js/src/xpconnect/src/xpcwrappednativeproto.cpp 1.18

/be

Comment 4

[Brendan Eich [:brendan]]

Reopened, failing tests from bug 393974.

/be

Comment 5

[Igor Bukanov]

Since changes here affects the public API in any case, why not to move the system bit into JSObject itself? JSClass* is aligned on 4 bytes, so there is one bit available besides the private one.

Comment 6

[Brendan Eich [:brendan]]

Because we don't want the system bit to vary over the life of an object, I would prefer (and I bet Jason would too, for gc-thing-flag elimination in actionmonkey) to avoid that change.

Debugging last night showed that we run on a system context when parenting an XPCWrappedNativeProto's JS object to a non-system parent. Seems wrong -- bz, jst, any insights? More in a bit when I get back to debugging.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: fix, v2

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 281399 [details] [diff] [review]
fix, v2

Alas Gecko creates new XPCWrappedNativeProto objects with non-system (content) parent objects on system (chrome) contexts. Boris and I talked about it and fixing this is hard enough that I'll file a separate bug. Here I avoid flagging window contexts as "system" and instead create chrome window objects with the system flag.

/be

Comment 9

[Brendan Eich [:brendan]]

Seems to pass mochitests.

/be

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 281399 [details] [diff] [review]
fix, v2

>Index: js/src/xpconnect/src/xpcwrappednative.cpp
>+    NS_ASSERTION(!JS_IsSystemObject(ccx, parent) ^
>+                 JS_IsSystemObject(ccx, mFlatJSObject),
>+                 "system flag mismatch");

Wouldn't the condition be more simply written as:

  JS_IsSystemObject(ccx, parent) == JS_IsSystemObject(ccx, mFlatJSObject)

?

Same elsewhere in this file.

>Index: js/src/xpconnect/src/xpcwrappednativeproto.cpp

Same nit here.

With that change, r=bzbarsky

Comment 11

[Igor Bukanov]

(In reply to comment #6)
> Because we don't want the system bit to vary over the life of an object, I
> would prefer (and I bet Jason would too, for gc-thing-flag elimination in
> actionmonkey) to avoid that change.

But the proposal to store the state of the flag in JSObject itself is orthogonal to whether it can be changed over time! And storing the flag inside JSObject would benefit ActionMonkey nicely as it means one GC flag less to worry.

Comment 12

[Igor Bukanov]

Comment on attachment 281399 [details] [diff] [review]
fix, v2

This is fine as a patch but given the inheritance of the system flag and the availability of JS_NewSystemObject, would it be possible to remove JS_FlagSystemContext alltogether?

Comment 13

[Igor Bukanov]

Comment on attachment 281399 [details] [diff] [review]
fix, v2

That was too quick r+, see below.

>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsobj.c,v
...
> JSObject *
> js_NewObject(JSContext *cx, JSClass *clasp, JSObject *proto, JSObject *parent)
> {
...
>+    /*
>+     * Require that obj has the same system flag as its parent or (if null)
>+     * its context's default system flag (see js_NewGCThing).
>+     */
>+    gcflags = GCX_OBJECT;
>+    if (parent)
>+        gcflags |= *js_GetGCThingFlags(parent) & GCF_SYSTEM;

The comment here does not match the code. If the context is system, js_NewGCThing makes the object system irrespective whether or not the parent is system. Given the pre-patch usage of JS_FlagSystemObject I suspect that comments records the intention, not the implementation.

Comment 14

[Igor Bukanov]

Attachment: using the class slot to store the system flag

This is the version of fix v2 from comment 7 that uses the unused bit from the class slot to store the system flag eliminating GCF_SYSTEM.

Please treat this as a proposal. If the idea is OK I would split the patch in 2 parts, the first one would just moves the system flag from GC flags to the object itself and the second implements the schema proposed in this bug on top of that.

Comment 15

[Jason Orendorff [:jorendorff]]

(In reply to comment #11)
> (In reply to comment #6)
> > Because we don't want the system bit to vary over the life of an object, I
> > would prefer (and I bet Jason would too, for gc-thing-flag elimination in
> > actionmonkey) to avoid that change.
> 
> But the proposal to store the state of the flag in JSObject itself is
> orthogonal to whether it can be changed over time! And storing the flag inside
> JSObject would benefit ActionMonkey nicely as it means one GC flag less to
> worry.

For what it's worth, I agree with Igor here.  This would save me a little work.

Comment 16

[Brendan Eich [:brendan]]

I'm short on time this week and really want to see this bug fixed quickly, and Igor makes a good case (I should have considered it more carefully). Since the class slot is already private-tagged, it costs nothing to clear another low bit from it, and it avoids gc-thing-flag complications now and in actionmonkey.

Igor, can you take this bug and do those two patches? I'll review quickly.

Please do get rid of JS_FlagSystemContext. The only consumer in the last patch I posted is mozJSComponentLoader.cpp, but it already calls

    rv = xpc->InitClassesWithNewWrappedGlobal(cx, backstagePass,
                                              NS_GET_IID(nsISupports),
                                              nsIXPConnect::
                                                  FLAG_SYSTEM_GLOBAL_OBJECT,
                                              getter_AddRefs(holder));

If that is sufficient for all cases to make JS XPCOM components have system global and subsidiary objects, then there is zero need for JS_FlagSystemContext.

/be

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #10)
> (From update of attachment 281399 [details] [diff] [review])
> >Index: js/src/xpconnect/src/xpcwrappednative.cpp
> >+    NS_ASSERTION(!JS_IsSystemObject(ccx, parent) ^
> >+                 JS_IsSystemObject(ccx, mFlatJSObject),
> >+                 "system flag mismatch");
> 
> Wouldn't the condition be more simply written as:
> 
>   JS_IsSystemObject(ccx, parent) == JS_IsSystemObject(ccx, mFlatJSObject)

Matter of taste? I enjoyed my hardware and boolean logic classes too much in grad school and it shows still.

/be

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Matter of taste?

I think == has much lower mind-print.

Comment 19

[Igor Bukanov]

I filed bug 396758 about storing the system bit in the object itself.

Comment 20

[Igor Bukanov]

Comment on attachment 281459 [details] [diff] [review]
using the class slot to store the system flag

A new patch is comming

Comment 21

[Igor Bukanov]

Attachment: alternative patch v1

The new patch is build on top of bug 396758. To avoid putting a particular policy on the system flag usage the patch just replaces in js/src:

-JS_FlagSystemObject(JSContext *cx, JSObject *obj)
+JS_NewSystemObject(JSContext *cx, JSClass *clasp, JSObject *proto,
+                   JSObject *parent, JSBool system)

where the new method creates the system objects when system is true. There is no inheritance of the system flag as any particular usage is delagated to the embedding. To make the intention clear in xpconnect sources I added

inline JSObject*
xpc_NewInheritSystemJSObject(JSContext *cx, JSClass *clasp, JSObject *proto,
                             JSObject *parent)
{
    return JS_NewSystemObject(cx, clasp, proto, parent,
                              JS_IsSystemObject(cx, parent));
}

The utility is used in 3 places where system objects can be created.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 285691 [details] [diff] [review]
alternative patch v1

>Index: js/src/xpconnect/loader/mozJSComponentLoader.cpp
> +#include "jsdbgapi.h"

Why is that needed?

>Index: js/src/xpconnect/src/nsXPConnect.cpp
>+                           aFlags & nsIXPConnect::FLAG_SYSTEM_GLOBAL_OBJECT);

That's not a JSBool.  Missing != 0 or some such (and some parens of course)?

>Index: js/src/xpconnect/src/xpcprivate.h
>+// either also a system object.

s/either/ /

The rest looks ok.  r=bzbarsky with the above addressed.

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 285691 [details] [diff] [review]
alternative patch v1

With bz's comments fixed, and s/NewInheritSystemSystemJSObject/NewSystemInheritingJSObject/ -- long enough that a few more chars for good grammar don't hurt ;-).

/be

Comment 24

[Igor Bukanov]

Attachment: alternative script stack accounting v2

Here is a new version that addresses the nits.

Comment 25

[Igor Bukanov]

I checked in the patch from the comment 24 to CVS trunk:

Checking in dom/src/base/nsJSEnvironment.cpp;
/cvsroot/mozilla/dom/src/base/nsJSEnvironment.cpp,v  <--  nsJSEnvironment.cpp
new revision: 1.367; previous revision: 1.366
done
Checking in js/src/jsdbgapi.c;
/cvsroot/mozilla/js/src/jsdbgapi.c,v  <--  jsdbgapi.c
new revision: 3.113; previous revision: 3.112
done
Checking in js/src/jsdbgapi.h;
/cvsroot/mozilla/js/src/jsdbgapi.h,v  <--  jsdbgapi.h
new revision: 3.41; previous revision: 3.40
done
Checking in js/src/xpconnect/src/nsXPConnect.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/nsXPConnect.cpp,v  <--  nsXPConnect.cpp
new revision: 1.143; previous revision: 1.142
done
Checking in js/src/xpconnect/src/xpcinlines.h;
/cvsroot/mozilla/js/src/xpconnect/src/xpcinlines.h,v  <--  xpcinlines.h
new revision: 1.19; previous revision: 1.18
done
Checking in js/src/xpconnect/src/xpcprivate.h;
/cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v  <--  xpcprivate.h
new revision: 1.242; previous revision: 1.241
done
Checking in js/src/xpconnect/src/xpcwrappednative.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,v  <--  xpcwrappednative.cpp
new revision: 1.163; previous revision: 1.162
done
Checking in js/src/xpconnect/src/xpcwrappednativeproto.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednativeproto.cpp,v  <--  xpcwrappednativeproto.cpp
new revision: 1.20; previous revision: 1.19
done