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

VERIFIED FIXED in Firefox 35

Status

()

Core
XPCOM
--
critical
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Robert Kaiser, Assigned: smaug)

Tracking

({crash})

Trunk
mozilla35
crash
Points:
---

Firefox Tracking Flags

(e10s?, firefox35 verified, firefox36 verified, firefox37 verified)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
FWIW, all those spiking crashes have a 0x24 address on 32bit systems and 0x48 on 64bit systems.
Summary: crash in nsXPCWrappedJS::CanSkip() → Nightly crash in nsXPCWrappedJS::CanSkip() with 0x24 or 0x48 address

Comment 2

4 years ago
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.
Flags: needinfo?(kairo)
(Reporter)

Comment 3

4 years ago
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.
Flags: needinfo?(kairo)
(Reporter)

Comment 4

4 years ago
Also, those are content process crashes.
Summary: Nightly crash in nsXPCWrappedJS::CanSkip() with 0x24 or 0x48 address → content process crash in nsXPCWrappedJS::CanSkip() with 0x24 or 0x48 address

Updated

4 years ago
tracking-e10s: --- → ?
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.
OS: Windows NT → All
Hardware: x86 → All
mccr8, are you saying nsIChannel was js implemented?
Could we have a weak wrapped js, then it gets
Assignee: nobody → bugs
Aha, we traverse but never unlink mChannel.
Created attachment 8489385 [details] [diff] [review]
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.
Attachment #8489385 - Flags: review?(continuation)
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?
Attachment #8489385 - Flags: review?(continuation) → review+
Oh I didn't see your comments before I reviewed your patch...
> 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.
(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.
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
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.
And unlinking mChannel in Document is somewhat different issue. I'd prefer to keep this bug
for the crash fix.
(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.
https://hg.mozilla.org/mozilla-central/rev/a92474257115
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
Status: RESOLVED → VERIFIED
status-firefox35: --- → verified
status-firefox36: --- → verified
status-firefox37: --- → verified

Comment 21

3 years ago
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
You need to log in before you can comment on or make changes to this bug.