Closed Bug 396487 Opened 14 years ago Closed 14 years ago

Make GCF_SYSTEM immutable per object, set at birth

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta1

People

(Reporter: brendan, Assigned: igor)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch fix (obsolete) — Splinter Review
Patch separated from bug 157334 attached.

/be
Attachment #281241 - Flags: review?(igor)
This supports sharing thing flags using gc pages.

/be
Attachment #281241 - Flags: review?(igor) → review+
Attachment #281241 - Flags: approval1.9?
Attachment #281241 - Flags: approval1.9? → approval1.9+
Sicking said a=him on the DOM part, via IRC.

/be
Status: NEW → ASSIGNED
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Reopened, failing tests from bug 393974.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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
Status: REOPENED → ASSIGNED
Attached patch fix, v2 (obsolete) — Splinter Review
Attachment #281241 - Attachment is obsolete: true
Attachment #281399 - Flags: review?(bzbarsky)
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
Attachment #281399 - Flags: review?(igor)
Seems to pass mochitests.

/be
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
Attachment #281399 - Flags: review?(bzbarsky) → review+
(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 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?
Attachment #281399 - Flags: review?(igor) → review+
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.
Attachment #281399 - Flags: review+ → review-
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.
Attachment #281459 - Flags: review?(brendan)
(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.

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
Assignee: brendan → igor
Status: ASSIGNED → NEW
(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
> Matter of taste?

I think == has much lower mind-print.
Depends on: 396758
I filed bug 396758 about storing the system bit in the object itself.
Comment on attachment 281459 [details] [diff] [review]
using the class slot to store the system flag

A new patch is comming
Attachment #281459 - Attachment is obsolete: true
Attachment #281459 - Flags: review?(brendan)
Attached patch alternative patch v1 (obsolete) — Splinter Review
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.
Attachment #281399 - Attachment is obsolete: true
Attachment #285691 - Flags: review?(bzbarsky)
Attachment #285691 - Flags: review?(brendan)
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.
Attachment #285691 - Flags: review?(bzbarsky) → review+
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
Attachment #285691 - Flags: review?(brendan)
Attachment #285691 - Flags: review+
Attachment #285691 - Flags: approval1.9?
Here is a new version that addresses the nits.
Attachment #285691 - Attachment is obsolete: true
Attachment #286041 - Flags: review+
Attachment #286041 - Flags: approval1.9?
Attachment #285691 - Flags: approval1.9?
Attachment #286041 - Flags: approval1.9? → approval1.9+
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
Status: NEW → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
No longer depends on: 403724
Depends on: 403724
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.