Closed Bug 614051 Opened 9 years ago Closed 9 years ago

TM: wrong behavior setting existing properties to joined function object values again

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(1 file, 3 obsolete files)

This is either a regression of bug 592412 or it was never fixed in the tracer.

The patch in bug 592412 patches jsobj.cpp without patching corresponding code in jstracer.cpp. The test does not exercise the tracer. The revised test (below) fails.

function f() {
    var a = [], b = [], i;
    for (i = 0; i < 20; i++) {
        a[i] = {m: i};
        b[i] = {};
    }

    for (i = 0; i < a.length; i++) {
        a[i].m = function() { return 0; };
        b[i].m = function() { return 1; };
    }

    assertEq(a[a.length - 2].m === a[a.length - 1].m, false);
    assertEq(b[b.length - 2].m === b[b.length - 1].m, false);
}
f();
blocking2.0: --- → ?
The test doesn't even work without -j. The property cache hit path in the interpreter is busted too.
Attached patch v1 (obsolete) — Splinter Review
OK, two separate bugs really:

  * SETMETHOD over an existing non-method property filled the property
    cache incorrectly, causing wrong behavior in the interpreter and
    jstracer.

  * INITMETHOD over an existing non-method property also filled the property
    cache incorrectly, creating a cache entry that would never hit
    in the interpreter but causing wrong behavior in jstracer.

The proposed fix is not to fill in such cases.
Assignee: general → jorendorff
Attachment #492480 - Flags: review?(brendan)
Attached patch v2 (obsolete) — Splinter Review
fix silly mistake (use of "added" before it's initialized)
Attachment #492480 - Attachment is obsolete: true
Attachment #492655 - Flags: review?(brendan)
Attachment #492480 - Flags: review?(brendan)
I found another bug. Overwriting a method with a different method doesn't work either.

function f() {
    var a = [], i, N = HOTLOOP + 2;
    for (i = 0; i < N; i++) {
        a[i] = {};
        a[i].m = function() { return 0; };
        a[i].m = function() { return 1; };
    }
    assertEq(a[N - 2].m === a[N - 1].m, false);
    for (i = 0; i < N; i++) {
        var f = a[i].m;
        assertEq(f(), 1);
    }
}
f();

Fix coming.
Attached patch v3 (obsolete) — Splinter Review
Attachment #492655 - Attachment is obsolete: true
Attachment #492771 - Flags: review?(brendan)
Attachment #492655 - Flags: review?(brendan)
Comment on attachment 492771 [details] [diff] [review]
v3

Withdrawing. It looks like parallel changes are needed in methodjit/PolyIC.cpp.
Attachment #492771 - Flags: review?(brendan)
Attached patch v4Splinter Review
Actually PolyIC gets it basically right; there was just a little oddity where a method property could be defined in one place and its slot set in a totally different place, apparently by good fortune. My change caused us to hit an assertion in between. Fixed the oddity, kept the assertion.
Attachment #492771 - Attachment is obsolete: true
Attachment #493055 - Flags: review?(brendan)
Comment on attachment 493055 [details] [diff] [review]
v4

Yikes. SetPropHit must die. Thanks for fixing. Can you make any regressor(s) depend on this bug?

/be
Attachment #493055 - Flags: review?(brendan) → review+
blocking2.0: ? → betaN+
Looks like this was pushed last Thursday - should it be marked fixed?

https://hg.mozilla.org/mozilla-central/rev/52d20032116a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 623863
Depends on: 627984
You need to log in before you can comment on or make changes to this bug.