Closed
Bug 838014
Opened 12 years ago
Closed 12 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•12 years ago
|
||
Attachment #710028 -
Flags: review?(terrence)
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #710029 -
Flags: review?(terrence)
Assignee | ||
Comment 3•12 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•12 years ago
|
||
Attachment #710031 -
Flags: review?(terrence)
Comment 5•12 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•12 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•12 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•12 years ago
|
||
Ok, I think I have this fixed up now.
Attachment #710968 -
Flags: review?(terrence)
Assignee | ||
Updated•12 years ago
|
Attachment #710031 -
Attachment is obsolete: true
Comment 9•12 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•12 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•12 years ago
|
Attachment #710028 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #710029 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #710968 -
Flags: checkin+
Assignee | ||
Comment 11•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave open]
Assignee | ||
Updated•12 years ago
|
Attachment #710030 -
Flags: checkin+
Assignee | ||
Comment 12•12 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•12 years ago
|
||
Comment on attachment 710030 [details] [diff] [review]
Rooting the JS debugger
is not
Attachment #710030 -
Flags: checkin+
Comment 14•12 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•12 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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #710029 -
Flags: checkin+
Assignee | ||
Comment 18•12 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•12 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•12 years ago
|
Attachment #710030 -
Attachment is obsolete: true
Comment 20•12 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•12 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•12 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•12 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•12 years ago
|
||
Assignee | ||
Comment 25•12 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•12 years ago
|
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.
Description
•