1604631 - Differential Testing: Different output message involving evalcx

Status: RESOLVED FIXED

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

Description

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

try {
    x = new Uint8ClampedArray;
    x.byteOffset;
    evalcx("let x = x.subarray(1);", this);
} catch (e) {
    print(e);
}

$ ./js-dbg-64-dm-linux-x86_64-930ad6def3c7 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
ReferenceError: can't access lexical declaration `x' before initialization
$

$ ./js-dbg-64-dm-linux-x86_64-930ad6def3c7 --fuzzing-safe --no-threads --ion-eager testcase.js
$

Tested this on m-c rev 930ad6def3c7.

My configure flags are:

AR=ar sh ./configure --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/a49a644cc220
user: Jan de Mooij
date: Fri Dec 13 19:24:56 2019 +0000
summary: Bug 1603373 part 4 - Remove ambiguous TypeSet::PrimitiveType(ValueType) method. r=iain,tcampbell

Jan, is bug 1603373 a likely regressor?

Comment 1

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

try {
    x = [];
    x[0];
    evalcx("let x = x.subarray(1);", this);
} catch (e) {
    print(e);
}

$ ./js-dbg-64-dm-linux-x86_64-930ad6def3c7 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
ReferenceError: can't access lexical declaration `x' before initialization

$ ./js-dbg-64-dm-linux-x86_64-930ad6def3c7 --fuzzing-safe --no-threads --ion-eager testcase.js
TypeError: undefined is not a function

Actually, subarray seems more related.

Comment 2

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

Here's another testcase without subarray, so evalcx may be another possible culprit.

try {
    evalcx("let c = x = 5, x;", this);
} catch (e) {
    print(e);
}

Comment 3

[Jan de Mooij [:jandem]]

Good find.

The script has a DEFLET x and SETGNAME x. When IonBuilder compiles the SETGNAME, the property does not exist yet on the lexical environment, so in testGlobalLexicalBinding it attaches a constraint and then assumes it's on the global.

Before bug 1603373, DEFLET/DefLexicalOperation would then add a magic type for the property and we'd invalidate. Now we ignore magic types for HeapTypeSets and we don't invalidate.

I'm not sure what the best fix is. The old behavior was bad and confusing (it would end up treating the TDZ magic value as TYPE_FLAG_LAZYARGS) and it's nice to not have magic types in HeapTypeSets.

Comment 4

[Jan de Mooij [:jandem]]

I have a patch for this but I want to land bug 1605066 first to simplify the environment object + TI situation.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1604631 - Trigger constraints when defining an uninitialized global lexical binding. r?iain!

Since bug 1603373 we no longer store magic types in HeapTypeSets. This broke an
Ion optimization because it uses constraints to guard a property does not exist
on the global lexical environment. This patch explicitly fires the constraint
when defining an uninitialized global lexical.

I was unable to write a test for this that doesn't require --ion-eager.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Jan, is this ready to land?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4902a2dea0f
Trigger constraints when defining an uninitialized global lexical binding. r=iain

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f4902a2dea0f

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact here which justifies uplift consideration or can this fix ride Fx74 to release?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

Is there a user impact here which justifies uplift consideration or can this fix ride Fx74 to release?

It's an edge case that's --ion-eager specific I think so I don't think we need to uplift.