917401 - crash in js::ObjectImpl::getClass() with profiler enabled

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[(not currently active) Ted Mielczarek]

This bug was filed from the Socorro interface and is 
report bp-427d732f-f52a-4de5-bd02-4995f2130917.
=============================================================

The google groups post in the URL field reliably crashes my Firefox Nightly on Windows if I have the Profiler enabled. Disabling the profiler makes it go away. I tested this on a clean profile, where it didn't crash, but then installing the profiler addon and reloading caused the crash.

Same crash:
https://crash-stats.mozilla.com/report/index/7bb1ca6c-43d3-40de-b909-b1e722130917
https://crash-stats.mozilla.com/report/index/394b7311-52fe-4474-bf76-be22f2130917

Comment 1

[Alice0775 White]

Crash with built-in profiler.
bp-152f7f01-e765-4d9e-beeb-829fc2130917

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/c38b60b9063e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130910161759
Bad:
http://hg.mozilla.org/mozilla-central/rev/f73bed2856a8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130910172959
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c38b60b9063e&tochange=f73bed2856a8


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0452b5b504d0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130910032034
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/19918a47a06f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130910044250
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0452b5b504d0&tochange=19918a47a06f
Regressed by:
255093e2f430	Shu-yu Guo — Bug 914478 - Fix checking for error in setElemTryCache. (r=jandem)

Comment 2

[Shu-yu Guo [:shu]]

Can anyone else reproduce this on a platform other than Windows?

Comment 3

[Alice0775 White]

Crash with built-in profiler on ubuntu12.04 32bit.

bp-f710ff91-2fe9-4a12-b830-93b0b2130917

http://hg.mozilla.org/mozilla-central/rev/dc909122bcf5
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130916030201

Comment 4

[Shu-yu Guo [:shu]]

(In reply to Alice0775 White from comment #3)
> Crash with built-in profiler on ubuntu12.04 32bit.
> 
> bp-f710ff91-2fe9-4a12-b830-93b0b2130917
> 
> http://hg.mozilla.org/mozilla-central/rev/dc909122bcf5
> Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0
> ID:20130916030201

Ah, 32bit eh? Thanks!

Comment 5

[Jan de Mooij [:jandem]]

Hm this is probably caused by having only 6 registers available on x86 when the profiler is enabled..

Comment 6

[Kannan Vijayan [:djvj]]

This is likely to have been introduced by my push to bug 909764, since shu's patch was a fix for a regression introduced by the patch for that bug.

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> Hm this is probably caused by having only 6 registers available on x86 when
> the profiler is enabled..

SetElementCache only allocates 6 on x86 though, I'm waiting on a 32bit build to investigate.

Comment 8

[Shu-yu Guo [:shu]]

Attachment: bug917401-regalloc.patch

Okay, this is a standing bug in Ion's linear scan regalloc in assigning incorrect registers under high register pressure + fixed registers. It's surprising this hasn't surfaced before. In this case, x86 constrains registers, the profiler constrains registers some more by taking away %ebp, and SetElementIC uses 6 registers, and so makes the perfect storm.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 806343 [details] [diff] [review]
bug917401-regalloc.patch

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

Ah good catch!

Comment 10

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/83cea421fa5e

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/83cea421fa5e

Comment 12

[Tracy Walker [:tracy]]

Ted or Alice, please verify, if you could.

Comment 13

[Alice0775 White]

Still crash with built-in profiler
bp-e4888cf8-1317-4c2c-8b48-81ddb2130920
http://hg.mozilla.org/releases/mozilla-aurora/rev/46b216260c1d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130920004001

No crash with built-in profiler
http://hg.mozilla.org/mozilla-central/rev/8f8a683dfc42
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130920030204

Comment 14

[Shu-yu Guo [:shu]]

Looking, but this was fixed locally on 32bit Linux for me.

Comment 15

[Shu-yu Guo [:shu]]

Wait, actually, what does your comment mean, Alice, if 
http://hg.mozilla.org/mozilla-central/rev/8f8a683dfc42 doesn't crash anymore, and that's a more recent revision, isn't this fixed?

Comment 16

[Alice0775 White]

(In reply to Shu-yu Guo [:shu] from comment #15)
> Wait, actually, what does your comment mean, Alice, if 
> http://hg.mozilla.org/mozilla-central/rev/8f8a683dfc42 doesn't crash
> anymore, and that's a more recent revision, isn't this fixed?

Still crash in Aurora26.a2 cset 46b216260c1d.
Not crash in Nightly27.0a1 cset 8f8a683dfc42.

Comment 17

[Shu-yu Guo [:shu]]

(In reply to Alice0775 White from comment #16)
> (In reply to Shu-yu Guo [:shu] from comment #15)
> > Wait, actually, what does your comment mean, Alice, if 
> > http://hg.mozilla.org/mozilla-central/rev/8f8a683dfc42 doesn't crash
> > anymore, and that's a more recent revision, isn't this fixed?
> 
> Still crash in Aurora26.a2 cset 46b216260c1d.
> Not crash in Nightly27.0a1 cset 8f8a683dfc42.

Ah I misunderstood the formatting, thanks for the clarification.

Comment 18

[Kyle Wooten]

Thanks!

Comment 19

[Shu-yu Guo [:shu]]

Comment on attachment 806343 [details] [diff] [review]
bug917401-regalloc.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not sure, probably when the LSRA was added
User impact if declined: 
Testing completed (on m-c, etc.): Overwriting registers with JIT + profiler on, probably exploitable
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch:

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/331b31231aa1

Comment 21

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 26.0a2 (2013-10-16), 27.0a1 (2013-10-16) Win 7