Fix usage of BluetoothAdapter from Bluetooth API code. |
||||||
Issue descriptionThe BluetoothAdapter object is ref-counted. This prevents the adapter from going away while code is still using it. ortuno@ pointed out that there is code in the Bluetooth API and BluetoothEventRouter which uses this ref-pointer incorrectly. The two usages are (copy/pasting from the other issue), 1. BluetoothEventRouter deleting the adapter inside a method called by the adapter.[1] 2. BluetoothEventRouter (basically) posting tasks using |this| rather than a weak_ptr.[2] [1] https://cs.chromium.org/chromium/src/extensions/browser/api/bluetooth/bluetooth_event_router.cc?type=cs&l=321 [2] https://cs.chromium.org/chromium/src/extensions/browser/api/bluetooth/bluetooth_api.cc?type=cs&l=179 We should fix this right away. Sonny, could you take this on?
,
Apr 10 2017
Since the dependent issue is high priority, we should land this in 59.
,
Apr 10 2017
[2] Should actually be fine: BluetoothStartDiscoveryFunction is an ExtensionFunction which is ref counted [1] could be a problem since BluetoothAdapterBlueZ adds itself as a DBus observer: bluez::BluezDBusManager::Get()->GetBluetoothAdapterClient()->AddObserver(this); Then can trigger: BluetoothAdapterBlueZ::AdapterPropertyChanged -> BluetoothAdapterBlueZ::DiscoveringChanged -> BluetoothEventRouter::AdapterDiscoveringChanged which could release the last adapter reference.
,
Apr 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/518c599b12849fb5ca778427c877c2250d116455 commit 518c599b12849fb5ca778427c877c2250d116455 Author: sonnysasaka <sonnysasaka@chromium.org> Date: Fri Apr 28 23:34:26 2017 PostTask MaybeReleaseAdapter in BluetoothEventRouter::AdapterDiscoveringChanged Calling MaybeReleaseAdapter directly from BluetoothEventRouter::AdapterDiscoveringChanged could be a problem since this can happen: BluetoothAdapterBlueZ::AdapterPropertyChanged -> BluetoothAdapterBlueZ::DiscoveringChanged -> BluetoothEventRouter::AdapterDiscoveringChanged -> MaybeReleaseAdapter If BluetoothEventRouter is the only pointer holder left then the caller of BluetoothEventRouter::AdapterDiscoveringChanged could be gone even before that function returns. BUG= 710216 Review-Url: https://codereview.chromium.org/2852773002 Cr-Commit-Position: refs/heads/master@{#468180} [modify] https://crrev.com/518c599b12849fb5ca778427c877c2250d116455/extensions/browser/api/bluetooth/bluetooth_event_router.cc
,
May 5 2017
Issue 700040 has been verified to have been fixed by the last CL. Closing this issue.
,
May 8 2017
,
May 8 2017
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/277d00927d269a72d50eed92c78bf1a9cc0e9b26 commit 277d00927d269a72d50eed92c78bf1a9cc0e9b26 Author: Jacob Dufault <jdufault@google.com> Date: Tue May 09 21:10:12 2017 PostTask MaybeReleaseAdapter in BluetoothEventRouter::AdapterDiscoveringChanged Calling MaybeReleaseAdapter directly from BluetoothEventRouter::AdapterDiscoveringChanged could be a problem since this can happen: BluetoothAdapterBlueZ::AdapterPropertyChanged -> BluetoothAdapterBlueZ::DiscoveringChanged -> BluetoothEventRouter::AdapterDiscoveringChanged -> MaybeReleaseAdapter If BluetoothEventRouter is the only pointer holder left then the caller of BluetoothEventRouter::AdapterDiscoveringChanged could be gone even before that function returns. BUG= 710216 Review-Url: https://codereview.chromium.org/2852773002 Cr-Commit-Position: refs/heads/master@{#468180} (cherry picked from commit 518c599b12849fb5ca778427c877c2250d116455) Review-Url: https://codereview.chromium.org/2865383002 . Cr-Commit-Position: refs/branch-heads/3071@{#487} Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641} [modify] https://crrev.com/277d00927d269a72d50eed92c78bf1a9cc0e9b26/extensions/browser/api/bluetooth/bluetooth_event_router.cc |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by r...@chromium.org
, Apr 10 2017