Closed
Bug 396487
Opened 17 years ago
Closed 17 years ago
Make GCF_SYSTEM immutable per object, set at birth
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9beta1
People
(Reporter: brendan, Assigned: igor)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
igor
:
review+
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
Patch separated from bug 157334 attached.
/be
Attachment #281241 -
Flags: review?(igor)
Reporter | ||
Comment 1•17 years ago
|
||
This supports sharing thing flags using gc pages.
/be
Assignee | ||
Updated•17 years ago
|
Attachment #281241 -
Flags: review?(igor) → review+
Reporter | ||
Updated•17 years ago
|
Attachment #281241 -
Flags: approval1.9?
Reporter | ||
Updated•17 years ago
|
Attachment #281241 -
Flags: approval1.9? → approval1.9+
Reporter | ||
Comment 2•17 years ago
|
||
Sicking said a=him on the DOM part, via IRC.
/be
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 4•17 years ago
|
||
Reopened, failing tests from bug 393974.
/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 5•17 years ago
|
||
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.
Reporter | ||
Comment 6•17 years ago
|
||
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
Reporter | ||
Comment 7•17 years ago
|
||
Attachment #281241 -
Attachment is obsolete: true
Attachment #281399 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 8•17 years ago
|
||
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)
Reporter | ||
Comment 9•17 years ago
|
||
Seems to pass mochitests.
/be
Comment 10•17 years ago
|
||
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+
Assignee | ||
Comment 11•17 years ago
|
||
(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.
Assignee | ||
Comment 12•17 years ago
|
||
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+
Assignee | ||
Comment 13•17 years ago
|
||
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-
Assignee | ||
Comment 14•17 years ago
|
||
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)
Comment 15•17 years ago
|
||
(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.
Reporter | ||
Comment 16•17 years ago
|
||
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
Reporter | ||
Comment 17•17 years ago
|
||
(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•17 years ago
|
||
> Matter of taste?
I think == has much lower mind-print.
Assignee | ||
Comment 19•17 years ago
|
||
I filed bug 396758 about storing the system bit in the object itself.
Assignee | ||
Comment 20•17 years ago
|
||
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)
Attachment #281241 -
Flags: approval1.9+
Assignee | ||
Comment 21•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Attachment #285691 -
Flags: review?(brendan)
Comment 22•17 years ago
|
||
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+
Reporter | ||
Comment 23•17 years ago
|
||
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?
Assignee | ||
Comment 24•17 years ago
|
||
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?
Updated•17 years ago
|
Attachment #286041 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 25•17 years ago
|
||
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: 17 years ago → 17 years ago
Resolution: --- → FIXED
Depends on: 403724
Updated•17 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
You need to log in
before you can comment on or make changes to this bug.
Description
•