Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2347133002:
bluetooth: Call error callback when characteristics get destroyed during a GATT Event (mac) (Closed)
Descriptionbluetooth: Call error callback when characteristics get destroyed during a GATT Event (mac)
When a device disconnects all its characteristics get destroyed which
was causing callbacks for GATT operations to get dropped. This patch
fixes only mac's write and read. Follow up patches will fix android and
windows.
BUG=621901
Committed: https://crrev.com/0fb374e6d52746fa37dc66f9bbb3f56a5eb11883
Cr-Commit-Position: refs/heads/master@{#420558}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add CHECK for connection #Patch Set 3 : Improve comments #
Messages
Total messages: 28 (18 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: + scheib@chromium.org
scheib: PTAL. Android and windows patches follow up.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, with either comment fix or test simplification: https://codereview.chromium.org/2347133002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/2347133002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:397: // On macOS we don't get any events after disconnection so no need to If there's just 'no need to remember' then we might as well do it the same on all platforms to simplify the test. I'm guessing something goes 'bonk' on macOS, or we don't support imulateGattCharacteristicRead(nullptr on macOS, and that's why you've done this. If so, say so in the comment.
https://codereview.chromium.org/2347133002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/2347133002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:397: // On macOS we don't get any events after disconnection so no need to On 2016/09/16 at 19:50:12, scheib wrote: > If there's just 'no need to remember' then we might as well do it the same on all platforms to simplify the test. > Through experimentation and the bug report for this CL I found that once a device disconnects we get no events for that device on macOS. This is the opposite of what happens on Android. When you disconnect during a gatt operation, an event is still dispatched for that gatt object. (I believe that is the real cause of the race condition we've been seeing in which a characteristic is destroyed before the gatt event arrives). I think we still want to test this behavior on Android. > I'm guessing something goes 'bonk' on macOS, or we don't support imulateGattCharacteristicRead(nullptr on macOS, and that's why you've done this. If so, say so in the comment. We could definitely support RememberCharacteristicForSubsequentAction but it would be a non-significant refactor. I haven't seen any evidence that it's something we actually want to test so I added a CHECK to make sure the assumption that we will never get events after disconnection is satisfied. If we start crashing then we know we need to handle that case and can refactor the mac testing framework to test for it.
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...
https://codereview.chromium.org/2347133002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/2347133002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:397: // On macOS we don't get any events after disconnection so no need to Thanks for explanation, please include in code the explanation of why mac is ifdef'd (complexity that doesn't test a scenario we hit, FYI we CHECK that we don't hit it), and that on non-mac we're testing the read event arriving after disconnection.
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 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.
https://codereview.chromium.org/2347133002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/2347133002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:397: // On macOS we don't get any events after disconnection so no need to On 2016/09/20 at 03:53:16, scheib wrote: > Thanks for explanation, please include in code the explanation of why mac is ifdef'd (complexity that doesn't test a scenario we hit, FYI we CHECK that we don't hit it), and that on non-mac we're testing the read event arriving after disconnection. Done.
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org Link to the patchset: https://codereview.chromium.org/2347133002/#ps40001 (title: "Improve comments")
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
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_...)
The CQ bit was checked by ortuno@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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Call error callback when characteristics get destroyed during a GATT Event (mac) When a device disconnects all its characteristics get destroyed which was causing callbacks for GATT operations to get dropped. This patch fixes only mac's write and read. Follow up patches will fix android and windows. BUG=621901 ========== to ========== bluetooth: Call error callback when characteristics get destroyed during a GATT Event (mac) When a device disconnects all its characteristics get destroyed which was causing callbacks for GATT operations to get dropped. This patch fixes only mac's write and read. Follow up patches will fix android and windows. BUG=621901 Committed: https://crrev.com/0fb374e6d52746fa37dc66f9bbb3f56a5eb11883 Cr-Commit-Position: refs/heads/master@{#420558} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0fb374e6d52746fa37dc66f9bbb3f56a5eb11883 Cr-Commit-Position: refs/heads/master@{#420558} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
