Closed Bug 1130768 Opened 5 years ago Closed 5 years ago

Assertion failure: returnAddr > method_->raw(), at BaselineJIT.cpp:647


(Core :: JavaScript Engine: JIT, defect)

Not set



Tracking Status
firefox37 --- fixed
firefox38 --- fixed


(Reporter: jandem, Assigned: jandem)



(1 file)

Testcase below asserts with --baseline-eager (inbound rev 9a3a666c17d7).

var g = newGlobal();
g.parent = this;
g.eval("(" + function() {
    var dbg = new Debugger(parent);
    dbg.onExceptionUnwind = function(frame) {
	frame.onPop = function() { frame.eval("x = 3"); };
} + ")()");
Object.defineProperty(this, "x", {set: [].map});
x = 1;
returnAddr is nullptr and we have an override pc so this could be from bug 1118826. Will look into this more next week.

Gary can you run autoBisect on this? :)
Flags: needinfo?(jdemooij)
Flags: needinfo?(gary)
I can't reproduce this on Mac 64-bit js debug shell:

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

on m-c rev 9a3a666c17d7. Not even on treeherder js shells either.
Flags: needinfo?(gary)
Attached patch PatchSplinter Review
There were two issues:

(1) We were in HandleException, called an onExceptionUnwind hook and triggered DebugModeOSR. DebugModeOSR then set the frame's return address to nullptr, because we will never return to that frame.

Then, while still in HandleException, we called an onPop hook which triggered annother DebugModeOSR and we got confused by the nullptr return address.

The simplest fix for this was to set the is-handling-exception flag for the whole duration of HandleException, so that DebugModeOSR will use the override pc also when we're executing the DebugEpilogue/onPop handler.

(2) After that, there was a problem with CloneOldBaselineStub. When HandleException unwinds scopes, it can modify the frame's override pc. This however means that if we have a stub frame on the stack, the stub frame's fallbackStub will no longer match this (modified) pc, so CloneOldBaselineStub asserted.

The patch sets the stub frame's stub pointer to nullptr in this case. I think that's fine as HandleException will never return to this stub frame.

Let me know if this all makes sense. The patch could use a careful review :)
Assignee: nobody → jdemooij
Flags: needinfo?(jdemooij)
Attachment #8562179 - Flags: review?(shu)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
> I can't reproduce this on Mac 64-bit js debug shell:

Thanks for looking into this :) It reproduced here on OS X with both 32-bit and 64-bit shells, debug and debug+opt, with and without --baseline-eager.
Comment on attachment 8562179 [details] [diff] [review]

Review of attachment 8562179 [details] [diff] [review]:

Thanks a lot for the fix! I missed this bug when it was first filed. The explanation and the high-level fix are right.

To be extra safe, could you assert !frame_->isHandlingException() in DebugModeOSRVolatileStub::invalid?

I'd like the code reorganized a bit. Specific comments inline below. r=me with those addressed.

::: js/src/jit/BaselineDebugModeOSR.cpp
@@ +220,5 @@
> +                // the entry's pcOffset we just set doesn't necessarily match
> +                // the stub that's still on the stack. To prevent that, we just
> +                // null prevFrameStubPtr, as the exception handler will never
> +                // return to this stub frame anyway.
> +                prevFrameStubPtr = nullptr;

I think it would be cleaner to instead patch CloneOldBaselineStub to set |entry.newStub = nullptr| and return early when |entry.frameKind == ICEntry::Kind_Invalid|. We shouldn't have invalid ICEntries with stub pointers other than via exception handling.

The comment is nice though and should accompany the code in CloneOldBaselineStub instead.

@@ +589,5 @@
> +                MOZ_ASSERT(!entry.oldStub);
> +                MOZ_ASSERT(!entry.newStub);
> +                layout->setStubPtr(nullptr);
> +                break;
> +            }

Along with the change above, this block would go away as well. Instead, the MOZ_ASSERT(entry.newStub) below would be changed to MOZ_ASSERT(entry.newStub || prevFrame->isHandlingException()).
Attachment #8562179 - Flags: review?(shu) → review+
Comment on attachment 8562179 [details] [diff] [review]

Review of attachment 8562179 [details] [diff] [review]:

Actually, wait. I'm debugging this myself right now and I'm not seeing the call to onPop causing the assert, but a second call to onExceptionUnwind.
Attachment #8562179 - Flags: review+
Comment on attachment 8562179 [details] [diff] [review]

Review of attachment 8562179 [details] [diff] [review]:

I'm dumb. Forgot it was --baseline-eager. r+ stands.
Attachment #8562179 - Flags: review+
Good suggestions. Pushed with those addressed:

NI myself to request uplift after this has been in a few nightlies.
Flags: needinfo?(jdemooij)
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8562179 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: Maybe bug 1118826, not sure.
[User impact if declined]: Crashes or other problems when using the debugger.
[Describe test coverage new/current, TreeHerder]: Patch adds a test and has been in a few nightlies.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.
Flags: needinfo?(jdemooij)
Attachment #8562179 - Flags: approval-mozilla-aurora?
Given that you understand the problems quite well (comment 3), can you verify the fix in the latest Nightly before we uplift to 37?
Flags: needinfo?(jdemooij)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #11)
> Given that you understand the problems quite well (comment 3), can you
> verify the fix in the latest Nightly before we uplift to 37?

I just verified the test in comment 0 no longer asserts (the patch also adds this test to our test suite, FWIW).
Flags: needinfo?(jdemooij)
Comment on attachment 8562179 [details] [diff] [review]

Thank you for verifying. Aurora+
Attachment #8562179 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.