694200 - Crash [@ js::mjit::ic::BaseIC::disable]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Test case for shell (see README file inside).

The attached test crashes on mozilla-central revision 866b2b1793cd (see README for running instructions).

Valgrind trace:

==15832== Invalid read of size 8
==15832==    at 0x7846B5: js::mjit::ic::BaseIC::disable(JSContext*, char const*, void*) (PolyIC.cpp:2330)
==15832==    by 0x78CD95: PICStubCompiler::disable(JSContext*, char const*) (PolyIC.cpp:184)
==15832==    by 0x78CD5E: PICStubCompiler::disable(char const*) (PolyIC.cpp:180)
==15832==    by 0x783694: js::mjit::ic::GetProp(js::VMFrame&, js::mjit::ic::PICInfo*) (PolyIC.cpp:1992)
==15832==    by 0x6E7C75: ??? (MethodJIT.cpp:164)
==15832==    by 0x404FB4F: ???
==15832==  Address 0x40 is not stack'd, malloc'd or (recently) free'd


S-s because crashes with this signature have been previously critical.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

On error paths, the IC code sometimes tried to disable the IC before returning an error.  This is a problem when the error was generated by a VM operation, as the error is checked before recompilation.  Patch does a sweep and removes all places where ICs are disabled on error paths.

Comment 2

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3783a31eda47

Comment 3

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/mozilla-central/rev/3783a31eda47

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Brian, is bug 695875 revolving around something similar to this bug?

Comment 5

[Brian Hackett [Laid off!]]

(In reply to Naoki Hirata :nhirata from comment #4)
> Brian, is bug 695875 revolving around something similar to this bug?

Possibly, yeah.  The crash in that bug predates the fix here, so it would be good to see if that crash can be reproduced on a current nightly.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 566829 [details] [diff] [review]
patch

Approved per today's triage meeting.

Comment 8

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b9725bcebd71

Comment 10

[Johnny Stenback (:jst)]

Do we need this for 8?

Comment 11

[Brian Hackett [Laid off!]]

No, this is a TI bug.

Comment 12

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Brian, is there another way to verify that the issue has been solved as running the testcase through js? I have problems to compile Firefox on OS X at least with the given changeset from comment 0.

Comment 13

[Christian Holler (:decoder)]

Confirmed fixed on trunk, aurora and beta.

@Henrik: You don't need to compile Firefox to verify tests like this, a JS shell suffices (which is much easier). If you need help on how to compile these, let me know (just ping me on IRC or email) :)

Comment 14

[u279076]

[qa-] since QA didn't verify this fix. Marking it VERIFIED based on comment 13.

Thanks Christian

Comment 15

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

(In reply to Christian Holler (:decoder) from comment #13)
> @Henrik: You don't need to compile Firefox to verify tests like this, a JS
> shell suffices (which is much easier). If you need help on how to compile
> these, let me know (just ping me on IRC or email) :)

You came me before now. Yesterday I have already seen this by checking the tinderbox folders on FTP. All the builds up there have the jsshell included. But thanks for the verification.