Closed Bug 1123011 Opened 7 years ago Closed 7 years ago

Differential Testing: Different output message involving "use strict"

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- affected

People

(Reporter: gkw, Assigned: bhackett1024)

Details

(Keywords: regression, testcase)

Attachments

(1 file)

count = 0
try {
    function f() {
        eval("\"use strict\";this.b=(function(){})")
    }
    for (var j = 0; j < 9; ++j) {
        count += 1
        print("count = " + count + ": " + f())
    }
} catch (e) {}

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-369a8f14ccf8 --fuzzing-safe --no-threads --ion-eager testcase.js
count = 1: undefined
count = 2: undefined
count = 3: undefined
count = 4: undefined
count = 5: undefined
count = 6: undefined
count = 7: undefined
count = 8: undefined

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-369a8f14ccf8 --fuzzing-safe --no-threads --baseline-eager testcase.js
count = 1: undefined
count = 2: undefined
count = 3: undefined
count = 4: undefined
count = 5: undefined
count = 6: undefined
count = 7: undefined
count = 8: undefined
count = 9: undefined

Tested this on m-c rev 369a8f14ccf8.

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-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect is running.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/95e326d4fd7d
parent:      214107:e066f112521a
user:        Hannes Verschore
date:        Wed Nov 05 16:46:21 2014 +0100
summary:     Bug 914255 - Reduce the number of objects tracked in a TypeSet, r=bhackett

Hannes, is bug 914255 a likely regressor?
Blocks: 914255
Flags: needinfo?(hv1989)
I'm pretty sure that if you try the following it will fail before the revision you are proposing.
(I'm not on my own computer, so I haven't checked if the '40' is high enough. But I think we track up to 32 objects before this revision. So 40 should do it.)


count = 0
try {
    function f() {
        eval("\"use strict\";this.b=(function(){})")
    }
    for (var j = 0; j < 40; ++j) {
        count += 1
        print("count = " + count + ": " + f())
    }
} catch (e) {}
Flags: needinfo?(hv1989)
You're right. It seems to go back before end-2013 [1]. Anyone able to look at this in any case?

[1] https://hg.mozilla.org/mozilla-central/rev/df3c2a1e86d3
No longer blocks: 914255
Flags: needinfo?(jdemooij)
Flags: needinfo?(hv1989)
Simpler testcase:

var global = this;
function f() {
    return eval("'use strict'; this;");
}
for (var j = 0; j < 5; ++j) {
    assertEq(f(), global);
}
This probably goes back to bug 743394. The problem is that we need to pass the correct, boxed |this| to eval scripts, because in strict mode the JSOP_THIS there will just return what's in the this-slot, without boxing it.

Brian, should we check if the eval string contains |use strict|, in addition to "arguments"/"eval" we check now? Can you think of a better fix?
Flags: needinfo?(jdemooij) → needinfo?(bhackett1024)
Flags: needinfo?(hv1989)
Attached patch patchSplinter Review
I don't think it's necessary to avoid executing the eval at all if it contains 'use strict', as in this case the strictness just affects the 'this' value we compute, and it's simple to do that in DirectEvalStringFromIon.  This patch does that, and also avoids optimizing 'eval' in arrow functions, as it looks like we weren't handling 'this' computation there correctly.
Assignee: nobody → bhackett1024
Flags: needinfo?(bhackett1024)
Attachment #8552722 - Flags: review?(jdemooij)
Comment on attachment 8552722 [details] [diff] [review]
patch

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

Thanks.
Attachment #8552722 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/4d669523c7dd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.