1237564 - Differential Testing: Different output message involving let

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

try {
    evalcx("let x = l()", this);
} catch (e) {}
Object.defineProperty(this, "x", {});
try {
    (function() {
        x = function() {};
        print("foo");
    })();
} catch (e) {}


$ ./js-dbg-64-dm-darwin-dd6d447fc6e0 --fuzzing-safe --no-threads --ion-eager testcase.js
foo

$ ./js-dbg-64-dm-darwin-dd6d447fc6e0 --fuzzing-safe --no-threads --no-baseline --no-ion testcase.js


Tested this on m-c rev dd6d447fc6e0.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r dd6d447fc6e0


Due to skipped revisions, the first bad revision could be any of:
changeset:   https://hg.mozilla.org/mozilla-central/rev/3bcc3881b95d
user:        Shu-yu Guo
date:        Tue Oct 06 14:00:28 2015 -0700
summary:     Bug 589199 - Make a global lexical scope and hook it up to JS entry points. (r=efaust)

changeset:   https://hg.mozilla.org/mozilla-central/rev/b9f647fe2d10
user:        Shu-yu Guo
date:        Tue Oct 06 14:00:29 2015 -0700
summary:     Bug 589199 - Parse and emit bytecode for global lexicals. (r=efaust)

changeset:   https://hg.mozilla.org/mozilla-central/rev/b2d8f1cd3afc
user:        Shu-yu Guo
date:        Tue Oct 06 14:00:29 2015 -0700
summary:     Bug 589199 - Support global lexicals in the interpreter. (r=efaust)

changeset:   https://hg.mozilla.org/mozilla-central/rev/6e4a8943d496
user:        Shu-yu Guo
date:        Tue Oct 06 14:00:29 2015 -0700
summary:     Bug 589199 - Support global lexicals in Baseline. (r=jandem)

changeset:   https://hg.mozilla.org/mozilla-central/rev/26fc971a24c4
user:        Shu-yu Guo
date:        Tue Oct 06 14:00:29 2015 -0700
summary:     Bug 589199 - Support global lexicals in Ion. (r=jandem)

changeset:   https://hg.mozilla.org/mozilla-central/rev/3228ac384c92
user:        Shu-yu Guo
date:        Tue Oct 06 14:00:29 2015 -0700
summary:     Bug 589199 - Fix eval static scope to play with the global lexical scope. (r=efaust)

changeset:   https://hg.mozilla.org/mozilla-central/rev/04eb8f524122
user:        Shu-yu Guo
date:        Tue Oct 06 14:00:29 2015 -0700
summary:     Bug 589199 - Fix up the global lexical scope when merging off-thread compiled scripts. (r=bhackett)

changeset:   https://hg.mozilla.org/mozilla-central/rev/c609df6d3895
user:        Shu-yu Guo
date:        Tue Oct 06 14:00:29 2015 -0700
summary:     Bug 1202902 - Support non-syntactic extensible lexical scopes. (r=billm)

Note that [dis]appearance of "foo". Assuming related to bug 589199. Not sure who to needinfo? since Shu-yu is away for awhile, setting needinfo? from Jan and Eric as a start.

Setting [fuzzblocker] as this is hard to ignore as it seems to involve "let".

Comment 1

[Jan de Mooij [:jandem]]

Good find!

The JITs have an optimization for JSOP_BINDGNAME:

    // We can bind name to the global object if the property exists on the
    // global and is non-configurable, as then it cannot be shadowed.
    if (Shape* shape = script->global().lookup(cx, name)) {
        if (!shape->configurable()) {
            frame.push(ObjectValue(script->global()));
            return true;

This test case first adds the lexical binding and *then* we define the global property, so we do in fact have a lexical binding shadowing a non-configurable global...

If this is indeed what should happen, we can simply fix the code above to check the property doesn't exist on the global lexical scope. The property can't be added to the lexical scope after this, because that will shadow and we will throw an exception. IIUC.

Here's a simpler testcase:

try {
    evaluate("let x = (() => { throw 3 })();");
} catch(e) {
    assertEq(e, 3);
}
Object.defineProperty(this, "x", {});
(function() { x = 3; })();

Comment 2

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> Good find!
> 
> The JITs have an optimization for JSOP_BINDGNAME:
> 
>     // We can bind name to the global object if the property exists on the
>     // global and is non-configurable, as then it cannot be shadowed.
>     if (Shape* shape = script->global().lookup(cx, name)) {
>         if (!shape->configurable()) {
>             frame.push(ObjectValue(script->global()));
>             return true;
> 
> This test case first adds the lexical binding and *then* we define the
> global property, so we do in fact have a lexical binding shadowing a
> non-configurable global...
> 
> If this is indeed what should happen, we can simply fix the code above to
> check the property doesn't exist on the global lexical scope. The property
> can't be added to the lexical scope after this, because that will shadow and
> we will throw an exception. IIUC.
> 

Good find indeed, I missed this case when writing the optimization. Your proposed fix sounds correct.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

The Ion code in IonBuilder::testGlobalLexicalBinding looks correct, so I only had to fix Baseline.

Comment 4

[Shu-yu Guo [:shu]]

Comment on attachment 8706428 [details] [diff] [review]
Patch

Review of attachment 8706428 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the fix!

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a769d6df2553

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

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

Jan, do you mind nominating this for approval‑mozilla‑esr45 as well? It would aid fuzzing on that branch, plus this bug is a [fuzzblocker].

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8706428 [details] [diff] [review]
Patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Simple fix, helps our JS fuzzers find bugs on ESR 45.
User impact if declined: Harder to do differential fuzzing on ESR.
Fix Landed on Version: 46.
Risk to taking this patch (and alternatives if risky): Low risk. It's a straight-forward fix for an edge case - patch landed 3 months ago and no regressions since then.
String or UUID changes made by this patch: None.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8706428 [details] [diff] [review]
Patch

Simplify the life of the fuzzer, taking it.
Should be in 45.1.0

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/64421b085ce2