838014 - Rooting in jsapi-tests/

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Another bug for various rooting resulting from scanning through the static analysis output.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Rooting in jsapi-tests/

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Rooting in the JS shell

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Rooting the JS debugger

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.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Rooting in ionmonkey

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 710028 [details] [diff] [review]
Rooting in jsapi-tests/

Review of attachment 710028 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this one looked tedious.

Comment 6

[Terrence Cole [:terrence]]

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.

Comment 7

[Terrence Cole [:terrence]]

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.

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Rooting in ionmonkey

Ok, I think I have this fixed up now.

Comment 9

[Terrence Cole [:terrence]]

Comment on attachment 710968 [details] [diff] [review]
Rooting in ionmonkey

Review of attachment 710968 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 10

[Steve Fink [:sfink] [:s:]]

(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.

Comment 11

[Steve Fink [:sfink] [:s:]]

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

Comment 12

[Steve Fink [:sfink] [:s:]]

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

Comment 13

[Steve Fink [:sfink] [:s:]]

Comment on attachment 710030 [details] [diff] [review]
Rooting the JS debugger

is not

Comment 14

[Jason Orendorff [:jorendorff]]

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?

Comment 15

[Steve Fink [:sfink] [:s:]]

(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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/347b76c470d2
https://hg.mozilla.org/mozilla-central/rev/1aa6e7308f8d
https://hg.mozilla.org/mozilla-central/rev/fc5e57e246e7

Comment 18

[Steve Fink [:sfink] [:s:]]

Comment on attachment 710030 [details] [diff] [review]
Rooting the JS debugger

Had the wrong patch marked as checked in.

Comment 19

[Steve Fink [:sfink] [:s:]]

Attachment: More rooting in Debugger

More easy stuff. All known remaining hazards in Debugger.cpp are from ScriptFrameIter, which is going to be... tricky.

Comment 20

[Terrence Cole [:terrence]]

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. :-(

Comment 21

[Steve Fink [:sfink] [:s:]]

Attachment: 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.

Comment 22

[Terrence Cole [: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.

Comment 23

[Nicholas Nethercote [inactive]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19857f43d44b
https://hg.mozilla.org/mozilla-central/rev/5cbd883b62b1

Comment 25

[Steve Fink [:sfink] [:s:]]

Thanks for the save, njn. (I *did* have a green try push, dammit!)

I'm done with this bug, finally.