Browser crash when using many web workers [@ GCGraphBuilder::NoteXPCOMChild ] or beyond

RESOLVED FIXED

Status

()

defect
P2
critical
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: jandem, Assigned: peterv)

Tracking

({crash})

Trunk
Points:
---

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: [sg:low], fixed-in-tracemonkey, crash signature)

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

9 years ago
Posted file Crash
The attached test case crashes Minefield after a few seconds/minutes. It creates a number of web workers which do some string concatenation.

Some crash reports:
http://crash-stats.mozilla.com/report/index/bp-1a112ec7-e12c-454a-af26-c32d82101023
http://crash-stats.mozilla.com/report/index/bp-0d5ea7b9-d39b-4624-b4fa-a4f802101023

Comment 1

9 years ago
We crash in cycle collector land doing something with XPCOM strings. Lovely.
We aren't handing off buffers from dependent strings to XPCOM or anything like that (Ropes, ulp) I hope.

/be
> We aren't handing off buffers from dependent strings to XPCOM

XPConnect uses JS_GetStringChars (see XPCConvert::JSData2Native and xpc_qsDOMString::xpc_qsDOMString).  I'd assume that flattens-n-stuff.

Comment 4

9 years ago
(In reply to comment #3)
> > We aren't handing off buffers from dependent strings to XPCOM
> 
> XPConnect uses JS_GetStringChars (see XPCConvert::JSData2Native and
> xpc_qsDOMString::xpc_qsDOMString).  I'd assume that flattens-n-stuff.

It does not flatten the dependent string in case of OOM when making the string independent.

Comment 5

9 years ago
looks like it might show up under a variety of signatures.
[@ GCGraphBuilder::NoteXPCOMChild ]
[@ nsAString_internal::MutatePrep ]
are the two in comment zero
 
....Signature number: 92-_purecallnsXPCWrappedJS::QueryInterfacensIDconst,void

in http://people.mozilla.com/crash_stacks/stack-summary-4.0b8pre.txt

has a similar pattern of 
0|2|xul.dll|GCGraphBuilder::NoteXPCOMChild(nsISupports *)
0|3|xul.dll|nsXPCWrappedJS::cycleCollection::Traverse(void *,nsCycleCollectionTraversalCallback &)
0|4|xul.dll|nsCycleCollector::MarkRoots(GCGraphBuilder &)
blocking2.0: --- → ?
Keywords: crash
Summary: Browser crash when using many web workers → Browser crash when using many web workers [@ GCGraphBuilder::NoteXPCOMChild ] or beyond
I've got a crash in a recording with this testcase. Looks more like bug 604756 though, asserting here:

http://mxr.mozilla.org/mozilla-central/source/js/src/jsstr.cpp#182
(Reporter)

Comment 7

9 years ago
(In reply to comment #6)
> I've got a crash in a recording with this testcase. Looks more like bug 604756
> though, asserting here:

Interesting, the test was written for that bug but never crashed in the JS engine for me, tried release and debug builds. More proof these crashes are related.
Whiteboard: [sg:critical?]
blocking2.0: ? → beta7+

Comment 8

9 years ago
I am pretty sure this is bug 604756. Could we look at that recording with top priority? It might fix 2 b7 blockers ...

Updated

9 years ago
Severity: normal → critical
Update: this is not the same as bug 604756. Andreas tried putting a lock around flatten, and this bug still happens, but the same patch seems to stop the flatten crash

Comment 10

9 years ago
Posted patch patch (obsolete) — Splinter Review
Assignee: general → gal

Updated

9 years ago
Assignee: gal → nobody
Component: JavaScript Engine → XPConnect
OS: Mac OS X → All
Priority: -- → P2
QA Contact: general → xpconnect
Hardware: x86 → All
Attachment #486539 - Flags: review?(peterv)
Attachment #486539 - Flags: review?(mrbkap)
Comment on attachment 486539 [details] [diff] [review]
patch

>diff --git a/js/src/xpconnect/src/nsXPConnect.cpp b/js/src/xpconnect/src/nsXPConnect.cpp
>+        priv->cycleCollectionEnabled = NS_IsMainThread();

This is going to conflict with my patch for bug 606585 (which is probably going to land first). When that lands, this will be able to just set priv->cycleCollectionEnabled to true.
Attachment #486539 - Flags: review?(mrbkap) → review+

Updated

9 years ago
Blocks: 608109

Updated

9 years ago
No longer blocks: 608109

Updated

9 years ago
Blocks: 608142

Updated

9 years ago
No longer blocks: 608142
(Assignee)

Updated

9 years ago
Attachment #486539 - Flags: review?(peterv) → review-
(Assignee)

Comment 12

9 years ago
Posted patch v2 (obsolete) — Splinter Review
Assignee: nobody → peterv
Attachment #486539 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 13

9 years ago
Posted patch v2.1 (obsolete) — Splinter Review
Attachment #486860 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #487050 - Flags: review?(jst)
(Assignee)

Comment 14

9 years ago
Posted patch v2.2Splinter Review
Attachment #487050 - Attachment is obsolete: true
Attachment #487067 - Flags: review?(jst)
Attachment #487050 - Flags: review?(jst)
Attachment #487067 - Flags: review?(jst) → review+
Fixed in mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/e7436725f170
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
There are two problems with the patch that landed, the first one is that it uses the wrong lock in XPCJSRuntime::AddXPConnectRoots(), it grabs the JS gc lock when it needs to grab the xpconnect map lock, and the second problem is that we end up checking whether strings should participate in cycle collection in CheckParticipatesInCycleCollection(), which tries to get the private data from their compartments, but some strings come from the default compartment which has no private data. I've fixed the latter issue by simply stating that cycle collection is never enabled for string objects. I just pushed that change to try and I'll see how it does there.
Posted patch Fixes to v2.2Splinter Review
This I believe fixes the problems with the patch that was checked in and got backed out. The problem was the use of the wrong lock, so this makes us use the xpconnect map lock (or monitor, as it happens to be), which is what we want, and this lets me undo the move of the JS gc auto lock class in v2.2. The other fix is to default to not noting any cc roots when enumerating over the js holders at the end of XPCJSRuntime::AddXPConnectRoots(). What this means is that we'll only ever note a cc root in that case if it contains an entry that should participate in cycle collection. That means that we will never add a root if all it points to is strings, for instance. And this also removes the code in CheckParticipatesInCycleCollection() which checks if a string wants to participate in cycle collection since Andreas convinced me that it makes no sense to tell the cycle collector about strings (which makes sense to me). I also added an assertion that checks that a JS holder never traces across compartments, which isn't needed here, but seemed like a worthy addition.

This patch tested good on try.
Attachment #487199 - Flags: review?(peterv)
Attachment #487199 - Flags: review?(mrbkap)

Updated

9 years ago
Attachment #487201 - Flags: review+

Updated

9 years ago
Attachment #487201 - Flags: review?(peterv)
v2.3 landed as http://hg.mozilla.org/mozilla-central/rev/de32b6e1ca00. Leaving bug open tho to make sure peterv gets to review this!

Comment 21

9 years ago
exploitable, but extremely hard to do so in practice (borderline impossible), I am calling this sg:low but feel free to up the severity if you disagree
Whiteboard: [sg:critical?] → [sg:low], fixed-in-tracemonkey
Had to take out the assertion I added. We end up mixing cached XBL prototype handlers from different compartments in nsGlobalWindow::mCachedXBLPrototypeHandlers. Not sure what that means yet, but that's unrelated to this fix per se.

Assertion removal:

http://hg.mozilla.org/mozilla-central/rev/18caa24f974d

Comment 23

9 years ago
This is resolved. I will send peterv a reminder to make sure he reviews this.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 24

9 years ago
Comment on attachment 487201 [details] [diff] [review]
v2.3 (2.2 + fixes)

>diff --git a/js/src/xpconnect/src/xpcjsruntime.cpp b/js/src/xpconnect/src/xpcjsruntime.cpp

>+#ifdef DEBUG
>+    if(aLangID == nsIProgrammingLanguage::JAVASCRIPT &&
>+       js_GetGCThingTraceKind(aThing) == JSTRACE_OBJECT)
>+    {
>+        JSCompartment *c = static_cast<JSObject*>(aThing)->compartment();
>+        JS_ASSERT(!closure->compartment || closure->compartment == c);
>+        closure->compartment = c;
>+    }
>+#endif
>+
>+    if(!closure->cycleCollectionEnabled &&
>+       aLangID == nsIProgrammingLanguage::JAVASCRIPT &&
>+       js_GetGCThingTraceKind(aThing) == JSTRACE_OBJECT)
>+    {
>+        closure->cycleCollectionEnabled =
>+            xpc::ParticipatesInCycleCollection(closure->cx,
>+                                               static_cast<JSObject*>(aThing));
>+    }

I should have made this use ADD_TO_CC (in nsXPConnect.cpp) instead of checking for JSTRACE_OBJECT.

>@@ -447,6 +491,8 @@ void XPCJSRuntime::AddXPConnectRoots(JSC
>                     nsXPConnect::JSContextParticipant());
>     }
> 
>+    XPCAutoLock lock(mMapLock);

This is the right lock for XPCWrappedNativeScope::SuspectAllWrappers but not for any of the XPCRootSetElems. We might need to add back a AutoLockJSGC?

>@@ -454,12 +500,26 @@ void XPCJSRuntime::AddXPConnectRoots(JSC

>     if(mJSHolders.ops)
>-        JS_DHashTableEnumerate(&mJSHolders, NoteJSHolder, &cb);
>+    {
>+        Closure closure = { cx, PR_TRUE, &cb
>+#if DEBUG
>+                            , nsnull
>+#endif

The |#if DEBUG| isn't realy needed (and shouldn't it be #ifdef anyway?).
Attachment #487201 - Flags: review?(peterv) → review-
Drive-by nit: ADD_TO_CC sounds like a standard verb-phrase method name that does the adding, but it is a predicate. SHOULD_ADD_TO_CC? MUST_ADD_TO_CC if it is not optional.

/be
> >@@ -447,6 +491,8 @@ void XPCJSRuntime::AddXPConnectRoots(JSC
> >                     nsXPConnect::JSContextParticipant());
> >     }
> > 
> >+    XPCAutoLock lock(mMapLock);
> 
> This is the right lock for XPCWrappedNativeScope::SuspectAllWrappers but not
> for any of the XPCRootSetElems. We might need to add back a AutoLockJSGC?

I talked this over with peterv already, but putting it here as well, just for the record.

mrbkap, bent and myself sat down a bit ago and studied this code closely and deemed the code with this patch safe. We would need to grab the GC lock here if an XPCRootSetElem could ever be *removed* from its list while the main thread is walking the list, but we're guaranteed that removals can not happen while the main thread is walking over these lists since all code that ever removes from this list already holds the map lock, which the main thread also holds while walking the XPCRootSetElem lists. Additions *can* however happen w/o the map lock held, but those are safe to do while the main thread is walking over the lists; we either consider the element being added while iterating, or we simply skip it, both of which are fine.
Comment on attachment 487199 [details] [diff] [review]
Fixes to v2.2

Given that this has already landed, I assume I don't need to stamp this.
Attachment #487199 - Flags: review?(mrbkap)
Crash Signature: [@ GCGraphBuilder::NoteXPCOMChild ]
(Assignee)

Comment 28

8 years ago
Comment on attachment 487199 [details] [diff] [review]
Fixes to v2.2

Comment 26 seems to make sense.
Attachment #487199 - Flags: review?(peterv) → review+

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.