962141 - JS crash spike starting with 2014-01-17 Nightly builds

Status: RESOLVED DUPLICATE of bug 961969

(Core :: JavaScript Engine, defect)

Description

[Robert Kaiser]

Starting with the builds of 2014-01-17, we have a significant increase in JS crashes, with multiple signatures that point in the direction of GC and JIT stuff, I guess we need to analyze more to get to a true cause.

See the bold signatures in https://crash-analysis.mozilla.com/rkaiser/2014-01-20/2014-01-20.firefox.29.explosiveness.html as a good starting point (bold means significant increase, the columns with dates contain crashes with that per million ADI on that day).

Comment 1

[Benjamin Smedberg]

Regression range from Juan: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=324e2cba1029&tochange=9bcc52594322

Comment 2

[Robert Kaiser]

FWIW, exact rooting (bug 753203) was pushed with https://hg.mozilla.org/mozilla-central/rev/6f7227918e79 on Jan 18, so it looks like this crash spike came prior to that.

Comment 3

[Robert Kaiser]

Adding large number of signatures that the data suggests to be related.

Also, bug 961466 might be this and has some narrowing-down based on a reproducible case.

Comment 4

[Robert Kaiser]

Bug 961788 could also be related.

Comment 5

[Benjamin Smedberg]

Candidate commits from JS-land:
	1044db0069e7	Boris Zbarsky — Bug 959927. Make AbstractFramePtr::returnValue return a HandleValue and make receiveCompletionValue take a HandleValue to fix debugger unsafe address hazards. r=terrence
	82ff353b0b27	Boris Zbarsky — Bug 959716. Fix unsafe reference hazards in jsapi-tests. r=terrence
	e040bf77d837	Boris Zbarsky — Bug 959926. Fix some miscellaneous unsafe pointer hazards. r=terrence
	a18cfe2cdc55	Nathan Froyd — Bug 952777 - part 5 - move JSJitInfo::argTypes to a separate JSTypedMethodJitInfo subclass; r=efaust,bz
	6f5cb5e810e5	Nathan Froyd — Bug 952777 - part 4 - move JSParallelNative into the union; r=efaust,bz
	bc45b7c4369b	Nathan Froyd — Bug 952777 - part 3 - use bitfields for integer fields in JSJitInfo; r=efaust,bz
	51d099fe7403	Nathan Froyd — Bug 952777 - part 2 - use explicitly typed enums to shrink JSJitInfo further; r=efaust
	b3d04b01c319	Nathan Froyd — Bug 952777 - part 1 - reorder JSJitInfo slots to pack better; r=efaust,bz
	f017ae03bb6c	Tom Schuster — Bug 939294 - Change xpidl jsval to handles. r=gabor,bz,khuey,bsmedberg,terrence
	cbfac99adeef	Jon Coppeard — Bug 960011 - Fix accidentally added rooting hazard r=me
	45a6f3480c59	Andy Wingo — Bug 960168 - Reified block scopes should prevent magic optimized arguments. r=luke
	bf6cb0c04562	Terrence Cole — Bug 959787 - Handlify several JSAPI interfaces that can GC, Part 3; r=sfink
	8b260c3236da	Terrence Cole — Bug 959787 - Handlify several JSAPI interfaces that can GC, Part 2; r=sfink
	ff4971e84f9f	Terrence Cole — Bug 959787 - Handlify several JSAPI interfaces that can GC, Part 1; r=sfink,Ms2ger
	a15cee5da933	Luke Wagner — Bug 916612 - Move the too-many args+vars checks (r=wingo)
	9845c94f44ff	Luke Wagner — Bug 916612 - Inflate stackDepth width in try notes (r=wingo)
	d2eca1d56402	Luke Wagner — Bug 916612 - Increase maximum number of local variables to 2^28 (r=wingo)
	556ddac71fd8	Luke Wagner — Bug 916612 - Put back the baseline nslots check (r=djvj)

I'm going to follow up and check whether e10s was enabled in any significant fraction of these: there were some other e10s-specific changes in the range, but I didn't include them because I doubt that affects the crash stats to this degree.

Comment 6

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Candidate commits from JS-land:
> 	1044db0069e7	Boris Zbarsky — Bug 959927. Make AbstractFramePtr::returnValue
> return a HandleValue and make receiveCompletionValue take a HandleValue to
> fix debugger unsafe address hazards. r=terrence
> 	82ff353b0b27	Boris Zbarsky — Bug 959716. Fix unsafe reference hazards in
> jsapi-tests. r=terrence
> 	e040bf77d837	Boris Zbarsky — Bug 959926. Fix some miscellaneous unsafe
> pointer hazards. r=terrence
> 	f017ae03bb6c	Tom Schuster — Bug 939294 - Change xpidl jsval to handles.
> r=gabor,bz,khuey,bsmedberg,terrence
> 	cbfac99adeef	Jon Coppeard — Bug 960011 - Fix accidentally added rooting
> hazard r=me
> 	45a6f3480c59	Andy Wingo — Bug 960168 - Reified block scopes should prevent
> magic optimized arguments. r=luke
> 	bf6cb0c04562	Terrence Cole — Bug 959787 - Handlify several JSAPI interfaces
> that can GC, Part 3; r=sfink
> 	8b260c3236da	Terrence Cole — Bug 959787 - Handlify several JSAPI interfaces
> that can GC, Part 2; r=sfink
> 	ff4971e84f9f	Terrence Cole — Bug 959787 - Handlify several JSAPI interfaces
> that can GC, Part 1; r=sfink,Ms2ger
> 	a15cee5da933	Luke Wagner — Bug 916612 - Move the too-many args+vars checks
> (r=wingo)
> 	9845c94f44ff	Luke Wagner — Bug 916612 - Inflate stackDepth width in try
> notes (r=wingo)
> 	d2eca1d56402	Luke Wagner — Bug 916612 - Increase maximum number of local
> variables to 2^28 (r=wingo)
> 	556ddac71fd8	Luke Wagner — Bug 916612 - Put back the baseline nslots check
> (r=djvj)

Removing Nathan's patches because of:

4a9892493aa5	Nathan Froyd — Backout 524be0420e79 and 4c39a7047e96:b3d04b01c319 (bug 952777) …

Comment 7

[Benjamin Smedberg]

->naveed per engineering meeting today

Comment 8

[Naveed Ihsanullah [:naveed]]

Terrence please look into this. The signatures, candidate commits and timing implicate the GGC/Rooting work.

Comment 9

[Luke Wagner [:luke]]

There's also a good chance this is bug 961969 (which can produce random-looking crashes in release builds).

Comment 10

[Robert Kaiser]

(In reply to Luke Wagner [:luke] from comment #9)
> There's also a good chance this is bug 961969 (which can produce
> random-looking crashes in release builds).

If that one was caused by bug 916612 then it's very much possible, as Alice tracked down the a very similar issue in bug 961466 comment #1 to that one as well.
Perhaps it has caused this whole family of crashes I filed this one for - do you think your patch that just went into inbound will fix the whole spike?

Comment 11

[Terrence Cole [:terrence]]

The handlification patches here are very, very unlikely to have caused this bug. I'd wait for results from backing out bug 961969 and fixing the underlying shape issue before investigating that.

Comment 12

[Luke Wagner [:luke]]

(In reply to Robert Kaiser (:[e-mail]) from comment #10)
> Perhaps it has caused this whole family of crashes I filed this one for - do
> you think your patch that just went into inbound will fix the whole spike?

It is already confirmed to have caused crashes on two other sites (see dups in bug), so I believe yes.

Comment 13

[Kyle Sanderson]

Repo case: http://plaguefest.com/threads/ukraine-in-revolt-livestream-vids.17931/

Pretty shocking there's been 5 snapshots and this hasn't been backed out.

Comment 14

[Till Schneidereit [:till]]

(In reply to Kyle Sanderson from comment #13)
> Pretty shocking there's been 5 snapshots and this hasn't been backed out.

Note that a) it took some time to identify what caused this, b) this started on Friday, and yesterday was a holiday in the US, and c) backing out the cause is the second-best option after pushing a fix, which (assuming this really is bug 961969) just happened.

Comment 15

[Kyle Sanderson]

(In reply to Till Schneidereit [:till] from comment #14)
> Note that a) it took some time to identify what caused this, b) this started
> on Friday, and yesterday was a holiday in the US, and c) backing out the
> cause is the second-best option after pushing a fix, which (assuming this
> really is bug 961969) just happened.

Ah, I wasn't trying to be rude or anything. All I was saying is there should be something automated that watches the crash-list. If something suddenly explodes, there's most likely a bug. Everything was filed on the 21st (as far as I can tell); which is what shocked me. It looks like Alice caught it on the 19th, which was still three days after it was pushed.

I probably should have filed on the 17th, which makes me part of the problem. But I digress.

Comment 16

[Robert Kaiser]

(In reply to Kyle Sanderson from comment #15)
> Ah, I wasn't trying to be rude or anything. All I was saying is there should
> be something automated that watches the crash-list. If something suddenly
> explodes, there's most likely a bug. Everything was filed on the 21st (as
> far as I can tell); which is what shocked me. It looks like Alice caught it
> on the 19th, which was still three days after it was pushed.

So, this landed on the 16th, which means the first Nightly with the problem was released on the 17th. We always only look at crash data for full days, so the first day that we would have had data to show the spike was 18th, which was a Saturday, so no paid staff was looking and noticing it. Automated bug reports make no sense because you need more details than "the crash rate is up". The next day that paid staff was at work was the 21st (because the 20th was a US holiday) and so this bug was only filed then.
That said, Alice, who is a volunteer helping us (yay for the Mozilla community!) filed bug bug 961466 on the 19th, and bug 961969, where the patch was done that hopefully fixed this, was filed on the 20th. Maybe I should have elevated one of those bugs into a general one instead of filing this one, but doing it this way looked clear to me, and bugs are cheap (unless one needs to write long explanations like this one).

And all that said, Nightly is not there to be 100% stable all the time, it's there to catch issues just like this before they go to larger audiences.

Comment 17

[u279076]

Not sure if this is the right bug but JSObject::is<js::ArgumentsObject>() has risen to top-crash territory in recent days on Aurora:
* Firefox 28:  #8 @ 1.17% (127 crashes in 7 days)

Comment 18

[Robert Kaiser]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #17)
> Not sure if this is the right bug but JSObject::is<js::ArgumentsObject>()
> has risen to top-crash territory in recent days on Aurora:
> * Firefox 28:  #8 @ 1.17% (127 crashes in 7 days)

Unless Aurora has a wide range of JS signatures spiking wildly, let's consider those two very different issues and file a new bug for the Aurora spike.

Comment 19

[u279076]

(In reply to Robert Kaiser (:[e-mail]) from comment #18)
> Unless Aurora has a wide range of JS signatures spiking wildly, let's
> consider those two very different issues and file a new bug for the Aurora
> spike.

Reported bug 963316.

Comment 20

[Robert Kaiser]

This looks like it's fixed by bug 961969, yes, so I'm marking as a dupe.