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

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.

(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.

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 &)

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

(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.

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

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 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.

Fixed in mozilla-central: http://hg.mozilla.org/mozilla-central/rev/e7436725f170

Backed out due to crashes when running mochitest etc. http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288396879.1288398224.6385.gz&fulltext=1#err1 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288397817.1288398194.6274.gz&fulltext=1#err1 etc

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.

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.

v2.3 landed as http://hg.mozilla.org/mozilla-central/rev/de32b6e1ca00. Leaving bug open tho to make sure peterv gets to review this!

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

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

This is resolved. I will send peterv a reminder to make sure he reviews this.

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?).

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.

Comment on attachment 487199 [details] [diff] [review] Fixes to v2.2 Comment 26 seems to make sense.