879079 - Fix some static analysis warnings in js/src

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

This fixes ValueToId, ToString and ToAtom by making them take a Handle when <CanGC>. This was mostly just changing args.get(i) to args.getHandle(i) and args[i] to args.handleAt(i). In spots where this wasn't appropriate I either added a new Rooted or -- for hot(ish) paths -- I re-used an existing root or fromMarkedLocation'd as required.

Comment 1

[Brian Hackett [Laid off!]]

Comment on attachment 757714 [details] [diff] [review]
v0

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

::: js/public/CallArgs.h
@@ +303,5 @@
>      /*
> +     * Returns the i-th zero-indexed argument as a handle, or |undefined| if
> +     * there's no such argument.
> +     */
> +    HandleValue getHandle(unsigned i) const {

This name is pretty confusing since there is also a handleAt(i) method.  How about handleOrUndefinedAt?

::: js/src/jsarray.cpp
@@ +1134,5 @@
>  
>      JS_ASSERT(start == MAX_ARRAY_INDEX + 1);
>      RootedValue value(cx);
>      RootedId id(cx);
> +    Value indexv;

Can you make this a RootedValue to not abuse fromMarkedLocation?  This code will almost never execute.

::: js/src/jsproxy.cpp
@@ +692,4 @@
>      if (!str)
>          return false;
>      rval.setString(str);
> +    v = v_;

Moving this assignment down introduces a rooting hazard.  This function looks like it can be handled the same way as Trap1?

::: js/src/jsreflect.cpp
@@ +3084,5 @@
>          return JS_FALSE;
>  
>      RootedValue val(cx);
>      if (!serialize.program(pn, &val)) {
> +        args.rval().setUndefined();

setNull

::: js/src/vm/Shape-inl.h
@@ +289,5 @@
>  #endif
>      if (self->hasShortID()) {
>          int16_t id = self->shortid();
> +        if (id < 0) {
> +            Value v = Int32Value(id);

Ditto earlier comment, this code is cold.

Comment 2

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9305b1486c2d

Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=bc861796988c

Comment 3

[Terrence Cole [:terrence]]

Attachment: v0: ToNumber

This makes ToNumber take a MutableHandleValue.

Comment 4

[Terrence Cole [:terrence]]

Attachment: v0: DefineOwnProperty

This makes the |Value descriptor| parameter of js_DefineOwnProperty (the jsobj.h instance) a Handle. It also moves the function inside namespace js::, since it will be identical to the friend api version when we add handles to the api.

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9305b1486c2d

Comment 6

[Terrence Cole [:terrence]]

Tried:    https://tbpl.mozilla.org/?tree=Try&rev=caec4355d935

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6d95672c5a68
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7f3ec3f49260

Comment 7

[Terrence Cole [:terrence]]

Backed out the ToNumber changes so we can investigate an SM(r) crash.
https://hg.mozilla.org/integration/mozilla-inbound/rev/702e35c02b97

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7f3ec3f49260

Comment 9

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d07c8c75774

Comment 10

[Terrence Cole [:terrence]]

Attachment: v0: handlify ToNumber (the other one)

The hard cases are places where we put an atom in a Value then immediately unpack it to call the static StringToNumber. I just exposed StringToNumber internally. There is one slow path where I had to add a Rooted. The rest are just trivial calls to args.handleAt.

Comment 11

[Terrence Cole [:terrence]]

Attachment: v0: more trivial, but serious exact rooting fixes

This weeks inevitable breakage. When are we getting the static analysis on TBPL again?

Comment 12

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

Comment on attachment 767500 [details] [diff] [review]
v0: handlify ToNumber (the other one)

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

Oh good. This is a nice sweep through a bunch of stuff.

Comment 13

[Terrence Cole [:terrence]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d219351389c7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a8c3cd7cbf73

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d07c8c75774

Comment 15

[Terrence Cole [:terrence]]

Attachment: Invoke[GetterSetter] v0

This switches the type of Invoke/InvokeGetterSetter to Value instead of const Value&.

Comment 16

[Terrence Cole [:terrence]]

Attachment: matchContextualKeyword v0

TokenStream's matchContextualKeyword should take a Handle<PropertyName*>. This is the only change needed to kill our last real rooting hazard... again.

Comment 17

[Terrence Cole [:terrence]]

Attachment: ValueToSource v0

The majority of users are StringValue(str) -> v.toString, so I added StringToSource.

Comment 18

[Terrence Cole [:terrence]]

Attachment: ToInteger v0

I'm actually wondering if we shouldn't make CallArgs::operator[] return Handle.

Comment 19

[Andy Wingo [:wingo]]

Comment on attachment 767987 [details] [diff] [review]
matchContextualKeyword v0

LGTM; I didn't realize that Handles would automatically unwrap their values.  In the future it might be a good idea to remove this behavior, to have the compiler let us know about this kind of error.

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d219351389c7
https://hg.mozilla.org/mozilla-central/rev/a8c3cd7cbf73

Comment 21

[Terrence Cole [:terrence]]

(In reply to Andy Wingo from comment #19)
> Comment on attachment 767987 [details] [diff] [review]
> matchContextualKeyword v0
> 
> LGTM; I didn't realize that Handles would automatically unwrap their values.
> In the future it might be a good idea to remove this behavior, to have the
> compiler let us know about this kind of error.

Bug 817164. Although considering that we explicitly don't want to root some paths for performance, we may never get to a spot where this is possible. In the meantime, the static analysis -- how I found this -- will keep this from regressing too far.

Comment 22

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

Comment on attachment 768006 [details] [diff] [review]
ValueToSource v0

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

This one is a little tricky, since it's a legit use of a bare JSString* in a place where you'd expect a Handle. If the JSAPI were Handlified, that would be fine without a comment, but right now I think it'd be better to mention /* No Handle needed. */ or something.

Comment 23

[Terrence Cole [:terrence]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/281bdbc7b3c4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/620c2af655b5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0cc4ad9881a7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cc11bf500293

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/281bdbc7b3c4
https://hg.mozilla.org/mozilla-central/rev/620c2af655b5
https://hg.mozilla.org/mozilla-central/rev/0cc4ad9881a7
https://hg.mozilla.org/mozilla-central/rev/cc11bf500293