Closed Bug 1175233 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(2 files)

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);
}
Attached file test case.
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Duplicate of this bug: 1174180
Duplicate of this bug: 1175680
You need to log in before you can comment on or make changes to this bug.