Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2221353002:
Bluetooth: mac: Crash while loging nil error. (Closed)
DescriptionBluetooth: mac: Crash while loging nil error.
Should not log error if no error.
BUG=625209
Committed: https://crrev.com/e56593588554cf77aeab9bdab380ea80a7553057
Cr-Commit-Position: refs/heads/master@{#411373}
Patch Set 1 #Patch Set 2 : Better fix #
Total comments: 8
Patch Set 3 : ortuno comments #
Total comments: 3
Patch Set 4 : Removing BluetoothLowEnergyDeviceMac::DidDisconnectPeripheral(BluetoothDevice::ConnectErrorCode) #
Messages
Total messages: 20 (7 generated)
jlebel@chromium.org changed reviewers: + fbeaufort@chromium.org, ortuno@chromium.org
Hello Giovanni, Can you review this patch? Thanks,
https://codereview.chromium.org/2221353002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/2221353002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:567: if (error) { DidFailToConnectPeripheral should always be called with an error right? Maybe just DCHECK. https://codereview.chromium.org/2221353002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:568: BluetoothDeviceMac::GetConnectErrorCodeFromNSError(error); Did you mean to save the result of this function in the error_code variable? Otherwise you are always logging ERROR_FAILED. https://codereview.chromium.org/2221353002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:592: device_mac->DidDisconnectPeripheral(); Maybe just pass in BluetoothDevice::ConnectErrorCode::ERROR_UNKNOWN here. That way you don't need to overload the function. https://codereview.chromium.org/2221353002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2221353002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:406: if (create_gatt_connection_error_callbacks_.empty()) { DidDisconnectPeripheral shouldn't be called after a connection attempt right? Why are we checking the callbacks queue?
Thanks, https://codereview.chromium.org/2221353002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/2221353002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:567: if (error) { On 2016/08/09 15:40:25, ortuno wrote: > DidFailToConnectPeripheral should always be called with an error right? Maybe > just DCHECK. From the documentation it is not obvious, but from CBCentralManager.h it is very clear: - (void)centralManager:(CBCentralManager *)central didFailToConnectPeripheral:(CBPeripheral *)peripheral error:(nullable NSError *)error; I guess if there is no answer from the peripheral, the connection fails without an error. https://codereview.chromium.org/2221353002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:568: BluetoothDeviceMac::GetConnectErrorCodeFromNSError(error); On 2016/08/09 15:40:25, ortuno wrote: > Did you mean to save the result of this function in the error_code variable? > Otherwise you are always logging ERROR_FAILED. Done. https://codereview.chromium.org/2221353002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:592: device_mac->DidDisconnectPeripheral(); On 2016/08/09 15:40:25, ortuno wrote: > Maybe just pass in BluetoothDevice::ConnectErrorCode::ERROR_UNKNOWN here. That > way you don't need to overload the function. But if we disconnect explicitly, we should not generate an error, no? https://codereview.chromium.org/2221353002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2221353002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:406: if (create_gatt_connection_error_callbacks_.empty()) { On 2016/08/09 15:40:25, ortuno wrote: > DidDisconnectPeripheral shouldn't be called after a connection attempt right? > Why are we checking the callbacks queue? I was guessing it disconnect is called before the connection is done, DidDisconnectPeripheral could be called. But I tried it, it doesn't seem to happen.
https://codereview.chromium.org/2221353002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2221353002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:400: void BluetoothLowEnergyDeviceMac::DidDisconnectPeripheral( I wonder if we really need this function now. Once a device disconnects there isn't much we need to do besides logging. Maybe we can just keep the non-error version of the function.
https://codereview.chromium.org/2221353002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2221353002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:400: void BluetoothLowEnergyDeviceMac::DidDisconnectPeripheral( On 2016/08/09 19:14:56, ortuno wrote: > I wonder if we really need this function now. Once a device disconnects there > isn't much we need to do besides logging. Maybe we can just keep the non-error > version of the function. There is no point to call BluetoothDevice::DidDisconnectGatt()?
The CQ bit was checked by jlebel@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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2221353002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2221353002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:400: void BluetoothLowEnergyDeviceMac::DidDisconnectPeripheral( On 2016/08/10 at 08:42:19, jlebel wrote: > On 2016/08/09 19:14:56, ortuno wrote: > > I wonder if we really need this function now. Once a device disconnects there > > isn't much we need to do besides logging. Maybe we can just keep the non-error > > version of the function. > > There is no point to call BluetoothDevice::DidDisconnectGatt()? I'm saying that you don't really need this error variant of the function because you don't do anything with the error. So you can just delete this function and always call the other one.
On 2016/08/10 16:49:10, ortuno wrote: > https://codereview.chromium.org/2221353002/diff/40001/device/bluetooth/blueto... > File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): > > https://codereview.chromium.org/2221353002/diff/40001/device/bluetooth/blueto... > device/bluetooth/bluetooth_low_energy_device_mac.mm:400: void > BluetoothLowEnergyDeviceMac::DidDisconnectPeripheral( > On 2016/08/10 at 08:42:19, jlebel wrote: > > On 2016/08/09 19:14:56, ortuno wrote: > > > I wonder if we really need this function now. Once a device disconnects > there > > > isn't much we need to do besides logging. Maybe we can just keep the > non-error > > > version of the function. > > > > There is no point to call BluetoothDevice::DidDisconnectGatt()? > > I'm saying that you don't really need this error variant of the function because > you don't do anything with the error. So you can just delete this function and > always call the other one. ok
Thanks! lgtm
The CQ bit was checked by jlebel@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 #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Bluetooth: mac: Crash while loging nil error. Should not log error if no error. BUG=625209 ========== to ========== Bluetooth: mac: Crash while loging nil error. Should not log error if no error. BUG=625209 Committed: https://crrev.com/e56593588554cf77aeab9bdab380ea80a7553057 Cr-Commit-Position: refs/heads/master@{#411373} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e56593588554cf77aeab9bdab380ea80a7553057 Cr-Commit-Position: refs/heads/master@{#411373}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2235313004/ by kolos@chromium.org. The reason for reverting is: This CL breaks BluetoothTest.BluetoothGattConnection_SimulateDisconnect and BluetoothTest.BluetoothGattConnection_DisconnectGatt_SimulateDisconnect on on Mac10.11 and Mac10.10 See crbug.com/637216. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
