BleConnectionManager: Check when receiving BLE advertisements if a SecureChannel already exists. No…

BleConnectionManager: Check when receiving BLE advertisements if a SecureChannel already exists. Not doing so results in a crash.

BUG=706640
Review-Url: https://codereview.chromium.org/2804123002
Cr-Commit-Position: refs/heads/master@{#462668}
Committed: https://chromium.googlesource.com/chromium/src/+/e9d564bfe77be2833f5aa818ebc064f261692b06

[Ryan Hansberry, 2017-04-06 21:02:49]

Description was changed from

==========
BleConnectionManager: Check when receiving BLE advertisements if a SecureChannel
already exists. Not doing so results in a crash.

BUG=706640
==========

to

==========
BleConnectionManager: Check when receiving BLE advertisements if a SecureChannel
already exists. Not doing so results in a crash.

BUG=706640
==========

[Ryan Hansberry, 2017-04-06 21:02:49]

hansberry@chromium.org changed reviewers:
+ khorimoto@chromium.org

[Ryan Hansberry, 2017-04-06 21:03:45]

Verified that the test crashes without the fix.

[Kyle Horimoto, 2017-04-06 21:17:31]

LGTM with some nits

https://codereview.chromium.org/2804123002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/ble_connection_manager.cc (right):

https://codereview.chromium.org/2804123002/diff/1/chromeos/components/tether/...
chromeos/components/tether/ble_connection_manager.cc:329: << ", Remote Device
ID: " << remote_device.GetDeviceId();
Truncate the device ID.

https://codereview.chromium.org/2804123002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/ble_connection_manager_unittest.cc (right):

https://codereview.chromium.org/2804123002/diff/1/chromeos/components/tether/...
chromeos/components/tether/ble_connection_manager_unittest.cc:688: // Test for
fix to crbug.com/706640. This test will crash without the fix.
In addition to just testing that it doesn't crash, can you test that only one
channel is created?

https://codereview.chromium.org/2804123002/diff/1/chromeos/components/tether/...
chromeos/components/tether/ble_connection_manager_unittest.cc:720:
manager_->SendMessage(test_devices_[0], "request1");
Do we need the stuff from here onwards? Seems unrelated to this test. If you
want to keep it, maybe try simulating more scan results during this section to
ensure that nothing goes wrong.

[Ryan Hansberry, 2017-04-06 22:14:59]

https://codereview.chromium.org/2804123002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/ble_connection_manager.cc (right):

https://codereview.chromium.org/2804123002/diff/1/chromeos/components/tether/...
chromeos/components/tether/ble_connection_manager.cc:329: << ", Remote Device
ID: " << remote_device.GetDeviceId();
On 2017/04/06 21:17:30, Kyle Horimoto wrote:
> Truncate the device ID.

Done for here and the log above as well.

https://codereview.chromium.org/2804123002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/ble_connection_manager_unittest.cc (right):

https://codereview.chromium.org/2804123002/diff/1/chromeos/components/tether/...
chromeos/components/tether/ble_connection_manager_unittest.cc:688: // Test for
fix to crbug.com/706640. This test will crash without the fix.
On 2017/04/06 21:17:30, Kyle Horimoto wrote:
> In addition to just testing that it doesn't crash, can you test that only one
> channel is created?

Good call, done.

https://codereview.chromium.org/2804123002/diff/1/chromeos/components/tether/...
chromeos/components/tether/ble_connection_manager_unittest.cc:720:
manager_->SendMessage(test_devices_[0], "request1");
On 2017/04/06 21:17:30, Kyle Horimoto wrote:
> Do we need the stuff from here onwards? Seems unrelated to this test. If you
> want to keep it, maybe try simulating more scan results during this section to
> ensure that nothing goes wrong.

Nope, no need for it. Removed.

[Ryan Hansberry, 2017-04-06 22:15:14]

The CQ bit was checked by hansberry@chromium.org

[Ryan Hansberry, 2017-04-06 22:15:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
khorimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2804123002/#ps20001
(title: "khorimoto@ comments.")

[commit-bot: I haz the power, 2017-04-06 22:16:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 23:09:52]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491516914109750,
"parent_rev": "2b277506fe914467d4bf3f0e3b1d807586d93408", "commit_rev":
"e9d564bfe77be2833f5aa818ebc064f261692b06"}

[commit-bot: I haz the power, 2017-04-06 23:10:48]

Description was changed from

==========
BleConnectionManager: Check when receiving BLE advertisements if a SecureChannel
already exists. Not doing so results in a crash.

BUG=706640
==========

to

==========
BleConnectionManager: Check when receiving BLE advertisements if a SecureChannel
already exists. Not doing so results in a crash.

BUG=706640

Review-Url: https://codereview.chromium.org/2804123002
Cr-Commit-Position: refs/heads/master@{#462668}
Committed:
https://chromium.googlesource.com/chromium/src/+/e9d564bfe77be2833f5aa818ebc0...
==========

[commit-bot: I haz the power, 2017-04-06 23:10:49]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/e9d564bfe77be2833f5aa818ebc0...