1757634 - Differential testing: ion miscompute related to Object.freeze

Status: RESOLVED FIXED

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

Description

[lukas.bernhard]

Steps to reproduce:

During differential fuzzing, I encountered a miscomputation regarding Object.freeze.
The attached sample computes different values, depending on whether ion is enabled or not.
Reproduces on git commit: 27dcec8e209a32707de61dc914ea8edf81075359

function main() {
  const v21 = [];
  const v23 = [0,0,0,0,v21];
  
  const v31 = [].__proto__;
  v31[0] = 123;
  Object.freeze([].__proto__);
  
  for (const v38 of v23) {
      v21[0] = 321;
  }
  print(v23[4][0]);  // w/o ion: 123, with ion: 321
}
main();

Actual results:

./js --fast-warmup --no-threads --cpu-count=1 --ion-offthread-compile=off --fuzzing-safe --differential-testing diff.js
=> computes 321

./js --no-threads --cpu-count=1 --baseline-warmup-threshold=10 --fuzzing-safe --differential-testing --no-ion diff.js
=> computes 123

Expected results:

v8 computes 123

Comment 1

[Nicolas B. Pierron [:nbp] {away}]

Jan, Iain, would you know what is going on with WarpBuilder / IonMonkey?

I would have assumed that Ion is correct, but apparently not.
It seems that v21[0] should lookup the __proto__ and be prevented from setting the value in v21.

Comment 2

[Iain Ireland [:iain]]

This actually has very little to do with Warp/Ion. It's a difference between the C++ interpreter and the jits. Specifically, the CacheIR implementation of tryAttachAddOrUpdateSparseElement is incorrect: it doesn't check to see if the prototype is frozen. The first time we try to assign to v21[0], we attach an IC that doesn't enforce the shadowing behaviour, then call into the VM for the actual operation. The second time, we use the IC and incorrectly set the value.

The following testcase has differential behaviour with/without --blinterp-eager:

const arr = [];

Array.prototype[0] = 123;
Object.freeze(Array.prototype);

for (var i = 0; i < 2; i++) {
  arr[0] = 321;
}
print(arr[0]);

At first glance, I think we should probably be reusing some of the checks from tryAttachSetDenseElementHole in tryAttachAddOrUpdateSparseElement.

Comment 3

[Jan de Mooij [:jandem]]

There are some subtle bugs here, I'll post some patches.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1757634 part 1 - Factor out PrototypeMayHaveIndexedProperties. r?iain!

This also lets us delete a debug-only function with the same name that ended up
doing the same thing. See next patch for an explanation.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1757634 part 2 - Use PrototypeMayHaveIndexedProperties in the sparse element code. r?iain!

This changes ObjectMayHaveExtraIndexedProperties(obj->proto) to
PrototypeMayHaveIndexedProperties(obj).

There's a subtle difference: the former does not ensure initializedLength == 0
for the first (prototype) object. The latter treats each prototype object the same
way. This fixes the issue reported in the bug.

Depends on D140812

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1757634 part 3 - Make ObjectMayHaveExtraIndexedProperties static. r?iain!

We also have to move these functions before the first caller.

Depends on D140813

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1757634 part 4 - Use CanAttachAddElement in tryAttachAddOrUpdateSparseElement. r?iain!

This change isn't strictly necessary after the previous patches, but it's more
consistent and more robust.

Depends on D140814

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1757634 part 5 - Add test. r?iain!

Depends on D140815

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/650dbe2dab51
part 1 - Factor out PrototypeMayHaveIndexedProperties. r=iain
https://hg.mozilla.org/integration/autoland/rev/cc183e448394
part 2 - Use PrototypeMayHaveIndexedProperties in the sparse element code. r=iain
https://hg.mozilla.org/integration/autoland/rev/d541260d0b12
part 3 - Make ObjectMayHaveExtraIndexedProperties static. r=iain
https://hg.mozilla.org/integration/autoland/rev/a486536ee3cc
part 4 - Use CanAttachAddElement in tryAttachAddOrUpdateSparseElement. r=iain
https://hg.mozilla.org/integration/autoland/rev/51fbd63e33b1
part 5 - Add test. r=iain

Comment 10

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/650dbe2dab51
https://hg.mozilla.org/mozilla-central/rev/cc183e448394
https://hg.mozilla.org/mozilla-central/rev/d541260d0b12
https://hg.mozilla.org/mozilla-central/rev/a486536ee3cc
https://hg.mozilla.org/mozilla-central/rev/51fbd63e33b1