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

RESOLVED FIXED in Firefox 43

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: brsun, Assigned: tzimmermann)

Tracking

unspecified
FxOS-S6 (04Sep)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

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
(Reporter)

Comment 2

3 years ago
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
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?
(Reporter)

Comment 4

3 years ago
Sure thing. :)
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
Created attachment 8651625 [details] [diff] [review]
[01] Bug 1195729: Fix ref-counting of Bluetooth result handlers for failed send operations
Attachment #8651625 - Flags: review?(brsun)
(Reporter)

Comment 6

3 years ago
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
Attachment #8651625 - Flags: review?(brsun) → review+
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).
(Reporter)

Comment 8

3 years ago
Thanks for the comment. :)
https://hg.mozilla.org/mozilla-central/rev/f31e07e4ce38
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S6 (04Sep)
You need to log in before you can comment on or make changes to this bug.