1153458 - Assertion failure: index >= size_t(parser.stackDepthAtPC(current)), at jsopcode.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

try {
    __defineGetter__("x", Iterator)()
} catch (e) {}
f = function() {
    return (function() {
        this.x
    })
}()
try {
    f()
} catch (e) {}
f()

asserts js debug shell on m-c changeset eb3a1c0262e4 with --fuzzing-safe --no-threads --baseline-eager at Assertion failure: index >= size_t(parser.stackDepthAtPC(current)), at jsopcode.cpp.

Configure options:

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

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-nspr-build" -r eb3a1c0262e4

=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20150406061259" and the hash "d084a35e8d79".
The "bad" changeset has the timestamp "20150406063758" and the hash "2ceecfb46d8c".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d084a35e8d79&tochange=2ceecfb46d8c

Eric, is bug 1094491 a likely regressor? (guessing from the regression window)

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x174f3c, 0x0000000100794966 js-dbg-64-nsprBuild-darwin-eb3a1c0262e4`js::DecompileValueGenerator(JSContext*, int, JS::Handle<JS::Value>, JS::Handle<JSString*>, int) + 52 at jsopcode.cpp:1756, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100794966 js-dbg-64-nsprBuild-darwin-eb3a1c0262e4`js::DecompileValueGenerator(JSContext*, int, JS::Handle<JS::Value>, JS::Handle<JSString*>, int) + 52 at jsopcode.cpp:1756
    frame #1: 0x0000000100794932 js-dbg-64-nsprBuild-darwin-eb3a1c0262e4`js::DecompileValueGenerator(JSContext*, int, JS::Handle<JS::Value>, JS::Handle<JSString*>, int) + 565 at jsopcode.cpp:1819
    frame #2: 0x00000001007946fd js-dbg-64-nsprBuild-darwin-eb3a1c0262e4`js::DecompileValueGenerator(cx=<unavailable>, spindex=<unavailable>, v=<unavailable>, fallbackArg=<unavailable>, skipStackHits=0) + 2221 at jsopcode.cpp:1842
    frame #3: 0x00000001006fd1dd js-dbg-64-nsprBuild-darwin-eb3a1c0262e4`js::ReportMissingArg(cx=0x0000000101ea5180, v=<unavailable>, arg=<unavailable>) + 221 at jscntxt.cpp:878
    frame #4: 0x000000010078e2a7 js-dbg-64-nsprBuild-darwin-eb3a1c0262e4`js::IteratorConstructor(cx=0x0000000101ea5180, argc=<unavailable>, vp=0x00007fff5fbfd750) + 87 at jsiter.cpp:848
(lldb)

Comment 2

[Johannes Schulte [:jschulte]]

Attachment: WIP.patch

Well, I could only look into this briefly, but the problem seems to be, that the decompiler needs the Object on the stack. Still, I'm not really sure, how my patch caused this and whether the other calls to DoCallNativeGetter also need to keep the stack in sync for the decompiler, so I'll have to look into this a bit more.

Comment 3

[Johannes Schulte [:jschulte]]

Attachment: v1.patch

Apparently, we can't use EmitStowICValues, as GET_GNAME also uses this stub and doesn't pass in a Value/R0. I wasn't able to reproduce the assertion-hit with the other two stubs calling DoCallNativeGetter, is it possible, that they are not affected?

Comment 4

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision ec1351f9bc58).

Comment 5

[Johannes Schulte [:jschulte]]

That's due to bug 1147939 removing the assertion.

Comment 6

[Eric Faust [:efaust]]

Comment on attachment 8592466 [details] [diff] [review]
v1.patch

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

This is certainly right. It should land, and I'm sorry the review lag has been intolerably long.

Comment 7

[Johannes Schulte [:jschulte]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb8de3041fc2

Comment 8

[Johannes Schulte [:jschulte]]

Attachment: v2.patch

So, I think, the problem was, that the old patch reused the tailcallreg. Now, we can't simply take the tailcallreg, as we run out of registers on x86, so I decided to go with this solution, which passes jit-tests locally on both x86 and x64.

Comment 9

[Eric Faust [:efaust]]

Comment on attachment 8621905 [details] [diff] [review]
v2.patch

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

Yep. Well hunted. Thanks for fixing this!

Comment 10

[Johannes Schulte [:jschulte]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f69b97742497

Comment 11

[Johannes Schulte [:jschulte]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01c8dbbf05f7

Comment 12

[Johannes Schulte [:jschulte]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9042dff1c42a

Comment 13

[Johannes Schulte [:jschulte]]

Attachment: v3.patch

Had to correct one minor mistake, hopefully, try will be green this time. Will add checkin-needed if that's the case.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5da7e1b2b6f7

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5da7e1b2b6f7

Comment 16

[Eric Faust [:efaust]]

This is fixed. Clearing ni?