Closed Bug 1239979 Opened 5 years ago Closed 5 years ago

Fix incomplete Bluetooth cleanup

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
2.6 S6 - 1/29
Tracking Status
firefox46 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(7 files, 3 obsolete files)

9.43 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
2.62 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
5.03 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
14.50 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
5.78 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
4.22 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
1.35 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
Bluetooth doesn't cleanup it's internal data structures during cleanup. There are a number of problems in the managers and socket class. It should be fixed to not leak or consume unnecessary resources.
Comment on attachment 8708289 [details] [diff] [review]
[01] Bug 1239979: Init and uninit all Bluetooth profile managers

Redirect profile manager init/deinit review to Shawn.
Attachment #8708289 - Flags: review?(btian) → review?(shuang)
Comment on attachment 8708290 [details] [diff] [review]
[02] Bug 1239979: Uninitialized Bluetooth profile managers explictly to release refs

Redirect profile manager init/deinit review to Shawn.
Attachment #8708290 - Flags: review?(btian) → review?(shuang)
Comment on attachment 8708289 [details] [diff] [review]
[01] Bug 1239979: Init and uninit all Bluetooth profile managers

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

LGTM. Thanks!
Attachment #8708289 - Flags: review?(shuang) → review+
Comment on attachment 8708291 [details] [diff] [review]
[03] Bug 1239979: Close sockets when deinitializing Bluetooth profile managers

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

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +1644,5 @@
>    mSuccessFlag = false;
>  
> +  if (mSocket &&
> +      mSocket->GetConnectionStatus() != SOCKET_DISCONNECTED) {
> +    mSocket->Close();

No need to close |mSocket| since here means |OnSocketDisconnect| of |mSocket| is already called [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothOppManager.cpp#1547

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +1210,5 @@
>    mDeviceAddress.Clear();
> +
> +  if (mSocket &&
> +      mSocket->GetConnectionStatus() != SOCKET_DISCONNECTED) {
> +    mSocket->Close();

Ditto.
Attachment #8708291 - Flags: review?(btian) → review+
Comment on attachment 8708293 [details] [diff] [review]
[05] Bug 1239979: Store pointer to Bluetooth socket interface in |BluetoothSocket|

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

LGTM
Attachment #8708293 - Flags: review?(btian) → review+
Comment on attachment 8708290 [details] [diff] [review]
[02] Bug 1239979: Uninitialized Bluetooth profile managers explictly to release refs

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

Thomas, thanks for catching this. LGTM, can you add RefPtr in |Get()| for A2dp/Avrcp manager?

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +320,3 @@
>      sBluetoothA2dpManager = nullptr;
>  
>      if (mRes) {

I was wondering if you would like to apply RefPtr<> to A2DP/Avrcp profile managers?
Attachment #8708290 - Flags: review?(shuang) → review+
Comment on attachment 8708292 [details] [diff] [review]
[04] Bug 1239979: Add |BluetoothSocket::Accept| method

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

LGTM
Attachment #8708292 - Flags: review?(btian) → review+
Comment on attachment 8708295 [details] [diff] [review]
[06] Bug 1239979: Cleanup |BluetoothSocket|'s internals when connections close

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

Please see my question below.

::: dom/bluetooth/bluedroid/BluetoothSocket.h
@@ +25,5 @@
>    BluetoothSocket(BluetoothSocketObserver* aObserver);
>    ~BluetoothSocket();
>  
> +  void                     SetObserver(BluetoothSocketObserver* aObserver);
> +  BluetoothSocketObserver* GetObserver() const;

I don't see where |GetObserver| is called. Do we still need it?
Attachment #8708295 - Flags: review?(btian)
Changes since v1:

 - store new A2DP/AVRCP managers directly in global static pointer
Attachment #8708290 - Attachment is obsolete: true
Attachment #8709401 - Flags: review+
Changes since v1:

  - don't close Bluetooth sockets if socket is already closed
Attachment #8708291 - Attachment is obsolete: true
Attachment #8709403 - Flags: review+
> > +  BluetoothSocketObserver* GetObserver() const;
> 
> I don't see where |GetObserver| is called. Do we still need it?

I added it for completeness. Will be removed in the next iteration.
Changes since v1:

  - removed |BluetoothSocket::GetObserver|
Attachment #8709405 - Flags: review?(btian)
Attachment #8708295 - Attachment is obsolete: true
I found that the patch set requires this change as well. The pointers to the Bluetooth managers change when re-enabling Bluetooth, so we can't store them in a static array.
Attachment #8709407 - Flags: review?(shuang)
Comment on attachment 8709405 [details] [diff] [review]
[06] Bug 1239979: Cleanup |BluetoothSocket|'s internals when connections close (v2)

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

LGTM
Attachment #8709405 - Flags: review?(btian) → review+
Comment on attachment 8709407 [details] [diff] [review]
[07] Bug 1239979: Get pointers to Bluetooth managers during each shutdown

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

LGTM. Oh I missed this part. Thanks a lot.
Attachment #8709407 - Flags: review?(shuang) → review+
You need to log in before you can comment on or make changes to this bug.