819611 - IonMonkey: Differential Testing: Getting different output w/without --ion-eager with Object.freeze

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

x = []
x.push(1)
x.unshift(0)
x.unshift(0)
Object.freeze(x)
try {
    x.sort(function() {
        x.length = 13
    })
} catch (e) {}
print(uneval(this))

shows the following output (only the last few lines copied below) on js opt shell on IonMonkey changeset 32638e411218 without --ion-eager:

/snip
}, wrapWithProto:function wrapWithProto() {
    [native code]
}, x:[0, 0, 1]})

but the following output with --ion-eager:

/snip
}, wrapWithProto:function wrapWithProto() {
    [native code]
}, x:[0, 0, 1, , , , , , , , , , ,]})

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   111211:2583a19e59ef
user:        Kannan Vijayan
date:        Tue Oct 23 22:18:11 2012 -0400
summary:     Bug 795801 - IC StrictPropertyOp setters in IonMonkey. (r=dvander)

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

x = [0, 0]
Object.freeze(x).map(function() {
    x.length = 6
})
print(x)

shows the following without --ion-eager:

0,0

but shows the following with --ion-eager:

0,0,,,,

Kannan, do you know if bug 795801 is indeed related?

Comment 2

[Kannan Vijayan [:djvj]]

(In reply to Gary Kwong [:gkw, :nth10sd] from comment #1)
> Kannan, do you know if bug 795801 is indeed related?

It seems related from my initial tracing.  I don't understand exactly how yet (it should be mimicing the same behaviour as the interpreter), but I'm looking into it.

Comment 3

[Kannan Vijayan [:djvj]]

It seems from investigation that the checker method (IsPropertySetterCallInlineable) used by the ion IC to identify is missing something before adding a stub to call the setter StrictPropertyOp.

The second run around, the setter gets called mutates the frozen array.

Trying to figure out what the missing check(s) are.  Since the object is frozen before
any of these methods are run, the issue is fixing the checks before attaching the stubs, as opposed to any guards compiled into the stub itself.

Comment 4

[Kannan Vijayan [:djvj]]

Attachment: Fix.

Add writability check to shape when attaching setter-call stubs.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

After this lands on m-c, the patch might need backporting to aurora.

Comment 6

[Kannan Vijayan [:djvj]]

Attachment: Test case.

Test case pulled from comment 1.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 690530 [details] [diff] [review]
Test case.

rs=me

Comment 8

[Kannan Vijayan [:djvj]]

Fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/a367c96b9459
Testcase: https://hg.mozilla.org/integration/mozilla-inbound/rev/ecdeb0c29a23

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a367c96b9459
https://hg.mozilla.org/mozilla-central/rev/ecdeb0c29a23

Comment 10

[Kannan Vijayan [:djvj]]

Comment on attachment 690497 [details] [diff] [review]
Fix.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
https://bugzilla.mozilla.org/show_bug.cgi?id=795801

User impact if declined:
Incorrect JS behaviour (frozen objects can be modified).

Testing completed (on m-c, etc.): 
Green on m-i tbpl.

Risk to taking this patch (and alternatives if risky):
Minimal.  Trivial addition of extra check that disables optimizing behaviour in one case.

String or UUID changes made by this patch: 
None.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cda5f407ec84
https://hg.mozilla.org/releases/mozilla-aurora/rev/6d7da67a7c82

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.