Closed Bug 838014 Opened 12 years ago Closed 12 years ago

Rooting in jsapi-tests/

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(5 files, 2 obsolete files)

Another bug for various rooting resulting from scanning through the static analysis output.
Attachment #710028 - Flags: review?(terrence)
Attachment #710029 - Flags: review?(terrence)
Attached patch Rooting the JS debugger (obsolete) — Splinter Review
This is mostly just rooting changes (triggered by rooting hazards detected by the static analysis, but spreading out a little.) I'm asking for review from jorendorff rather than terrence because I mucked with the debugger signature a bit in a likely-to-be-objectionable way. Specifically, it appeared to me to be statically known whether you want to parse the resumption value or not, and that made rooting a little easier. But I may not be right about that.
Attachment #710030 - Flags: review?(jorendorff)
Attached patch Rooting in ionmonkey (obsolete) — Splinter Review
Attachment #710031 - Flags: review?(terrence)
Comment on attachment 710028 [details] [diff] [review] Rooting in jsapi-tests/ Review of attachment 710028 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this one looked tedious.
Attachment #710028 - Flags: review?(terrence) → review+
Comment on attachment 710029 [details] [diff] [review] Rooting in the JS shell Review of attachment 710029 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/shell/js.cpp @@ +4320,4 @@ > return true; > } > > + val = new Value; Wow, I didn't know we were allowed to use |new|. As great as allocators are as a concept I'd really like to limit us to < 4 in a single program. Please replace this with js_new<Value>, unless I'm missing some reason why we cannot do that here.
Attachment #710029 - Flags: review?(terrence) → review+
Comment on attachment 710031 [details] [diff] [review] Rooting in ionmonkey Review of attachment 710031 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonBuilder.h @@ +190,5 @@ > > JSFunction *getSingleCallTarget(types::StackTypeSet *calleeTypes); > bool getPolyCallTargets(types::StackTypeSet *calleeTypes, > AutoObjectVector &targets, uint32_t maxTargets); > + bool canInlineTarget(HandleFunction target); IonBuilder should be in a SuppressNoGC zone now. ::: js/src/ion/TypeOracle.h @@ +154,5 @@ > } > virtual bool canInlineCall(HandleScript caller, jsbytecode *pc) { > return false; > } > + virtual bool canEnterInlinedFunction(HandleScript caller, jsbytecode *pc, HandleFunction callee) { Likewise for the TypeOracle.
Attachment #710031 - Flags: review?(terrence)
Ok, I think I have this fixed up now.
Attachment #710968 - Flags: review?(terrence)
Attachment #710031 - Attachment is obsolete: true
Comment on attachment 710968 [details] [diff] [review] Rooting in ionmonkey Review of attachment 710968 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.
Attachment #710968 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #6) > ::: js/src/shell/js.cpp > @@ +4320,4 @@ > > return true; > > } > > > > + val = new Value; > > Wow, I didn't know we were allowed to use |new|. As great as allocators are > as a concept I'd really like to limit us to < 4 in a single program. Please > replace this with js_new<Value>, unless I'm missing some reason why we > cannot do that here. :-) It's the shell, it can use anything! I just mindlessly did s/jsval/Value/ on it, but I've changed it to cx->new_<Value>() for ya.
Attachment #710028 - Flags: checkin+
Attachment #710029 - Flags: checkin+
Attachment #710968 - Flags: checkin+
Whiteboard: [leave open]
Attachment #710030 - Flags: checkin+
Comment on attachment 710030 [details] [diff] [review] Rooting the JS debugger is not
Attachment #710030 - Flags: checkin+
Comment on attachment 710030 [details] [diff] [review] Rooting the JS debugger Review of attachment 710030 [details] [diff] [review]: ----------------------------------------------------------------- I agree, whether vp is NULL or not is statically known per call site. This change should be fine. ::: js/src/vm/Debugger.cpp @@ +748,5 @@ > > JSTrapStatus > +Debugger::handleUncaughtExceptionHelper(Maybe<AutoCompartment> &ac, > + MutableHandleValue *vp, > + bool parseResumption, bool callHook) OK, but the parseResumption argument is redundant with vp, right? It's false exactly when vp is null?
Attachment #710030 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #14) > Comment on attachment 710030 [details] [diff] [review] > Rooting the JS debugger > > Review of attachment 710030 [details] [diff] [review]: > ----------------------------------------------------------------- > > I agree, whether vp is NULL or not is statically known per call site. This > change should be fine. > > ::: js/src/vm/Debugger.cpp > @@ +748,5 @@ > > > > JSTrapStatus > > +Debugger::handleUncaughtExceptionHelper(Maybe<AutoCompartment> &ac, > > + MutableHandleValue *vp, > > + bool parseResumption, bool callHook) > > OK, but the parseResumption argument is redundant with vp, right? It's false > exactly when vp is null? Er, yes, and that is completely obvious, so why would I go to the trouble of adding a boolean when the original code was already doing everything just fine? Argh. Thanks!
Attachment #710029 - Flags: checkin+
Comment on attachment 710030 [details] [diff] [review] Rooting the JS debugger Had the wrong patch marked as checked in.
Attachment #710030 - Flags: checkin+
More easy stuff. All known remaining hazards in Debugger.cpp are from ScriptFrameIter, which is going to be... tricky.
Attachment #713179 - Flags: review?(terrence)
Attachment #710030 - Attachment is obsolete: true
Comment on attachment 713179 [details] [diff] [review] More rooting in Debugger Review of attachment 713179 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Sorry for the delay, had this open since 10:30, but forgot hit submit. :-(
Attachment #713179 - Flags: review?(terrence) → review+
The Debugger patch breaks the compile because I ended up changing some jsdbgapi APIs. I'd rather make the change, though, if we're ok to start unleashing the stack rooting to the world.
Attachment #714101 - Flags: review?(terrence)
Comment on attachment 714101 [details] [diff] [review] Some minor rooting in JSD and xpconnect for some should-be-private JSAPI changes Review of attachment 714101 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Steve Fink [:sfink] from comment #21) > Created attachment 714101 [details] [diff] [review] > Some minor rooting in JSD and xpconnect for some should-be-private JSAPI > changes > > The Debugger patch breaks the compile because I ended up changing some > jsdbgapi APIs. I'd rather make the change, though, if we're ok to start > unleashing the stack rooting to the world. Cry havoc, let slip, etc.
Attachment #714101 - Flags: review?(terrence) → review+
Thanks for the save, njn. (I *did* have a green try push, dammit!) I'm done with this bug, finally.
Whiteboard: [leave open]
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: