Closed Bug 1237564 Opened 6 years ago Closed 6 years ago

Differential Testing: Different output message involving let

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
All
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed
firefox47 --- fixed
firefox-esr45 --- fixed

People

(Reporter: gkw, Assigned: jandem)

References

Details

(Keywords: regression, testcase, Whiteboard: [fuzzblocker])

Attachments

(1 file)

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".
Flags: needinfo?(jdemooij)
Flags: needinfo?(efaustbmo)
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; })();
Flags: needinfo?(jdemooij) → needinfo?(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.
Flags: needinfo?(shu)
Flags: needinfo?(efaustbmo)
Attached patch PatchSplinter Review
The Ion code in IonBuilder::testGlobalLexicalBinding looks correct, so I only had to fix Baseline.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8706428 - Flags: review?(shu)
Comment on attachment 8706428 [details] [diff] [review]
Patch

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

Thanks for the fix!
Attachment #8706428 - Flags: review?(shu) → review+
https://hg.mozilla.org/mozilla-central/rev/a769d6df2553
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Has Regression Range: --- → yes
Has STR: --- → yes
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].
Flags: needinfo?(jdemooij)
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.
Flags: needinfo?(jdemooij)
Attachment #8706428 - Flags: approval-mozilla-esr45?
Comment on attachment 8706428 [details] [diff] [review]
Patch

Simplify the life of the fuzzer, taking it.
Should be in 45.1.0
Attachment #8706428 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
You need to log in before you can comment on or make changes to this bug.