Closed
Bug 332648
Opened 18 years ago
Closed 12 years ago
Expose js::AutoGCRooter and its subclasses via a public header
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: bzbarsky, Assigned: Ms2ger)
References
Details
Attachments
(8 files)
10.64 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
26.21 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
8.12 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
2.88 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
6.88 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
3.76 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
13.57 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
4.00 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
See bug 328007 comment 2. What approach should we be using when we have a JSContext*? I believe pretty much all consumers of nsAutoGCRoot have one, so if there's something better we could be doing, we should do it.
Comment 1•18 years ago
|
||
It's private or at best "friend" API, but you could include "jscntxt.h" and use JSTempValueRooter, JS_PUSH_SINGLE_TEMP_ROOT, and JS_POP_TEMP_ROOT. We are all friends here, eh? /be
Comment 2•18 years ago
|
||
If an array of jsvals needs protecting, use JS_PUSH_TEMP_VALUE_ROOT. /be
Reporter | ||
Comment 3•18 years ago
|
||
Actually, I'd appreciate either docs on this stuff or someone else doing the patch; I can't easily make sense of the API in jscntxt.h...
Updated•15 years ago
|
Assignee: general → nobody
QA Contact: ian → general
Comment 4•14 years ago
|
||
There's still a FIXME comment in jscntxt.h pointing here. Changing the summary to reflect where we are now. This is still wanted. Really the main motivation is not to make to make js::AutoGCRooter and so forth public but to make jscntxt.h private again. Anyone writing serious JSAPI code is basically forced to include it; these classes are just too useful to ignore. One way to fix this is: namespace js { struct ExposedContextFields { volatile jsint operationCallbackFlag; AutoGCRooter *autoGCRooters; }; class AutoGCRooter { public: AutoGCRooter(JSContext *cx, ptrdiff_t tag) : context((ExposedContextFields *) cx), down(context->autoGCRooters), tag(tag) { context->autoGCRooters = this; } ... }; } /* namespace js */ Then in jscntxt.h we arrange for JSContext's layout to be just so. (I don't think it can just be a derived class, because of JSD, but I might be wrong.) I wonder if jwalden would be willing to do the honors.
Summary: Do better GC safety in nsAutoGCRoot if we have a JSContext* → Expose js::AutoGCRooter and its subclasses via a public header
Comment 5•14 years ago
|
||
Hmm. Actually, if we're as close on bug 516832 as it looks, maybe we can WONTFIX this soon.
Comment 6•14 years ago
|
||
(In reply to comment #5) > Hmm. Actually, if we're as close on bug 516832 as it looks, maybe we can > WONTFIX this soon. We still need temporary rooting for values and value vectors not on the thread stack, even if that bug comes in quickly. Will it? I can't tell. But we'll still need fast RAII helpers for some hard cases, so why not fix this bug too/first? /be
Comment 7•14 years ago
|
||
(In reply to comment #5) > Hmm. Actually, if we're as close on bug 516832 as it looks, maybe we can > WONTFIX this soon. We may need some time to have the conservative stack scanner working only in an assertion mode to verify a scope of potential leaks. Still that should be done quickly so I agree with WONTFIX here. We may expose some extra classes to do the rooting of value arrays etc., but that do not need inline methods.
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → general
Component: DOM → JavaScript Engine
OS: Linux → All
QA Contact: general → general
Hardware: x86 → All
Assignee | ||
Updated•12 years ago
|
Assignee: general → Ms2ger
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #585707 -
Flags: review?(evilpies)
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #585708 -
Flags: review?(evilpies)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #585709 -
Flags: review?(evilpies)
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #585710 -
Flags: review?(evilpies)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #585711 -
Flags: review?(evilpies)
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #585712 -
Flags: review?(evilpies)
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #585713 -
Flags: review?(evilpies)
Assignee | ||
Comment 15•12 years ago
|
||
Attachment #585714 -
Flags: review?(evilpies)
Comment 16•12 years ago
|
||
(In reply to Ms2ger from comment #8) > Created attachment 585707 [details] [diff] [review] > Part a: Move AutoGCRooter to jsapi.h. Is the reason for this is the exact GC work that is going on? If so, why not to base the rooters on Handle and other classes?
Updated•12 years ago
|
Attachment #585708 -
Flags: review?(evilpies) → review+
Updated•12 years ago
|
Attachment #585709 -
Flags: review?(evilpies) → review+
Comment 17•12 years ago
|
||
Comment on attachment 585710 [details] [diff] [review] Part d: Move AutoStringRooter to jsapi.h. Review of attachment 585710 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jscntxt.h @@ -1602,5 @@ > typedef RootedVar<JSAtom*> RootedVarAtom; > typedef RootedVar<jsid> RootedVarId; > typedef RootedVar<Value> RootedVarValue; > > /* FIXME(bug 332648): Move this into a public header. */ Remove?
Attachment #585710 -
Flags: review?(evilpies) → review+
Updated•12 years ago
|
Attachment #585711 -
Flags: review?(evilpies) → review+
Comment 18•12 years ago
|
||
Comment on attachment 585712 [details] [diff] [review] Part f: Move AutoIdRooter to jsapi.h. Review of attachment 585712 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsgc.cpp @@ +1995,5 @@ > MarkRoot(trc, obj, "js::AutoObjectRooter.obj"); > return; > > case ID: > + MarkRoot(trc, static_cast<AutoIdRooter *>(this)->id_, "js::AutoIdRooter.id_"); Nit: JS::AutoIdRooter.id_ ?
Attachment #585712 -
Flags: review?(evilpies) → review+
Comment 19•12 years ago
|
||
Comment on attachment 585713 [details] [diff] [review] Part g: Move AutoIdArray to jsapi.h. Review of attachment 585713 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsHTMLCanvasElement.cpp @@ +517,5 @@ > return NS_ERROR_FAILURE; > } > > if (JSVAL_IS_BOOLEAN(propval)) { > + contextProps->SetPropertyAsBool(pstr, JSVAL_TO_BOOLEAN(propval)); Thanks for these changes in this file :O
Attachment #585713 -
Flags: review?(evilpies) → review+
Comment 20•12 years ago
|
||
Comment on attachment 585714 [details] [diff] [review] Part h: Move AutoEnumStateArray to jsapi.h. Review of attachment 585714 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jscntxt.cpp @@ +1742,5 @@ > JS_ASSERT(ok); > } > } > > +} /* namespace JS */ I am not sure, but why isn't this is jsapi.cpp ?
Attachment #585714 -
Flags: review?(evilpies) → review+
Comment 21•12 years ago
|
||
Comment on attachment 585707 [details] [diff] [review] Part a: Move AutoGCRooter to jsapi.h. Review of attachment 585707 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.cpp @@ +6594,5 @@ > +SetAutoGCRooter(JSContext *cx, AutoGCRooter *rooter) > +{ > + cx->autoGCRooters = rooter; > +} > + As on IRC, we shouldn't expose them.
Attachment #585707 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 22•12 years ago
|
||
(In reply to Igor Bukanov from comment #16) > (In reply to Ms2ger from comment #8) > > Created attachment 585707 [details] [diff] [review] > > Part a: Move AutoGCRooter to jsapi.h. > > Is the reason for this is the exact GC work that is going on? If so, why not > to base the rooters on Handle and other classes? The reason is that Gecko uses these classes, and we don't want Gecko to include jscntxt.h. If there are better APIs Gecko is supposed to use, I'd be happy to see patches to that effect, but I don't know enough about what this code does to do that myself and still sleep well at night.
Comment 23•12 years ago
|
||
(In reply to Ms2ger from comment #22) > (In reply to Igor Bukanov from comment #16) > > (In reply to Ms2ger from comment #8) > > > Created attachment 585707 [details] [diff] [review] > > > Part a: Move AutoGCRooter to jsapi.h. > > > > Is the reason for this is the exact GC work that is going on? If so, why not > > to base the rooters on Handle and other classes? > > The reason is that Gecko uses these classes, and we don't want Gecko to > include jscntxt.h. If there are better APIs Gecko is supposed to use, I'd be > happy to see patches to that effect, but I don't know enough about what this > code does to do that myself and still sleep well at night. If this is only for Gecko, move the classes to jsfriendapi.h, not jsapi.h. Also lets not add those GetAutoGCRooter and SetAutoGCRooter as they introduce a couple of calls per each rotter instance. Instead just add a class like JSBaseContext that contains the fields that are necessary to implement the rooting and then subclass JSContext from it. This way the rooters can just cast the passed JSContext instance to JSBaseContext and access the rooter stack.
Assignee | ||
Comment 24•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4c85015dc460 https://hg.mozilla.org/mozilla-central/rev/6407283df40b https://hg.mozilla.org/mozilla-central/rev/422290830373 https://hg.mozilla.org/mozilla-central/rev/13f996c0d905 https://hg.mozilla.org/mozilla-central/rev/af8168703a40 https://hg.mozilla.org/mozilla-central/rev/8fd1b8345680 https://hg.mozilla.org/mozilla-central/rev/2140f9cb5d6a https://hg.mozilla.org/mozilla-central/rev/4fc86339a424
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in
before you can comment on or make changes to this bug.
Description
•