1785200 - Differential testing: TypeError only with CacheIR

Status: RESOLVED FIXED

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

Description

[lukas.bernhard]

Steps to reproduce:

During differential fuzzing, I encountered a miscomputation. The attached sample throws an exception, depending on whether ion is enabled or not. Reproduces on git commit: a8ce393df9c82b1166aac3ea2e02f5e6acfbe3d3
Bisecting didn't identify a commit; the oldest version I managed to build features the miscomputation.

sample.js:

function main() {
    const map = new WeakMap();
    const v21 = [];
    const v24 = Object.defineProperty(v21,5,map);
    for (let idx = 0; idx < 10; idx++) {
        const v27 = Reflect.get(v24,idx,map);
    }
}
main();

obj-x86_64-pc-linux-gnu/dist/bin/js --fast-warmup --no-threads --cpu-count=1 --ion-offthread-compile=off --fuzzing-safe --differential-testing sample.js

Throws: sample.js:6:29 TypeError: get method called on incompatible Array

obj-x86_64-pc-linux-gnu/dist/bin/js --no-threads --cpu-count=1 --ion-offthread-compile=off --fuzzing-safe --differential-testing --no-ion sample.js

Does not throw. d8 doesn't throw either.

Comment 1

[André Bargull [:anba]]

Cleaned up test case, throws a TypeError with --no-threads --no-ion --fast-warmup, but no TypeError with --no-threads --no-ion.

function main() {
  const map = new WeakMap();

  const array = [];

  Object.defineProperty(array, 5, {
    get: WeakMap.prototype.get,
  });

  for (let idx = 0; idx < 10; idx++) {
    Reflect.get(array, idx, map);
  }
}
main();

Comment 2

[André Bargull [:anba]]

Updated title to reflect that this is a CacheIR and not an Ion bug.

Comment 3

[Iain Ireland [:iain]]

The problem here is with sparse element access. GetSparseElementHelper assumes that the receiver and the target are the same, but that isn't the case for cases like Reflect.get that use GetElemSuper. This matters if we end up calling a getter, because the getter can observe the this value.

The easy fix is to disable the sparse element IC for GetElemSuper. After that fix, the same testcase runs into a problem in tryAttachGenericElement. Although NativeGetElement takes a receiver argument, emitCallNativeGetElementResult (and visitCallNativeGetElement, the transpiled equivalent) always pass the target as the receiver. This case is a backstop, so it probably makes sense to support super here.

Looking at the other GetProp ICs, I think the rest are okay: they either use the receiver, check isSuper() already, or otherwise verify that the property can't be a getter (eg the arguments object ICs all check hasOverriddenX). It would be nice if we could find a more robust way to ensure that we don't ignore the receiver, though.

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1785200: Fix receiver handling in sparse and generic element ICs r=jandem

It's probably possible to juggle registers around to make this work on 32-bit x86, but it doesn't seem worthwhile. copyToScratchValueRegister doesn't work, because AutoCallVM::prepare uses the output as a scratch register.

I added one testcase specifically targeting the original fuzzbug, and another slightly broader testcase based on the testcase we added for tryAttachGenericElement (bug1488786.js).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4edc744d8b73
Fix receiver handling in sparse and generic element ICs r=jandem

Comment 6

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/4edc744d8b73