1120677 - Assertion failure: retAddr != nullptr, at jit/JitFrames.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

// Randomly chosen test: js/src/jit-test/tests/debug/bug1001372.js
var g = newGlobal();
g.parent = this;
g.eval('Debugger(parent).onExceptionUnwind = function() {};');
// Randomly chosen test: js/src/jit-test/tests/basic/bug792239.js
// -- reduced away --
// Randomly chosen test: js/src/jit-test/tests/baseline/bug847446.js
function h() {
    yield;
}
function f() {
    for (var i in h()) {
        f();
    }
}
f();

asserts js debug shell on m-c changeset cac64af410a1 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: retAddr != nullptr, at jit/JitFrames.cpp.

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

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20150110105930" and the hash "a55f093defe1".
The "bad" changeset has the timestamp "20150110111030" and the hash "c213d9d53886".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a55f093defe1&tochange=c213d9d53886

Setting s-s to be safe as this is probably related to profiling judging from the assertion name, and setting needinfo? from Jan. Jan, is bug 1118826 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x7b8da, 0x0000000100382b39 js-dbg-opt-64-dm-nsprBuild-darwin-cac64af410a1`js::jit::GetPcScript(cx=<unavailable>, scriptRes=<unavailable>, pcRes=<unavailable>) + 1529 at JitFrames.cpp:1484, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100382b39 js-dbg-opt-64-dm-nsprBuild-darwin-cac64af410a1`js::jit::GetPcScript(cx=<unavailable>, scriptRes=<unavailable>, pcRes=<unavailable>) + 1529 at JitFrames.cpp:1484
    frame #1: 0x0000000100534583 js-dbg-opt-64-dm-nsprBuild-darwin-cac64af410a1`JSContext::findVersion() const [inlined] JSContext::currentScript(this=<unavailable>, ppc=<unavailable>, allowCrossCompartment=<unavailable>) const + 85 at jscntxtinlines.h:463
    frame #2: 0x000000010053452e js-dbg-opt-64-dm-nsprBuild-darwin-cac64af410a1`JSContext::findVersion(this=0x0000000101f03c60) const + 14 at jscntxt.cpp:1212
    frame #3: 0x00000001005419c8 js-dbg-opt-64-dm-nsprBuild-darwin-cac64af410a1`JS::CompileOptions::CompileOptions(this=0x00007fff5f9ffaa8, cx=0x0000000101f03c60, version=<unavailable>) + 280 at jsapi.cpp:3977
    frame #4: 0x0000000100641ce4 js-dbg-opt-64-dm-nsprBuild-darwin-cac64af410a1`js::CloneScript(cx=0x0000000101f03c60, newKind=<unavailable>, enclosingScope=<unavailable>, fun=<unavailable>, src=<unavailable>) + 4052 at jsscript.cpp:3080
(lldb)

Comment 2

[Daniel Veditz [:dveditz]]

if this is a null deref does it need to be a s-s bug?

Comment 3

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

I don't think so, moreover it's debugger-related so it probably isn't that severe. Was just being cautious. Will just leave it for Jan to open up.

Comment 4

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

This was found by combining random js tests together with jsfunfuzz, the specific file(s) is/are:

http://hg.mozilla.org/mozilla-central/file/cac64af410a1/js/src/jit-test/tests/debug/bug1001372.js
http://hg.mozilla.org/mozilla-central/file/cac64af410a1/js/src/jit-test/tests/basic/bug792239.js
http://hg.mozilla.org/mozilla-central/file/cac64af410a1/js/src/jit-test/tests/baseline/bug847446.js

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch

This patch fixes GetPcScript to not use the PcScriptCache for BaselineFrames with an override pc.

I had to rewrite GetPcScript to use it.type() instead of it.prevType(). Although this may require an extra ++it, that shouldn't really matter and I think it's more readable and more similar to what we use elsewhere.

We can simplify GetPcScript a bit more by allocating PcScriptCache eagerly, so that we don't need to check it's non-null, but I want to backport this patch so I'll do that separately.

Comment 6

[Shu-yu Guo [:shu]]

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

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

Looks sensible enough to me.

::: js/src/jit/JitFrames.cpp
@@ +1455,5 @@
>  GetPcScript(JSContext *cx, JSScript **scriptRes, jsbytecode **pcRes)
>  {
>      JitSpew(JitSpew_IonSnapshots, "Recover PC & Script from the last frame.");
>  
> +    // Recover the return address.

I would like to expand this comment and explain that whole purpose of recovering the return address is to lookup in a retaddr->pc cache, as pc computation is expensive. But if we have an override pc, which is cheap to get, we definitely don't need the return address. Moreover, sometimes when an override pc is present during exception handling, the return address is nullptr as a sanity check since we do not return to the frame that threw the exception.

@@ +1459,5 @@
> +    // Recover the return address.
> +    JitFrameIterator it(cx);
> +    uint8_t *retAddr;
> +    if (it.type() == JitFrame_Exit) {
> +        ++it;

I agree, I think avoiding using prevType() makes this function easier to read and reason about anyhow.

Comment 7

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/491f9b6be0ee

Comment 8

[Jan de Mooij [:jandem]]

Opening up because it's not security sensitive, but we should backport it to 37.

Comment 9

[Jan de Mooij [:jandem]]

NI myself to request approval tomorrow.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/491f9b6be0ee

Comment 11

[Jan de Mooij [:jandem]]

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

Approval Request Comment
[Feature/regressing bug #]: Bug 1118826 part 1 (landed in 37 right before the merge).
[User impact if declined]: Maybe crashes or bugs in some cases. I'm not sure and it's a simple patch so better safe than sorry.
[Describe test coverage new/current, TreeHerder]: Landed on m-c yesterday, fixes reported testcase.
[Risks and why]: Low risk.
[String/UUID change made/needed]: No.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9aed7f7c9de8