791589 - IonMonkey: BraidJump crash: visitApplyArgsGeneric() doesn't use ImmGCPtr for ArgumentsRectifier

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[[e-mail] (:bhearsum)]

Twice today I was playing the game in the linked URL and Firefox crashed. I've played this game tons of times in the past 6 months and never had an issue. Strangely, my crash signatures are very different:
https://crash-stats.mozilla.com/report/index/bp-49ebfb52-0b35-4efc-842f-b731e2120916
https://crash-stats.mozilla.com/report/index/bp-dd70542a-9121-4f4b-818c-cda722120916

I was running a build of http://hg.mozilla.org/mozilla-central/rev/f7c89de3ab43 at the time.

Filed under JS because one of the crashes is in JS, and IonMonkey very recently landed on Nightly.

Comment 1

[Scoobidiver (away)]

There is only one crash of each signature.
Do you have reliable STR? In that case, does it happen in Safe Mode?

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Ben Hearsum [:bhearsum] from comment #0)
> Twice today I was playing the game in the linked URL and Firefox crashed.
> I've played this game tons of times in the past 6 months and never had an
> issue.
> 
> […]
> 
> Filed under JS because one of the crashes is in JS, and IonMonkey very
> recently landed on Nightly.

I would be interested if you can get a bigger backtrace (with the DOM crash) by going into about:config and enabling "profiler.enabled", this would slow down a bit the JS engine but should provide some mechanism which hopefully should help the crash reporter to provide a better backtrace.  If you don't want to get many slow down caused by the sampling profiler, you can increase the period of the sampling in "profiler.interval".

Comment 3

[[e-mail] (:bhearsum)]

I'll see if I can repro with the profiler on. Anything special I have to do to get the stack afterwards?

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Ben Hearsum [:bhearsum] from comment #3)
> I'll see if I can repro with the profiler on. Anything special I have to do
> to get the stack afterwards?

No, hopefully, the crash report should be able to walk the stack and follow the IonMonkey stack frames.  I don't think we have any instrumentation to use SPS stack to provide useful JS locations yet, but we should look into it if this works.

Comment 5

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

Ben, how easy is it for you to reproduce this? (I'm trying to crash the game on my Mac but it seems to work.)

It might be worth re-testing after bug 790464 lands.

Comment 6

[[e-mail] (:bhearsum)]

I can repro nearly at will. It takes about 30s to 2min on my Linux machine to crash. I just crashed with the profiler enabled, in fact: https://crash-stats.mozilla.com/report/index/36b21922-9e2c-4a18-9a8d-21e1d2120917

Comment 7

[Sean Stangl [:sstangl]]

I can reproduce this with the latest nightly.

Comment 8

[Sean Stangl [:sstangl]]

Appears to be fixed by Bug 779411 -- with its patch applied, the game runs fine locally. Leaving unresolved so Ben can verify after Bug 779411 is fixed.

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Ben Hearsum [:bhearsum] from comment #6)
> I can repro nearly at will. It takes about 30s to 2min on my Linux machine
> to crash. I just crashed with the profiler enabled, in fact:
> https://crash-stats.mozilla.com/report/index/36b21922-9e2c-4a18-9a8d-
> 21e1d2120917

Thanks for testing with the profiler, it did not work as I expected. You can restore the default parameters. I haven't been able to reproduce it too.

Comment 10

[[e-mail] (:bhearsum)]

I'm still crashing even thought bug 790464/779411 are fixed. https://crash-stats.mozilla.com/report/index/bp-540393f9-6c70-40ee-ad75-dca412120919 is my most recent one, with a useless stack =\.

Comment 11

[Sean Stangl [:sstangl]]

I'm not able to reproduce it locally. Are there any STR other than playing the game for a while?

Comment 12

[Sean Stangl [:sstangl]]

Reproduced with a debug build! We're in business.

Comment 13

[Sean Stangl [:sstangl]]

Attachment: Mark ArgumentsRectifier in visitApplyArgsGeneric().

The ArgumentsRectifier is being GC'd because visitApplyArgsGeneric() uses an ImmWord() to move the address, not a markable ImmGCPtr(). We then run off into the weeds and execute a ton of bogus random instructions, then maybe crash. This is actually s-s, but it's fixed now.

Giving review to Waldo because the fix is simple, other people are suspiciously absent, and I want this in-tree asap.

Comment 14

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 662712 [details] [diff] [review]
Mark ArgumentsRectifier in visitApplyArgsGeneric().

I assume that since this is in the argumentsrectifier, even though we're doing more memory access, it has 0 impact on perf.

Comment 15

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99a42cef003f

Thanks, Marty and Ben. Yeah, this will have no effect on performance. Verified in-browser that braidjump runs with a debug build without dying with this patch applied. There might be other bugs that aren't cropping up for me, but if there are, we can open separate bugs for them.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/99a42cef003f