Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2276213002:
bluetooth: Fix mac crash when logging is enabled (Closed)
|
Created:
4 years, 3 months ago by ortuno Modified:
4 years, 3 months ago Reviewers:
Jeffrey Yasskin CC:
chromium-reviews, ortuno+watch_chromium.org, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Fix mac crash when logging is enabled
DidDisconnectPeripheral is not always called with an error, for example
when we've initiated the disconnection. This could cause a crash when
we try to log the error.
BUG=625209
Committed: https://crrev.com/172be983dd03edd4e42b7f4d74d3db86fd178a5a
Cr-Commit-Position: refs/heads/master@{#414456}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 16 (7 generated)
The CQ bit was checked by ortuno@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...
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org
jyasskin: PTAL. I know I have other pending CLs but since this fixes a crash I figured this has higher priority for branch point.
lgtm https://codereview.chromium.org/2276213002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/2276213002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_adapter_mac.mm:600: BluetoothDevice::ConnectErrorCode::ERROR_UNKNOWN; If this happens when we explicitly disconnected while a connect was pending, should we use something like ERROR_ABORTED? https://codereview.chromium.org/2276213002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2276213002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_low_energy_device_mac.mm:375: DidFailToConnectGatt(BluetoothDevice::ConnectErrorCode::ERROR_FAILED); I think I've failed to specify this (https://github.com/WebBluetoothCG/web-bluetooth/issues/152?), but I think we're going to want to use AbortError when you call disconnect() while connect() is pending.
Oh, also, it'd be great to have tests for this fix. On Wed, Aug 24, 2016 at 4:28 PM, <jyasskin@chromium.org> wrote: > lgtm > > > > > https://codereview.chromium.org/2276213002/diff/1/device/ > bluetooth/bluetooth_adapter_mac.mm > File device/bluetooth/bluetooth_adapter_mac.mm (right): > > https://codereview.chromium.org/2276213002/diff/1/device/ > bluetooth/bluetooth_adapter_mac.mm#newcode600 > device/bluetooth/bluetooth_adapter_mac.mm:600: > BluetoothDevice::ConnectErrorCode::ERROR_UNKNOWN; > If this happens when we explicitly disconnected while a connect was > pending, should we use something like ERROR_ABORTED? > > https://codereview.chromium.org/2276213002/diff/1/device/ > bluetooth/bluetooth_low_energy_device_mac.mm > File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): > > https://codereview.chromium.org/2276213002/diff/1/device/ > bluetooth/bluetooth_low_energy_device_mac.mm#newcode375 > device/bluetooth/bluetooth_low_energy_device_mac.mm:375: > DidFailToConnectGatt(BluetoothDevice::ConnectErrorCode::ERROR_FAILED); > I think I've failed to specify this > (https://github.com/WebBluetoothCG/web-bluetooth/issues/152? > <https://github.com/WebBluetoothCG/web-bluetooth/issues/152>), but I > think we're going to want to use AbortError when you call disconnect() > while connect() is pending. > > https://codereview.chromium.org/2276213002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'm not sure what the test would be since this only happens when logging is enabled. Most tests fail when logging is enabled because of this bug. On Wed, Aug 24, 2016, 4:31 PM Jeffrey Yasskin <jyasskin@chromium.org> wrote: > Oh, also, it'd be great to have tests for this fix. > > On Wed, Aug 24, 2016 at 4:28 PM, <jyasskin@chromium.org> wrote: > >> lgtm >> >> >> >> >> >> https://codereview.chromium.org/2276213002/diff/1/device/bluetooth/bluetooth_... >> File device/bluetooth/bluetooth_adapter_mac.mm (right): >> >> >> https://codereview.chromium.org/2276213002/diff/1/device/bluetooth/bluetooth_... >> device/bluetooth/bluetooth_adapter_mac.mm:600: >> BluetoothDevice::ConnectErrorCode::ERROR_UNKNOWN; >> If this happens when we explicitly disconnected while a connect was >> pending, should we use something like ERROR_ABORTED? >> >> >> https://codereview.chromium.org/2276213002/diff/1/device/bluetooth/bluetooth_... >> File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): >> >> >> https://codereview.chromium.org/2276213002/diff/1/device/bluetooth/bluetooth_... >> device/bluetooth/bluetooth_low_energy_device_mac.mm:375: >> DidFailToConnectGatt(BluetoothDevice::ConnectErrorCode::ERROR_FAILED); >> I think I've failed to specify this >> (https://github.com/WebBluetoothCG/web-bluetooth/issues/152? >> <https://github.com/WebBluetoothCG/web-bluetooth/issues/152>), but I >> think we're going to want to use AbortError when you call disconnect() >> while connect() is pending. >> >> https://codereview.chromium.org/2276213002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Cool, if there are existing tests that this fixes when logging is enabled, that's enough for me. On Wed, Aug 24, 2016 at 4:34 PM, Giovanni Ortuño <ortuno@chromium.org> wrote: > I'm not sure what the test would be since this only happens when logging > is enabled. Most tests fail when logging is enabled because of this bug. > > On Wed, Aug 24, 2016, 4:31 PM Jeffrey Yasskin <jyasskin@chromium.org> > wrote: > >> Oh, also, it'd be great to have tests for this fix. >> >> On Wed, Aug 24, 2016 at 4:28 PM, <jyasskin@chromium.org> wrote: >> >>> lgtm >>> >>> >>> >>> >>> https://codereview.chromium.org/2276213002/diff/1/device/ >>> bluetooth/bluetooth_adapter_mac.mm >>> File device/bluetooth/bluetooth_adapter_mac.mm (right): >>> >>> https://codereview.chromium.org/2276213002/diff/1/device/ >>> bluetooth/bluetooth_adapter_mac.mm#newcode600 >>> device/bluetooth/bluetooth_adapter_mac.mm:600: >>> BluetoothDevice::ConnectErrorCode::ERROR_UNKNOWN; >>> If this happens when we explicitly disconnected while a connect was >>> pending, should we use something like ERROR_ABORTED? >>> >>> https://codereview.chromium.org/2276213002/diff/1/device/ >>> bluetooth/bluetooth_low_energy_device_mac.mm >>> File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): >>> >>> https://codereview.chromium.org/2276213002/diff/1/device/ >>> bluetooth/bluetooth_low_energy_device_mac.mm#newcode375 >>> device/bluetooth/bluetooth_low_energy_device_mac.mm:375: >>> DidFailToConnectGatt(BluetoothDevice::ConnectErrorCode::ERROR_FAILED); >>> I think I've failed to specify this >>> (https://github.com/WebBluetoothCG/web-bluetooth/issues/152? >>> <https://github.com/WebBluetoothCG/web-bluetooth/issues/152>), but I >>> think we're going to want to use AbortError when you call disconnect() >>> while connect() is pending. >>> >>> https://codereview.chromium.org/2276213002/ >>> >> >> -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
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 ortuno@chromium.org
Thanks! https://codereview.chromium.org/2276213002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/2276213002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_adapter_mac.mm:600: BluetoothDevice::ConnectErrorCode::ERROR_UNKNOWN; On 2016/08/24 at 23:28:13, Jeffrey Yasskin wrote: > If this happens when we explicitly disconnected while a connect was pending, should we use something like ERROR_ABORTED? We should but I would like to do that in a separate CL that fixes all platforms. If we were to only fix mac we would also have to change a bunch of tests. I opened http://crbug.com/641010 to keep track. https://codereview.chromium.org/2276213002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2276213002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_low_energy_device_mac.mm:375: DidFailToConnectGatt(BluetoothDevice::ConnectErrorCode::ERROR_FAILED); On 2016/08/24 at 23:28:13, Jeffrey Yasskin wrote: > I think I've failed to specify this (https://github.com/WebBluetoothCG/web-bluetooth/issues/152?), but I think we're going to want to use AbortError when you call disconnect() while connect() is pending. That makes sense. We currently have no error for that though and we would have to fix android as well if we wanted to change it in this cl. I opened http://crbug.com/641010 though.
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 #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Fix mac crash when logging is enabled DidDisconnectPeripheral is not always called with an error, for example when we've initiated the disconnection. This could cause a crash when we try to log the error. BUG=625209 ========== to ========== bluetooth: Fix mac crash when logging is enabled DidDisconnectPeripheral is not always called with an error, for example when we've initiated the disconnection. This could cause a crash when we try to log the error. BUG=625209 Committed: https://crrev.com/172be983dd03edd4e42b7f4d74d3db86fd178a5a Cr-Commit-Position: refs/heads/master@{#414456} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/172be983dd03edd4e42b7f4d74d3db86fd178a5a Cr-Commit-Position: refs/heads/master@{#414456} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
