1067344 - content process crash in nsXPCWrappedJS::CanSkip() with 0x24 or 0x48 address

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Robert Kaiser]

This bug was filed from the Socorro interface and is 
report bp-997371c6-8a14-46b5-945b-090992140915.
=============================================================

This crash started spiking on Nightly 35 with the 20140911064110 build even though it was present with low volume before. The regression seems to have happened in code from 9/10.

Top frames:
0 	xul.dll 	nsXPCWrappedJS::CanSkip() 	js/xpconnect/src/XPCWrappedJS.cpp
1 	xul.dll 	nsXPCWrappedJS::CanSkip() 	js/xpconnect/src/XPCWrappedJS.cpp
2 	xul.dll 	CCGraphBuilder::NoteXPCOMChild(nsISupports*) 	xpcom/base/nsCycleCollector.cpp
3 	xul.dll 	nsDocument::cycleCollection::Traverse(void*, nsCycleCollectionTraversalCallback&) 	content/base/src/nsDocument.cpp
4 	xul.dll 	CCGraphBuilder::Traverse(PtrInfo*) 	xpcom/base/nsCycleCollector.cpp
5 	xul.dll 	nsCycleCollector::MarkRoots(js::SliceBudget&) 	xpcom/base/nsCycleCollector.cpp
6 	xul.dll 	nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*) 	xpcom/base/nsCycleCollector.cpp
7 	xul.dll 	nsCycleCollector_collectSlice(__int64) 	xpcom/base/nsCycleCollector.cpp
8 	xul.dll 	nsJSContext::RunCycleCollectorSlice() 	dom/base/nsJSEnvironment.cpp
9 	xul.dll 	CCTimerFired 	dom/base/nsJSEnvironment.cpp

For stats and more reports see https://crash-stats.mozilla.com/report/list?signature=nsXPCWrappedJS%3A%3ACanSkip%28%29

Comment 1

[Robert Kaiser]

FWIW, all those spiking crashes have a 0x24 address on 32bit systems and 0x48 on 64bit systems.

Comment 2

[Benjamin Smedberg]

Can we have a regression range link?

Crash here: http://hg.mozilla.org/mozilla-central/annotate/d070787de8f7/js/xpconnect/src/XPCWrappedJS.cpp#l63

indicates that the nsXPCWrappedJS `this` is null, when called from here: http://hg.mozilla.org/mozilla-central/annotate/d070787de8f7/js/xpconnect/src/XPCWrappedJS.cpp#l74 mRoot is null even though !IsRootWrapper.

Comment 3

[Robert Kaiser]

This is the first build after bug 1064885 so I guess any regression range I can get will point to this as the cause and this is an e10s regression.

Comment 4

[Robert Kaiser]

Also, those are content process crashes.

Comment 5

[Andrew McCreight [:mccr8]]

It looks like the root wrapper is null.  I think the only way mRoot can be null is if it was unlinked first.  I guess we could null check it, though this indicates we're in some kind of bad state.

Also, for the 10 or so crashes I looked at, these were all in nsDocument.  For about half of those I was able to see what field it was traversing, which was all mChannel.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

mccr8, are you saying nsIChannel was js implemented?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Could we have a weak wrapped js, then it gets

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Aha, we traverse but never unlink mChannel.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: null check

I think we should add the null check, since the wrapperjs thing
traversed but not unlinked is somewhat valid case, and I assume the issue
may happen elsewhere too.

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8489385 [details] [diff] [review]
null check

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

::: js/xpconnect/src/XPCWrappedJS.cpp
@@ +70,5 @@
>  
>      // For non-root wrappers, check if the root wrapper will be
>      // added to the CC graph.
> +    if (!IsRootWrapper()) {
> +        NS_ENSURE_TRUE(mRoot, false);

This should probably just go at the top level of the function, with a comment about how we're checking if we're unlinked.

Also, nsDocument::mChannel is traversed but not unlinked as far as I can see.  Should that be unlinked?

Comment 11

[Andrew McCreight [:mccr8]]

Oh I didn't see your comments before I reviewed your patch...

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Also, nsDocument::mChannel is traversed but not unlinked as far as I can see.  Should
> that be unlinked?

Perhaps, but channels are not CCed anyway, note.

Comment 13

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky [:bz] from comment #12)
> Perhaps, but channels are not CCed anyway, note.

In this particular case, mChannel is an nsXPCWrappedJS, so it is CCed.

Comment 14

[Andrew McCreight [:mccr8]]

Some crude MXRing turns up a few nsIChannel implemented in JS:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/RemoteWebProgress.jsm#26

mxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/RemoteAddonsChild.jsm#237

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Since we can skip even without mRoot, and this is unusual case, I'll keep the check in the
place where it is in the patch, but adding a comment.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

And unlinking mChannel in Document is somewhat different issue. I'd prefer to keep this bug
for the crash fix.

Comment 17

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #16)
> And unlinking mChannel in Document is somewhat different issue. I'd prefer
> to keep this bug for the crash fix.

Sounds reasonable, though if the document itself is already unlinked, then unlinking mChannel might fix this crash, too.  I filed bug 1067449 for unlinking mChannel.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: https://hg.mozilla.org/integration/mozilla-inbound/rev/a92474257115

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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 20

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Socorro [1] stats over the past 4 weeks show:
- Firefox 35 = 3 crashes (one in each beta: b1, b2, b3)
- Firefox 36 = 1 crash in Nightly
- Firefox 37 = 1 crash in Nightly

[1] - https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=nsXPCWrappedJS%3A%3ACanSkip%28%29

Comment 21

[mkdante381]

Random crash with Enhanced Steam 7.3 extension on Steam sites: http://steamcommunity.com/ and http://store.steampowered.com/
Crash signature:
[@ CCGraphBuilder::NoteXPCOMChild(nsISupports*) ]

Crash report:
https://crash-stats.mozilla.com/report/index/ebc0a598-68f4-438e-a54a-b6d682150316