RObjectState should convert to a native if the type of the value does not match.

RESOLVED FIXED in Firefox 41

Status

()

--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
This Bug is similar to Bug 1174322, except that Recover are expected to not be able to distinguish between Int32 types and Doubles types.

We should handle cases where the and Int32 operation is promoted into a Double operation.  A test case similar to the following should be able to trigger this issue:


var uceFault = function (i) {
    if (i >= 31)
        uceFault = function (i) { return true; };
    return false;
}

function f(i) {
    var obj = {
      i: i,
      v: i + i
    };
    assertRecoveredOnBailout(obj, true);
    assertRecoveredOnBailout(obj.v, true);
    if (uceFault(i) || uceFault(i)) {
        // MObjectState::recover should neither fail,
        // nor coerce its result to an int32.
        assertEq(obj.v, 2 * i);
    }
    return 2 * obj.i;
}

var min = -100;
for (var j = min; j <= 31; ++j) {
    var i = (Math.pow(2, j) | 0;
    f(i);
}
(Assignee)

Comment 1

3 years ago
Created attachment 8623629 [details]
test case.
(Assignee)

Comment 2

3 years ago
Created attachment 8623722 [details] [diff] [review]
RObjectState::recover: Handle cases where the property type does not match the recovered value.
Attachment #8623722 - Flags: review?(bhackett1024)
Comment on attachment 8623722 [details] [diff] [review]
RObjectState::recover: Handle cases where the property type does not match the recovered value.

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

::: js/src/jit/BaselineBailouts.cpp
@@ +1606,5 @@
> +
> +        JitSpew(JitSpew_BaselineBailouts, "Invalidating due to bounds check failure");
> +        if (!Invalidate(cx, outerScript))
> +            return false;
> +    }

Can you bundle up this logic into an InvalidateAfterBailout function?  That should be cleaner and will allow the new comment below to only appear in one place.

::: js/src/jit/Recover.cpp
@@ +1396,5 @@
>                  val.setBoolean(val.toInt32() != 0);
>  
> +            id = NameToId(properties[i].name);
> +            ObjectOpResult result;
> +            if (!js::SetProperty(cx, object, id, val, receiver, result))

no js:: needed

@@ +1399,5 @@
> +            ObjectOpResult result;
> +            if (!js::SetProperty(cx, object, id, val, receiver, result))
> +                return false;
> +            if (!result)
> +                return result.reportError(cx, object, id);

Maybe assert or comment here that the SetProperty call can only fail due to OOM.
Attachment #8623722 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/e613eb7895d6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1174180
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1175680
You need to log in before you can comment on or make changes to this bug.