1195729 - [Bluetooth] Release result handler if failing to send IPC messages to bluetoothd.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Bruce Sun [:brsun]]

The created |BluetoothGattResultHandler| would leak if |Send(aPDU, aRes)| fails.

This is just an edge case found by tracing codes, so this issue has not been encountered actually.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp#38
[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp#48
[3] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp#56

Comment 1

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

Are you sure? In the interface methods, such as |BluetoothDaemonGattClientInterface::RegisterClient|, [1] the call to |DispatchError| will deliver the |OnError| to the result handler. That should also free the result handler.

BTW there is a potential bug at line 55 where aRef is AddRef'ed unconditionally; even if |aRef| is |nullptr|.

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

Comment 2

[Bruce Sun [:brsun]]

I just did a little experiment on it, and the result matches what I thought. If [1][2][3] on comment 0 return failure (ex. NS_ERROR_NOT_AVAILABLE), then none of the destructors of |BluetoothGattResultHandler| or derived classes would be called.

Yes, |OnError()| would be called under this circumstance, but it doesn't help on releasing the reference correctly.

Basically |DaemonResultRunnable0| (or |DaemonResultRunnableN|) uses RAII trick to management the reference count of |aRes|, so |aRes| wouldn't be deleted if the reference count has been increased by others. Because the reference count of |aRes| is add-refed in |Send(aPDU, aRes)| explicitly, and |aRes| isn't released explicitly in any other place, so |aRes| will leak if |Send(aPDU, aUserData)| fails.

cf. Compare to the success case of |Send(aPDU, aUserData)|, the reference count of |aRes| will be managed later in |BluetoothDaemonGattModule::HandleRsp(aHeader, aPDU, aUserData)| by combining with |nsRefPtr<>| and |already_AddRefed<>|[1]. The reference count of |aRes| will be decreased after |nsRefPtr<>| leaves its local scope. So there is no leak issue in success case of |Send(aPDU, aUserData)|.

p.s. There is another crash issue if |OnError()| should be called (ref. bug 1195834).

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp#1567-1581

Comment 3

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

Hi
 
> Basically |DaemonResultRunnable0| (or |DaemonResultRunnableN|) uses RAII
> trick to management the reference count of |aRes|, so |aRes| wouldn't be
> deleted if the reference count has been increased by others. Because the
> reference count of |aRes| is add-refed in |Send(aPDU, aRes)| explicitly, and
> |aRes| isn't released explicitly in any other place, so |aRes| will leak if
> |Send(aPDU, aUserData)| fails.

Good catch! Thanks *a lot* for noticing this.

This problem should be present in the other modules as well. In case you're going to fix it, could you supply patches for them as well?

Comment 4

[Bruce Sun [:brsun]]

Sure thing. :)

Comment 5

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

Attachment: [01] Bug 1195729: Fix ref-counting of Bluetooth result handlers for failed send operations

Comment 6

[Bruce Sun [:brsun]]

Comment on attachment 8651625 [details] [diff] [review]
[01] Bug 1195729: Fix ref-counting of Bluetooth result handlers for failed send operations

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

LGTM.

nit: How about adding |NS_WARN_IF|[1] to have a warning log?

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h?from=NS_WARN_IF#45

Comment 7

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

Thanks!

Sending a command without a result handler is not itself an error. And there should be a warning where the send operation actually fails (probably generated to an errno constant).

Comment 8

[Bruce Sun [:brsun]]

Thanks for the comment. :)

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/f31e07e4ce38

Comment 10

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

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f31e07e4ce38