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.

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 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 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 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 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 on attachment 8708292 [details] [diff] [review] [04] Bug 1239979: Add |BluetoothSocket::Accept| method Review of attachment 8708292 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

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?

Changes since v1: - store new A2DP/AVRCP managers directly in global static pointer

Changes since v1: - don't close Bluetooth sockets if socket is already closed

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

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

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

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

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