Closed Bug 838014 Opened 7 years ago Closed 7 years ago

Rooting in jsapi-tests/

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

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!
Duplicate of this bug: 839485
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: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.