Closed
Bug 838014
Opened 9 years ago
Closed 9 years ago
Rooting in jsapi-tests/
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(5 files, 2 obsolete files)
7.44 KB,
patch
|
terrence
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
27.14 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
16.24 KB,
patch
|
terrence
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
16.36 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
14.02 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
Another bug for various rooting resulting from scanning through the static analysis output.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #710028 -
Flags: review?(terrence)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #710029 -
Flags: review?(terrence)
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #710031 -
Flags: review?(terrence)
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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 7•9 years ago
|
||
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)
Assignee | ||
Comment 8•9 years ago
|
||
Ok, I think I have this fixed up now.
Attachment #710968 -
Flags: review?(terrence)
Assignee | ||
Updated•9 years ago
|
Attachment #710031 -
Attachment is obsolete: true
Comment 9•9 years ago
|
||
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+
Assignee | ||
Comment 10•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Attachment #710028 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #710029 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #710968 -
Flags: checkin+
Assignee | ||
Comment 11•9 years ago
|
||
Landing what's been reviewed so far. http://hg.mozilla.org/integration/mozilla-inbound/rev/c6757e68cfc9 http://hg.mozilla.org/integration/mozilla-inbound/rev/272c0e4616ab http://hg.mozilla.org/integration/mozilla-inbound/rev/968921905c81
Assignee | ||
Updated•9 years ago
|
Whiteboard: [leave open]
Assignee | ||
Updated•9 years ago
|
Attachment #710030 -
Flags: checkin+
Assignee | ||
Comment 12•9 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/5abed7bd3cb7 http://hg.mozilla.org/integration/mozilla-inbound/rev/b5ec7ce219b2 http://hg.mozilla.org/integration/mozilla-inbound/rev/ced35810adb2 http://hg.mozilla.org/integration/mozilla-inbound/rev/b37dbf89509f - backed out 272c0e4616ab http://hg.mozilla.org/integration/mozilla-inbound/rev/2e94eda6a728 - backed out c6757e68cfc9 http://hg.mozilla.org/integration/mozilla-inbound/rev/c6757e68cfc9 http://hg.mozilla.org/integration/mozilla-inbound/rev/272c0e4616ab http://hg.mozilla.org/integration/mozilla-inbound/rev/968921905c81
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 710030 [details] [diff] [review] Rooting the JS debugger is not
Attachment #710030 -
Flags: checkin+
Comment 14•9 years ago
|
||
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+
Assignee | ||
Comment 15•9 years ago
|
||
(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!
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/347b76c470d2 https://hg.mozilla.org/mozilla-central/rev/1aa6e7308f8d https://hg.mozilla.org/mozilla-central/rev/fc5e57e246e7
Assignee | ||
Updated•9 years ago
|
Attachment #710029 -
Flags: checkin+
Assignee | ||
Comment 18•9 years ago
|
||
Comment on attachment 710030 [details] [diff] [review] Rooting the JS debugger Had the wrong patch marked as checked in.
Attachment #710030 -
Flags: checkin+
Assignee | ||
Comment 19•9 years ago
|
||
More easy stuff. All known remaining hazards in Debugger.cpp are from ScriptFrameIter, which is going to be... tricky.
Attachment #713179 -
Flags: review?(terrence)
Assignee | ||
Updated•9 years ago
|
Attachment #710030 -
Attachment is obsolete: true
Comment 20•9 years ago
|
||
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+
Assignee | ||
Comment 21•9 years ago
|
||
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 22•9 years ago
|
||
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+
![]() |
||
Comment 23•9 years ago
|
||
The landing of "More rooting in debugger" and my follow-up bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/19857f43d44b https://hg.mozilla.org/integration/mozilla-inbound/rev/77bd010e0e62
Comment 24•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/19857f43d44b https://hg.mozilla.org/mozilla-central/rev/5cbd883b62b1
Assignee | ||
Comment 25•9 years ago
|
||
Thanks for the save, njn. (I *did* have a green try push, dammit!) I'm done with this bug, finally.
Whiteboard: [leave open]
Assignee | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•