Closed Bug 1133389 Opened 5 years ago Closed 5 years ago

Differential Testing: Different output message involving arguments.callee

Categories

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

x86_64
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: gkw, Assigned: nbp)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, testcase)

Attachments

(1 file)

function f() {
    function g() {
        print(x)
    }
    g()
}
try {
    Object.defineProperty(this, "x", {
        get: function() {
            return arguments.callee.caller.caller.a
        }
    })
    for (var k = 0; k < 2; k++) {
        f()
    }
} catch (e) {}

$ ./js-dbg-64-dm-nsprBuild-darwin-81f979b17fbd --fuzzing-safe --no-threads --ion-eager testcase.js
undefined

$ ./js-dbg-64-dm-nsprBuild-darwin-81f979b17fbd --fuzzing-safe --no-threads --baseline-eager testcase.js
undefined
undefined

Tested this on m-c rev 81f979b17fbd.

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

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-optimize --enable-more-deterministic --enable-nspr-build -R ~/trees/mozilla-central" -r 81f979b17fbd

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/a50d660f09da
user:        Nicolas B. Pierron
date:        Fri Dec 19 15:28:30 2014 +0100
summary:     Bug 1073033 part 3 - Recover MLambda on bailouts. r=shu

Nicolas, is bug 1073033 a likely regressor?
Flags: needinfo?(nicolas.b.pierron)
// Clearer testcase. With --ion-eager, prints |f| first time and |null| second time.

var o = {}
Object.defineProperty(o, "p", {
    get: function() {
        print(arguments.callee.caller.caller)
    }
});

function f() {
    function g() {
        o.p;
    }
    g()
}

for (var k = 0; k < 2; k++) {
    f()
}
I will look at this issue in 2 weeks.
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8576033 [details] [diff] [review]
Fix FrameIter::matchCallee to consider all inner functions and not only lambdas.

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

::: js/src/vm/Stack.cpp
@@ -1116,5 @@
> -    // invalidating the frame. Thus, if one of the function is not a lambda we can just
> -    // compare it against the calleeTemplate.
> -    if (!fun->isLambda() || !currentCallee->isLambda())
> -        return currentCallee == fun;
> -

Ah, bummer. Sorry I didn't catch this in review.
Attachment #8576033 - Flags: review?(shu) → review+
Bug 1073033 landed in Gecko 37+
https://hg.mozilla.org/mozilla-central/rev/f0f3cba361d1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8576033 [details] [diff] [review]
Fix FrameIter::matchCallee to consider all inner functions and not only lambdas.

Approval Request Comment
[Feature/regressing bug #]:
Bug 1073033

[User impact if declined]:
Wrong JS behaviour with fun.arguments.caller

[Describe test coverage new/current, TreeHerder]:
in-testsuite+

[Risks and why]: 
Their is a little performance risk, for any code which is frequently iterating through a stack of non-escaped & named inner functions.  (which already exists for lambdas)

[String/UUID change made/needed]:
N/A
Attachment #8576033 - Flags: approval-mozilla-beta?
Attachment #8576033 - Flags: approval-mozilla-aurora?
Comment on attachment 8576033 [details] [diff] [review]
Fix FrameIter::matchCallee to consider all inner functions and not only lambdas.

Happy to have a fix for this regression, which was introduced in 37. Beta+ Aurora+
Attachment #8576033 - Flags: approval-mozilla-beta?
Attachment #8576033 - Flags: approval-mozilla-beta+
Attachment #8576033 - Flags: approval-mozilla-aurora?
Attachment #8576033 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.