805299 - IonMonkey: Crash [@ 0xdeadbeee]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Testcase for shell

The attached testcase crashes on mozilla-central revision 1c3e4cb1f754 (run with --ion-eager).

Comment 1

[Christian Holler (:decoder)]

Crash trace:

Program received signal SIGSEGV, Segmentation fault.
0xdeadbeee in ?? ()
(gdb) bt
#0  0xdeadbeee in ?? ()
#1  0x0047f4e4 in js::ion::AutoFlushCache::~AutoFlushCache (this=0x40030788, __in_chrg=<value optimized out>) at /home/decoder/mozilla-central/js/src/ion/arm/Assembler-arm.cpp:2474
#2  0x00d8d690 in ?? ()
Cannot access memory at address 0x0
#3  0x00d8d690 in ?? ()
Cannot access memory at address 0x0
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

Comment 2

[Robert Kaiser]

What's the status here? This is marked tracking# for 18, which is entering beta this week.

Comment 3

[Marty Rosenberg [:mjrosenb]]

A quick patch would be to just abort compilation when we hit this case.  This case should basically never come up.  The issue has to do with pools, and is a bad fix for bug 759464.
I'm currently trying to cook up an actual reasonable way to handle this case.

Comment 4

[Marty Rosenberg [:mjrosenb]]

Attachment: /home/mrosenberg/patches/disable-r0.patch

This patch causes compilation to abort when we detect a case that we were previously handling incorrectly.  There were a few cases where assertions were then triggering due to the world not looking quite right, so explicit checks needed to be added.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 685092 [details] [diff] [review]
/home/mrosenberg/patches/disable-r0.patch

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

::: js/src/ion/IonMacroAssembler.h
@@ +499,5 @@
>      }
>  
>      void link(IonCode *code) {
> +        if (m_buffer.oom())
> +            return;

Better to make this JS_ASSERT(!m_buffer.oom()), and then check for OOM where needed in ion::Linker::newCode().

::: js/src/ion/RangeAnalysis.cpp
@@ +532,5 @@
>                      return false;
>              }
>          }
>          iters++;
> +        if (iters >= numBlocks * 1000)

Part of this patch?

Comment 6

[Robert Kaiser]

Marty, are you planning to land this reviewed patch?
Also, please nominate for beta and aurora approval once it sticks on trunk.

Comment 7

[Marty Rosenberg [:mjrosenb]]

landed on mi: https://hg.mozilla.org/integration/mozilla-inbound/rev/162e2608bd3c

Comment 8

[(no longer active)]

Backed out because this breaks debug builds:

https://hg.mozilla.org/integration/mozilla-inbound/rev/97b197902642

Comment 9

[Marty Rosenberg [:mjrosenb]]

My bad.  tbpl logs point right at the culprit: I directly reference m_buffer in shared code inside of JS_ASSERT, so it will crash only in debug builds on all non-arm platforms.

Comment 10

[Marty Rosenberg [:mjrosenb]]

re-landed, more green: https://hg.mozilla.org/integration/mozilla-inbound/rev/a592d7dfb470

Comment 11

[Ed Morley [:emorley]]

Backed out for crashes on Android:
https://tbpl.mozilla.org/php/getParsedLog.php?id=17551768&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/3559bc4dfe1b

Comment 12

[Alex Keybl [:akeybl]]

Marty - can you continue this investigation? This is an sg:crit regression from FF17 and we're going to build with b4 Tuesday. Would be really great to get a fix uplifted before then.

Comment 13

[Marty Rosenberg [:mjrosenb]]

yeah, I'm working on it, and fighting with mochitests, as usual.

Comment 14

[Marty Rosenberg [:mjrosenb]]

landed again:
http://hg.mozilla.org/mozilla-central/rev/21aeb525c630
This time with a try run first.

Comment 15

[Marty Rosenberg [:mjrosenb]]

err: http://hg.mozilla.org/integration/mozilla-inbound/rev/21aeb525c630
my script occasionally generates bogus urls.

Comment 16

[Marty Rosenberg [:mjrosenb]]

Attachment: patch that was actually landed on m-i

[Approval Request Comment]
Bug caused by (feature/regressing bug #): IM landing + 763333
User impact if declined: mysterious crashes, possibly incorrect behavior
Testing completed (on m-c, etc.): on m-i
Risk to taking this patch (and alternatives if risky): 
This patch is the safe approach. I'm just nuking the code paths that are trick/were questionable, and causing us to not compile the code with IonMonkey.  Sadly, it uses the OOM mechanism, and that isn't quite perfect.  If there are paths where OOM conditions need to be checked, and they aren't we may throw an assertion.
String or UUID changes made by this patch:

Comment 17

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/21aeb525c630

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 690055 [details] [diff] [review]
patch that was actually landed on m-i

Low risk enough that we can take this for beta 4 which goes to build tomorrow, please go ahead with branch uplift.

Comment 19

[Marty Rosenberg [:mjrosenb]]

landed on both aurora and beta:
https://hg.mozilla.org/releases/mozilla-aurora/rev/3a41271d7364
https://hg.mozilla.org/releases/mozilla-beta/rev/4946d63b2dec

Comment 20

[Robert Kaiser]

(In reply to Marty Rosenberg [:mjrosenb] from comment #19)
> landed on both aurora and beta

Setting flags accordingly.

Comment 21

[u279076]

Can this be put in testsuite?