Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2279853002:
Make BluetoothAdapterBlueZ::Init early out if shutdown (Closed)
|
Created:
4 years, 3 months ago by sky Modified:
4 years, 3 months ago Reviewers:
Rahul Chaturvedi CC:
chromium-reviews, scheib+watch_chromium.org, ortuno+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake BluetoothAdapterBlueZ::Init early out if shutdown
If a test creates a BluetoothAdapterBlueZ, inits, then shuts it down
it's entirely possible to get to BluetoothAdapterBlueZ::Init() after
bluez::BluezDBusManager has been destroyed. This patch makes
BluetoothAdapterBlueZ::Init early out in such a situation.
BUG=641026
TEST=none
R=rkc@chromium.org
Committed: https://crrev.com/ff1b30c1127c08a265e95f2b6cc60d65e8bd7e49
Cr-Commit-Position: refs/heads/master@{#414584}
Patch Set 1 #
Total comments: 3
Patch Set 2 : move conditional #Messages
Total messages: 20 (13 generated)
https://codereview.chromium.org/2279853002/diff/1/device/bluetooth/bluez/blue... File device/bluetooth/bluez/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/2279853002/diff/1/device/bluetooth/bluez/blue... device/bluetooth/bluez/bluetooth_adapter_bluez.cc:212: if (dbus_is_shutdown_) I don't know if we need to set initialized_ and run init_callback_ as happens on 218/219 in this case. One option is to combine the conditionals, by that I mean move this into 217. I'm not familiar enough with the code to know which is preferable.
The CQ bit was checked by sky@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
steel@chromium.org changed reviewers: + steel@chromium.org - rkc@chromium.org
https://codereview.chromium.org/2279853002/diff/1/device/bluetooth/bluez/blue... File device/bluetooth/bluez/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/2279853002/diff/1/device/bluetooth/bluez/blue... device/bluetooth/bluez/bluetooth_adapter_bluez.cc:212: if (dbus_is_shutdown_) On 2016/08/25 17:15:09, sky wrote: > I don't know if we need to set initialized_ and run init_callback_ as happens on > 218/219 in this case. One option is to combine the conditionals, by that I mean > move this into 217. I'm not familiar enough with the code to know which is > preferable. In case a test is waiting on the adapter to get ready (which plenty do), they'd time out. Even if they were to fail, we'd rather they fail than time out. So this should be moved down to 217, if (dbus_is_shutdown_ || !bluez::BluezDBusManager::Get()->IsObjectManagerSupported()) {
https://codereview.chromium.org/2279853002/diff/1/device/bluetooth/bluez/blue... File device/bluetooth/bluez/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/2279853002/diff/1/device/bluetooth/bluez/blue... device/bluetooth/bluez/bluetooth_adapter_bluez.cc:212: if (dbus_is_shutdown_) On 2016/08/25 20:21:59, Rahul Chaturvedi wrote: > On 2016/08/25 17:15:09, sky wrote: > > I don't know if we need to set initialized_ and run init_callback_ as happens > on > > 218/219 in this case. One option is to combine the conditionals, by that I > mean > > move this into 217. I'm not familiar enough with the code to know which is > > preferable. > > In case a test is waiting on the adapter to get ready (which plenty do), they'd > time out. Even if they were to fail, we'd rather they fail than time out. So > this should be moved down to 217, > if (dbus_is_shutdown_ || > !bluez::BluezDBusManager::Get()->IsObjectManagerSupported()) { Done.
The CQ bit was checked by sky@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by steel@chromium.org
The CQ bit was unchecked by steel@chromium.org
lgtm
The CQ bit was checked by sky@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Make BluetoothAdapterBlueZ::Init early out if shutdown If a test creates a BluetoothAdapterBlueZ, inits, then shuts it down it's entirely possible to get to BluetoothAdapterBlueZ::Init() after bluez::BluezDBusManager has been destroyed. This patch makes BluetoothAdapterBlueZ::Init early out in such a situation. BUG=641026 TEST=none R=rkc@chromium.org ========== to ========== Make BluetoothAdapterBlueZ::Init early out if shutdown If a test creates a BluetoothAdapterBlueZ, inits, then shuts it down it's entirely possible to get to BluetoothAdapterBlueZ::Init() after bluez::BluezDBusManager has been destroyed. This patch makes BluetoothAdapterBlueZ::Init early out in such a situation. BUG=641026 TEST=none R=rkc@chromium.org Committed: https://crrev.com/ff1b30c1127c08a265e95f2b6cc60d65e8bd7e49 Cr-Commit-Position: refs/heads/master@{#414584} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ff1b30c1127c08a265e95f2b6cc60d65e8bd7e49 Cr-Commit-Position: refs/heads/master@{#414584} |
