1239979 - Fix incomplete Bluetooth cleanup

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1239979: Init and uninit all Bluetooth profile managers

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1239979: Uninitialized Bluetooth profile managers explictly to release refs

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1239979: Close sockets when deinitializing Bluetooth profile managers

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 1239979: Add |BluetoothSocket::Accept| method

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] Bug 1239979: Store pointer to Bluetooth socket interface in |BluetoothSocket|

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] Bug 1239979: Cleanup |BluetoothSocket|'s internals when connections close

Comment 7

[Ben Tian (inactive)]

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.

Comment 8

[Ben Tian (inactive)]

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.

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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!

Comment 10

[Ben Tian (inactive)]

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.

Comment 11

[Ben Tian (inactive)]

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

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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?

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8708292 [details] [diff] [review]
[04] Bug 1239979: Add |BluetoothSocket::Accept| method

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

LGTM

Comment 14

[Ben Tian (inactive)]

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?

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1239979: Uninitialized Bluetooth profile managers explictly to release refs (v2)

Changes since v1:

 - store new A2DP/AVRCP managers directly in global static pointer

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1239979: Close sockets when deinitializing Bluetooth profile managers (v2)

Changes since v1:

  - don't close Bluetooth sockets if socket is already closed

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] Bug 1239979: Cleanup |BluetoothSocket|'s internals when connections close (v2)

Changes since v1:

  - removed |BluetoothSocket::GetObserver|

Comment 19

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [07] Bug 1239979: Get pointers to Bluetooth managers during each shutdown

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.

Comment 20

[Ben Tian (inactive)]

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

Comment 21

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/541f217c62db
https://hg.mozilla.org/integration/b2g-inbound/rev/c08aaa73c679
https://hg.mozilla.org/integration/b2g-inbound/rev/f632ea7d8d41
https://hg.mozilla.org/integration/b2g-inbound/rev/cdf8c7c78c8b
https://hg.mozilla.org/integration/b2g-inbound/rev/bbdedc47cf6f
https://hg.mozilla.org/integration/b2g-inbound/rev/7be5a7820a66
https://hg.mozilla.org/integration/b2g-inbound/rev/448d520ede25

Comment 23

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks for the reviews.

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=448d520ede25

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/541f217c62db
https://hg.mozilla.org/mozilla-central/rev/c08aaa73c679
https://hg.mozilla.org/mozilla-central/rev/f632ea7d8d41
https://hg.mozilla.org/mozilla-central/rev/cdf8c7c78c8b
https://hg.mozilla.org/mozilla-central/rev/bbdedc47cf6f
https://hg.mozilla.org/mozilla-central/rev/7be5a7820a66
https://hg.mozilla.org/mozilla-central/rev/448d520ede25