Closed Bug 796184 Opened 7 years ago Closed 7 years ago

[b2g-bluetooth] Crash on shutdown when a bluetooth device is attached.

Categories

(Core :: DOM: Device Interfaces, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: qdot, Assigned: qdot)

References

Details

Attachments

(1 file, 1 obsolete file)

Repro:

- Exit B2G in a normal way.

Expected behavior:

- B2G just exits

Actual behavior:

Crash
This was crashing because we weren't closing the socket on exit, then because we weren't cleaning ourselves up correctly once we dropped the reference.

nsAutoPtr ended up being a really bad idea. Wanted to keep the semantics the same for most of my pointers but just ended up causing myself trouble.

Will mark gyeh as an addition review on the Bluetooth*Manager stuff. Changed Cleanup() to be CloseSocket(), since CloseSocket() will drop the reference to the manager, which will trigger the destructor, which will call Cleanup itself.
Attachment #668220 - Flags: review?(jones.chris.g)
Attachment #668220 - Flags: review?(gyeh)
Comment on attachment 668220 [details] [diff] [review]
Patch 1 (v1) - Revert UnixSocketImpl to a bare pointer, make managers use CloseSocket

I didn't review the dom/bluetooth changes.

>diff --git a/ipc/unixsocket/UnixSocket.cpp b/ipc/unixsocket/UnixSocket.cpp

>@@ -153,7 +154,7 @@ public:
>    * directly from main thread. All non-main-thread accesses should happen with
>    * mImpl as container.
>    */
>-  RefPtr<UnixSocketConsumer> mConsumer;
>+  nsRefPtr<UnixSocketConsumer> mConsumer;
> 

Why this change?

>+  // Since impl is an autoptr, forget it before we lose our consumer member

This comment doesn't match your change.

>+  UnixSocketImpl* impl = mImpl;
>+  mImpl->mConsumer.forget();

With the mConsumer RefPtr -> nsRefPtr change, this will leak the
reference to |mConsumer|.
Attachment #668220 - Flags: review?(jones.chris.g)
Comment on attachment 668220 [details] [diff] [review]
Patch 1 (v1) - Revert UnixSocketImpl to a bare pointer, make managers use CloseSocket

Review of attachment 668220 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for dom/bluetooth/Bluetooth*Manager part
Attachment #668220 - Flags: review?(gyeh) → review+
The RefPtr -> nsRefPtr thing wasn't supposed to happen in the last patch, I was changing things to try to fix another bug and forgot to remove that. Fixed in this patch, along with other comments.
Attachment #668220 - Attachment is obsolete: true
Attachment #668538 - Flags: review?(jones.chris.g)
Comment on attachment 668538 [details] [diff] [review]
Patch 1 (v2) - Revert UnixSocketImpl to a bare pointer, make managers use CloseSocket

r=me on UnixSocket changes.
Attachment #668538 - Flags: review?(jones.chris.g) → review+
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/c9e91088418a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.