815542 - crash in IncrementalCollectSlice

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Scoobidiver (away)]

It's #2 top browser crasher in 20.0a1 on Linux.
It started spiking around 19.0a1/20121117. The regression range for the spike might be:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=58ebb638a7ea&tochange=9a6d708faf3f

Signature 	IncrementalCollectSlice More Reports Search
UUID	dc45ff0c-32fc-4517-b219-2ae202121126
Date Processed	2012-11-26 22:38:35
Uptime	1889
Last Crash	35.4 minutes before submission
Install Age	31.5 minutes since version was first installed.
Install Time	2012-11-26 22:06:43
Product	Firefox
Version	20.0a1
Build ID	20121126030823
Release Channel	nightly
OS	Linux
OS Version	0.0.0 Linux 3.5.2 #1 SMP Wed Aug 15 17:35:07 EDT 2012 i686
Build Architecture	x86
Build Architecture Info	AuthenticAM family 15 model 75 stepping 2
Crash Reason	SIGSEGV
Crash Address	0x2c9
App Notes 	
OpenGL: VMware, Inc. -- Gallium 0.4 on llvmpipe (LLVM 0x209) -- 2.1 Mesa 8.0.4 -- texture_from_pixmap
EMCheckCompatibility	True

Frame 	Module 	Signature 	Source
0 	libxul.so 	IncrementalCollectSlice 	js/src/jsgc.cpp:3429
1 	libxul.so 	GCCycle 	js/src/jsgc.cpp:4558
2 	libxul.so 	Collect 	js/src/jsgc.cpp:4673
3 	libxul.so 	js::IncrementalGC 	js/src/jsfriendapi.cpp:190
4 	libxul.so 	nsJSContext::GarbageCollectNow 	dom/base/nsJSEnvironment.cpp:2948
5 	libxul.so 	GCTimerFired 	dom/base/nsJSEnvironment.cpp:3210
6 	libxul.so 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:482
7 	libxul.so 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:565
8 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:627
9 	libxul.so 	NS_ProcessNextEvent_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:221
10 	libxul.so 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:117
11 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:215
12 	libxul.so 	nsBaseAppShell::Run 	widget/xpwidgets/nsBaseAppShell.cpp:163
13 	libxul.so 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:290
14 	libxul.so 	XREMain::XRE_mainRun 	toolkit/xre/nsAppRunner.cpp:3823
15 	libxul.so 	XREMain::XRE_main 	toolkit/xre/nsAppRunner.cpp:3890
16 	libxul.so 	XRE_main 	toolkit/xre/nsAppRunner.cpp:4084
17 	firefox 	main 	browser/app/nsBrowserApp.cpp:174

More reports at:
https://crash-stats.mozilla.com/report/list?signature=IncrementalCollectSlice

Comment 1

[Daniel Veditz [:dveditz]]

CC'ing billm and jonco who touched this code the most over the summer when this is likely to have regressed.

Worries me that crashing in GC could be a security problem. Many of the crashes are near(ish) null like this one and worry me somewhat less. For example, the crash on this line occurs a lot and is either near 0x541 in 64-bit builds or 0x2c9 in x86 or arm builds (some minor variation by build date). "dst->compartment()->maybeAlive = true;"

Some of the Firefox 17.0 crashes on Windows look scarier, but maybe it's a separate GC problem: bp-d6f96ab7-c82c-4343-b021-f3b0a2121124. and few in Linux look like they've jumped off into nowhere as well.

Comment 2

[Andrew McCreight [:mccr8]]

Bug 812393 is in that range.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

There seem to be a few problems grouped in the same signature. The spike is from a NULL pointer deref in which a cross-compartment wrapper is wrapping NULL.

Comment 4

[Scoobidiver (away)]

One comment says Firefox crashed after accessing settings of LastPass 2.0.

Comment 5

[Alex Keybl [:akeybl]]

Tracking for FF20 since it's a top crash there, but this isn't yet a major user issue in FF19.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Bill McCloskey (:billm) from comment #3)
> The spike is
> from a NULL pointer deref in which a cross-compartment wrapper is wrapping
> NULL.

Sounds like that might same issue as bug 816054 then.

Comment 7

[Scoobidiver (away)]

(In reply to Alex Keybl [:akeybl] from comment #5)
> Tracking for FF20 since it's a top crash there, but this isn't yet a major
> user issue in FF19.
It's #1 top browser crasher in 19.0a2 on Linux.

Comment 8

[Andrew McCreight [:mccr8]]

These are mostly crashes on a small-ish address (0x2cd) here: http://hg.mozilla.org/releases/mozilla-aurora/annotate/32dba69af0fa/js/src/jsgc.cpp#l3457

This code was introduced by Bill's TRANSPLANT gc patch, bug 803376.

Comment 9

[Andrew McCreight [:mccr8]]

That line is:
  dst->compartment()->maybeAlive = true;

0x2cd is 717, and JSCompartment is a pretty huge structure with maybeAlive fairly far along it, so my guess would be that given the consistency in that particular address that compartment() is NULL, which sounds similar to comment 3.

Comment 10

[u279076]

Dropping qawanted as per yesterdays Channel Meeting discussion. Given level of engagement by Engineering there's no immediate QA need here. Please re-add QAWANTED if there's something you'd like QA to follow-up on.

Comment 11

[Alex Keybl [:akeybl]]

KaiRo - should we consider uplifting bug 816054 to FF19? Looks like this isn't a very prevalent crash in FF20/21, which both have that fix.

Comment 12

[Robert Kaiser]

(In reply to Alex Keybl [:akeybl] from comment #11)
> KaiRo - should we consider uplifting bug 816054 to FF19?

If :billm and/or :jonco think it fixes this problem, I'm all for it, but I have no clue as to this could be true or not.

Comment 13

[Jon Coppeard (:jonco)]

(In reply to Robert Kaiser (:[e-mail]) from comment #12)

The patch for bug 816054 doesn't actually fix any problems, it simply improves the assertions to make the problem apparent sooner and make it easier to debug.

Whatever caused this must have got fixed by something else I think.

Comment 14

[Alex Keybl [:akeybl]]

(In reply to Jon Coppeard (:jonco) from comment #13)
> (In reply to Robert Kaiser (:[e-mail]) from comment #12)
> 
> The patch for bug 816054 doesn't actually fix any problems, it simply
> improves the assertions to make the problem apparent sooner and make it
> easier to debug.
> 
> Whatever caused this must have got fixed by something else I think.

How are you planning on determining that? This is a high volume Linux top crasher, and beta 3 is going to build today. We need your help.

Comment 15

[Jon Coppeard (:jonco)]

Attachment: Possible fix

Here's a fix for an issue introduced by 812393, which is possibly the cause of these crashes.  It's a bit difficult to tell for sure because I haven't been able to reproduce the issue.

The problem is that XPCStringConvert::ReadableToJSVal keeps a cached JSString and it's possible for it to hand out a pointer to this after sweeping has started but before the string is finalized, resulting in a pointer to garbage.

If this is in fact the root of this issue, then the reason the crash occurs more infrequently in 20 onwards is because bug 790338 greatly reduced the window of opportunity for this to happen, because sweeping now happens in small groups of compartments rather than all compartments at once.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 705971 [details] [diff] [review]
Possible fix

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

Thanks. Sorry you had to track this down; looks like it was all my fault.

::: js/xpconnect/src/XPCString.cpp
@@ -35,5 @@
>      nsStringBuffer* buf = nsStringBuffer::FromData(chars);
> -    if (buf == sCachedBuffer) {
> -        sCachedBuffer = nullptr;
> -        // No need to clear sCachedString
> -    }

Maybe assert here that buf != sCachedBuffer.

Comment 17

[Jon Coppeard (:jonco)]

(In reply to Bill McCloskey (:billm) from comment #16)

No worries :)

> ::: js/xpconnect/src/XPCString.cpp
> @@ -35,5 @@
> >      nsStringBuffer* buf = nsStringBuffer::FromData(chars);
> > -    if (buf == sCachedBuffer) {
> > -        sCachedBuffer = nullptr;
> > -        // No need to clear sCachedString
> > -    }
> 
> Maybe assert here that buf != sCachedBuffer.

I tried this but actually I think it is valid for another string with the same nsStringBuffer to be created and cached after sweeping starts but before the original is finalized.  Really I think we want to assert that the string being finalized isn't cached, but the string pointer is not passed to the finalizer.

Comment 18

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd471ce8fd8e

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cd471ce8fd8e

Comment 20

[Alex Keybl [:akeybl]]

(In reply to Jon Coppeard (:jonco) from comment #18)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/cd471ce8fd8e

Jon - can you nominate for uplift to aurora/beta no later than today with a risk evaluation?

Comment 21

[Jon Coppeard (:jonco)]

Comment on attachment 705971 [details] [diff] [review]
Possible fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 812393
User impact if declined: Occasional crashes
Testing completed (on m-c, etc.): This patch has been on m-c for a week
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

Comment 22

[Alex Keybl [:akeybl]]

(In reply to Jon Coppeard (:jonco) from comment #21)
> Risk to taking this patch (and alternatives if risky): Low

Do you have any ideas as to where regressions would rear their head if found? Would it be another stability issue?

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 705971 [details] [diff] [review]
Possible fix

Approving for Aurora in preparation for uplift to Beta.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e1d996af862

Comment 25

[Jon Coppeard (:jonco)]

(In reply to Alex Keybl [:akeybl] from comment #22)
> Do you have any ideas as to where regressions would rear their head if
> found? Would it be another stability issue?

It's always possible that a similar issue would occur, I think the patch is pretty safe though.

Comment 26

[Alex Keybl [:akeybl]]

Spoke with jonco - we plan to backout bug 812393 (thus causing a slight perf regression) for FF19 and only take the forward fix for FF20.

Comment 27

[Alex Keybl [:akeybl]]

https://bugzilla.mozilla.org/show_bug.cgi?id=812393#c6

Comment 28

[Paul Silaghi, QA [:pauly]]

There are 54 crashes on FF 20b1 (buildID: 20130220104816) in the last week. I assume this is not really fixed ?

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This signature contains a bunch of different crashes. However, the Linux crashes do seem to have gone away, as far as I can tell.

Comment 30

[u279076]

(In reply to Bill McCloskey (:billm) from comment #29)
> This signature contains a bunch of different crashes. However, the Linux
> crashes do seem to have gone away, as far as I can tell.

Were the changes made in this bug for Firefox 20 only meant to address the Linux crashes contained in this signature? If so, Paul, please file a follow up bug.