Fix some static analysis warnings in js/src

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

({dev-doc-needed})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave-open])

Attachments

(9 attachments)

Posted patch v0Splinter Review
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.
Attachment #757714 - Flags: review?(bhackett1024)
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.
Attachment #757714 - Flags: review?(bhackett1024) → review+
Whiteboard: [leave-open]
Attachment #757714 - Flags: checkin+
Posted patch v0: ToNumberSplinter Review
This makes ToNumber take a MutableHandleValue.
Attachment #758830 - Flags: review?(sphink)
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.
Attachment #758831 - Flags: review?(sphink)
Attachment #758830 - Flags: review?(sphink) → review+
Attachment #758831 - Flags: review?(sphink) → review+
Backed out the ToNumber changes so we can investigate an SM(r) crash.
https://hg.mozilla.org/integration/mozilla-inbound/rev/702e35c02b97
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.
Attachment #767500 - Flags: review?(sphink)
This weeks inevitable breakage. When are we getting the static analysis on TBPL again?
Attachment #767510 - Flags: review?(sphink)
Attachment #767510 - Flags: review?(sphink) → review+
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.
Attachment #767500 - Flags: review?(sphink) → review+
This switches the type of Invoke/InvokeGetterSetter to Value instead of const Value&.
Attachment #767986 - Flags: review?(sphink)
TokenStream's matchContextualKeyword should take a Handle<PropertyName*>. This is the only change needed to kill our last real rooting hazard... again.
Attachment #767987 - Flags: review?(sphink)
Blocks: 885281
Attachment #767987 - Flags: review?(sphink) → review?(wingo)
The majority of users are StringValue(str) -> v.toString, so I added StringToSource.
Attachment #768006 - Flags: review?(sphink)
Posted patch ToInteger v0Splinter Review
I'm actually wondering if we shouldn't make CallArgs::operator[] return Handle.
Attachment #768014 - Flags: review?(sphink)
Attachment #758830 - Flags: checkin+
Attachment #767500 - Flags: checkin+
Attachment #758831 - Flags: checkin+
Attachment #767510 - Flags: checkin+
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.
Attachment #767987 - Flags: review?(wingo) → review+
(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.
Attachment #767986 - Flags: review?(sphink) → review+
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.
Attachment #768006 - Flags: review?(sphink) → review+
Attachment #768014 - Flags: review?(sphink) → review+
No longer blocks: ExactRooting
Attachment #767986 - Flags: checkin+
Attachment #767987 - Flags: checkin+
Attachment #768006 - Flags: checkin+
Attachment #768014 - Flags: checkin+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.