This bug was filed from the Socorro interface and is report bp-908a7530-37fb-4dff-bb31-60feb2140919. ============================================================= Top Frames: 0 libxul.so JSAutoCompartment::JSAutoCompartment(JSContext*, JSObject*) 1 libxul.so IPC::DeserializeArrayBuffer(JS::Handle<JSObject*>, nsTArray<unsigned char> const&, JS::MutableHandle<JS::Value>) dom/network/TCPSocketChild.cpp 2 libxul.so mozilla::dom::TCPSocketParent::RecvData(SendableData const&, unsigned int const&) dom/network/TCPSocketParent.cpp 3 libxul.so mozilla::net::PTCPSocketParent::OnMessageReceived(IPC::Message const&) /builds/slave/b2g_m-cen_flm_ntly-00000000000/build/objdir-gecko/ipc/ipdl/PTCPSocketParent.cpp:306 4 libxul.so mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /builds/slave/b2g_m-cen_flm_ntly-00000000000/build/objdir-gecko/ipc/ipdl/PContentParent.cpp:2332 5 libxul.so mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) ipc/glue/MessageChannel.cpp 6 libxul.so mozilla::ipc::MessageChannel::OnMaybeDequeueOne() ipc/glue/MessageChannel.cpp [...] This is pretty much the topcrash on B2G Nightly (2.2/35.0a1) right now and started with 2014-09-17 builds on. Most of those have address 0xbad0bad1, some 0x0 or small offsets to that. More reports Flame: https://crash-stats.mozilla.com/report/list?signature=JSAutoCompartment%3A%3AJSAutoCompartment%28JSContext%2A%2C%20JSObject%2A%29%20%7C%20IPC%3A%3ADeserializeArrayBuffer%28JS%3A%3AHandle%3CJSObject%2A%3E%2C%20nsTArray%3Cunsigned%20char%3E%20const%26%2C%20JS%3A%3AMutableHandle%3CJS%3A%3AValue%3E%29#tab-reports Keon/Peak/hamachi: https://crash-stats.mozilla.com/report/list?signature=JSAutoCompartment%3A%3AJSAutoCompartment%20%7C%20IPC%3A%3ADeserializeArrayBuffer%28JS%3A%3AHandle%3CJSObject%2A%3E%2C%20nsTArray%3Cunsigned%20char%3E%20const%26%2C%20JS%3A%3AMutableHandle%3CJS%3A%3AValue%3E%29#tab-reports

I believe I originally wrote the code under the assumption that if mIntermediary is alive, mIntermediaryObj would therefore be its reflector and would also be alive. However, http://hg.mozilla.org/mozilla-central/annotate/4f2cac8d72da/dom/network/TCPSocketParent.h#l64 just looks scary now, so we should try making it a PersistentRooted instead of a raw JSObject pointer.

Josh, can you own this, or do we need someone else here?

Why don't we have an analysis that screams about that?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #3) > Why don't we have an analysis that screams about that? ni sfink for that

Presumably a regression from enabling GGC on B2G.

If it's 0xbad0bad1 then the real issue here is using a raw ptr vs a Heap that you trace or some such, no? Using a PersistentRooted for a reflector of the object itself sounds like it would leak, at first glance.

[Tracking Requested - why for this release]: Crashes

(In reply to Andrew McCreight [:mccr8] from comment #4) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #3) > > Why don't we have an analysis that screams about that? > > ni sfink for that The (current) analysis doesn't see mIntermediaryObj being held live across a GC, because the only method that does anything more than set it and return is RecvData, and there it is copied into a local rooted variable. It should be warning about a reference being taken in InitJS(), but that's a separate report that we don't pay a whole lot of attention to because it has too many false positives. It's about time to scan through it and check them again, though. Oh. Only I just checked, and it's not in there. <https://ftp-ssl.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-inbound-linux64-b2g-haz/20140922101523/refs.txt.gz> That's bad, and I will look into it. Bug 1071194. The entire TCPSocketParent class itself should be regarded as holding a GCPointer because of the JSObject* field. And it is, from gcTypes.txt.gz: GCPointer: mozilla::dom::TCPSocketParent contains field 'mIntermediaryObj' of type JSObject contains field 'field:0' of type js::ObjectImpl (probably via inheritance) because I said so But that doesn't help because code always has a pointer to a TCPSocketParent, which means the thing on the stack is a pointer to a pointer to a GC thing, which is perfectly fine. It's just like a Handle. (In reply to Josh Matthews [:jdm] from comment #1) > I believe I originally wrote the code under the assumption that if > mIntermediary is alive, mIntermediaryObj would therefore be its reflector > and would also be alive. However, > http://hg.mozilla.org/mozilla-central/annotate/4f2cac8d72da/dom/network/ > TCPSocketParent.h#l64 just looks scary now, so we should try making it a > PersistentRooted instead of a raw JSObject pointer. It's not about object liveness any more. The GC needs to update pointers now, so you have to tell it where you've hidden them away. Or you can think of it as worrying about the liveness of all edges, not just nodes. Right now, it doesn't know about the edge from TCPSocketParent to mIntermediaryObj. You have to trace it. (It looks like mIntermediaryObj was nursery allocated and was moved during a minor GC. 0xbad0bad1 comes from http://dxr.mozilla.org/mozilla-central/source/js/src/gc/Nursery.h#65) I think bz's right, mIntermdiaryObj should be a Heap<JSObject*> and you have to trace it. I looked for example code and found IDBCursor.h, which uses mozilla::HoldJSObjects() -- I guess that's a way to get the cycle collector to trace for you? Seems a little expensive to maintain another list for this case, but there can't be enough of these to matter.

Yes, HoldJSObjects() is part of what goes into cycle collecting an object, and is the most common way to root JS code in the browser. If these TCPSocket things can be held alive by JS, or by some other cycle collected object, then it will have to be cycle collected, though if it is used on multiple threads then it can't be cycle collected.

Yeah, TCPSocketParent's can be held alive by JS: http://mxr.mozilla.org/mozilla-central/source/dom/network/TCPSocketParentIntermediary.js#69

This compiles and passes tests. Do we have any good STR yet? Is there someone experiencing this crash who can test this patch? I also don't know who's a good reviewer for it.

Comment on attachment 8493326 [details] [diff] [review] Trace the IPC TCP socket's reflector Review of attachment 8493326 [details] [diff] [review]: ----------------------------------------------------------------- Olli or I can review this, it looks like pretty boilerplate CC stuff. ::: dom/network/TCPSocketParent.cpp @@ +41,5 @@ > +NS_IMPL_CYCLE_COLLECTION_CLASS(TCPSocketParentBase) > + > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(TCPSocketParentBase) > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSocket) > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIntermediary) You are missing NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS in here, so that the traverse method calls the TRACE method. @@ +140,5 @@ > TCPSocketParent::InitJS(JS::Handle<JS::Value> aIntermediary, JSContext* aCx) > { > MOZ_ASSERT(aIntermediary.isObject()); > mIntermediaryObj = &aIntermediary.toObject(); > + mozilla::HoldJSObjects(this); Please move the Hold to the ctor and the Drop to the dtor to avoid weird issues in case the object gets used earlier or later than expected.

Comment on attachment 8493402 [details] [diff] [review] Trace the IPC TCP socket's reflector Review of attachment 8493402 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/TCPSocketParent.cpp @@ +46,5 @@ > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > + > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(TCPSocketParentBase) > + tmp->mIntermediaryObj = nullptr; nit: Please make the order consistent between traverse and unlink. So either put mIntermediaryObj stuff first or last in both. This makes it easier to check later that they are consistent.

From inbound, it looks like you are missing an #include "mozilla/HoldDropJSObjects.h"

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/050ebdd41f36