Closed
Bug 1070990
Opened 10 years ago
Closed 10 years ago
B2G crash in JSAutoCompartment::JSAutoCompartment | IPC::DeserializeArrayBuffer (address 0xbad0bad1)
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
Tracking | Status | |
---|---|---|
firefox34 | --- | disabled |
firefox35 | - | fixed |
firefox-esr31 | --- | unaffected |
b2g-v1.4 | --- | unaffected |
b2g-v2.0 | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.2 | --- | fixed |
People
(Reporter: kairo, Assigned: jdm)
References
Details
(4 keywords, Whiteboard: [b2g-crash])
Crash Data
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•10 years ago
|
Component: IPC → DOM
Assignee | ||
Comment 1•10 years ago
|
||
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.
Comment 2•10 years ago
|
||
Josh, can you own this, or do we need someone else here?
Why don't we have an analysis that screams about that?
Updated•10 years ago
|
Group: core-security
Comment 4•10 years ago
|
||
(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
Flags: needinfo?(sphink)
Comment 6•10 years ago
|
||
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.
Comment 7•10 years ago
|
||
[Tracking Requested - why for this release]: Crashes
tracking-firefox35:
--- → ?
Comment 8•10 years ago
|
||
(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.
Flags: needinfo?(sphink)
Comment 9•10 years ago
|
||
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.
Assignee | ||
Comment 10•10 years ago
|
||
Yeah, TCPSocketParent's can be held alive by JS: http://mxr.mozilla.org/mozilla-central/source/dom/network/TCPSocketParentIntermediary.js#69
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → josh
Assignee | ||
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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.
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8493402 -
Flags: review?(continuation)
Assignee | ||
Updated•10 years ago
|
Attachment #8493326 -
Attachment is obsolete: true
Comment 14•10 years ago
|
||
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.
Attachment #8493402 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 15•10 years ago
|
||
Comment 16•10 years ago
|
||
From inbound, it looks like you are missing an #include "mozilla/HoldDropJSObjects.h"
status-firefox34:
--- → disabled
status-firefox35:
--- → affected
Flags: needinfo?(josh)
Assignee | ||
Comment 18•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(josh)
Comment 20•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-b2g-v1.4:
--- → unaffected
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.2:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•10 years ago
|
status-firefox-esr31:
--- → unaffected
Updated•10 years ago
|
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•