[Easy Unlock] Port the BluetoothThrottler class to native code.

[Easy Unlock] Port the BluetoothThrottler class to native code.

BUG=424419
TEST=components_unittests
R=tengs@chromium.org

Committed: https://crrev.com/fd7c534b719de832eb7ce1e3aa9043ba063f68c4
Cr-Commit-Position: refs/heads/master@{#326706}

[Tim Song, 2014-10-27 17:49:25]

The interface in the app for BluetoothThrottler returns a ConnectionFinder for
the Controller to use transparently by the controller.

The interface is a bit different in this CL. Are you thinking of doing some
refactoring?

https://codereview.chromium.org/663693002/diff/1/components/proximity_auth/bl...
File components/proximity_auth/bluetooth_throttler.h (right):

https://codereview.chromium.org/663693002/diff/1/components/proximity_auth/bl...
components/proximity_auth/bluetooth_throttler.h:33: // corrupted, and
unrecoverable state. http://crbug.com/345232
This is actually not entirely true now that this is a Chrome component. We can
recover by deleting the device from the list of known devices, and try
reconnecting.

[Ilya Sherman, 2014-10-28 00:40:47]

On 2014/10/27 17:49:25, Tim Song wrote:
> The interface in the app for BluetoothThrottler returns a ConnectionFinder for
> the Controller to use transparently by the controller.
> 
> The interface is a bit different in this CL. Are you thinking of doing some
> refactoring?

Yeah, I intentionally made this change as part of this CL.  The main interesting
work in porting the JavaScript code over to C++ has been establishing a clear
memory ownership model for all of the classes.  I went back and forth on this
one for a while, but ultimately decided that this API is safer (i.e. makes
correct memory management easier) than the one we had in JavaScript.

https://codereview.chromium.org/663693002/diff/1/components/proximity_auth/bl...
File components/proximity_auth/bluetooth_throttler.h (right):

https://codereview.chromium.org/663693002/diff/1/components/proximity_auth/bl...
components/proximity_auth/bluetooth_throttler.h:33: // corrupted, and
unrecoverable state. http://crbug.com/345232
On 2014/10/27 17:49:25, Tim Song wrote:
> This is actually not entirely true now that this is a Chrome component. We can
> recover by deleting the device from the list of known devices, and try
> reconnecting.

Hmm, but is that a valid thing to do?  What if the device is paired for reasons
unrelated to Smart Unlock?

[Tim Song, 2014-11-20 23:00:09]

Sorry for the late response on this one.

I'm still a bit unclear how this BluetoothThrottler interface will interact with
the rest of the system. What will call BluetoothThrottler::FindConnection()? I
would prefer that this workaround is handled transparently by the controller.

Maybe instead we should pass the throttler into BluetoothConnection and throttle
on Connect() attempts?

https://codereview.chromium.org/663693002/diff/1/components/proximity_auth/bl...
File components/proximity_auth/bluetooth_throttler.cc (right):

https://codereview.chromium.org/663693002/diff/1/components/proximity_auth/bl...
components/proximity_auth/bluetooth_throttler.cc:101: void
BluetoothThrottler::OnConnectionStatusChanged(
We should probably add the observer to the bluetooth adapter for OnDeviceChanged
events. Other services could be creating connections to the same phone.

https://codereview.chromium.org/663693002/diff/1/components/proximity_auth/bl...
File components/proximity_auth/bluetooth_throttler.h (right):

https://codereview.chromium.org/663693002/diff/1/components/proximity_auth/bl...
components/proximity_auth/bluetooth_throttler.h:33: // corrupted, and
unrecoverable state. http://crbug.com/345232
On 2014/10/28 00:40:47, Ilya Sherman wrote:
> On 2014/10/27 17:49:25, Tim Song wrote:
> > This is actually not entirely true now that this is a Chrome component. We
can
> > recover by deleting the device from the list of known devices, and try
> > reconnecting.
> 
> Hmm, but is that a valid thing to do?  What if the device is paired for
reasons
> unrelated to Smart Unlock?

That's true. Maybe disconnect also works. I'll have to test it.

[Ilya Sherman, 2015-04-02 22:59:35]

Finally getting back around to this class.  Sorry for the great delay!

On 2014/11/20 23:00:09, Tim Song wrote:
> Sorry for the late response on this one.
> 
> I'm still a bit unclear how this BluetoothThrottler interface will interact
with
> the rest of the system. What will call BluetoothThrottler::FindConnection()? I
> would prefer that this workaround is handled transparently by the controller.
> 
> Maybe instead we should pass the throttler into BluetoothConnection and
throttle
> on Connect() attempts?

BluetoothThrottler::FindConnection() is called by the controller.  This class is
just extracting this particular wart of the controller, and moving it into a
separate file.

https://chromiumcodereview.appspot.com/663693002/diff/1/components/proximity_...
File components/proximity_auth/bluetooth_throttler.cc (right):

https://chromiumcodereview.appspot.com/663693002/diff/1/components/proximity_...
components/proximity_auth/bluetooth_throttler.cc:101: void
BluetoothThrottler::OnConnectionStatusChanged(
On 2014/11/20 23:00:09, Tim Song wrote:
> We should probably add the observer to the bluetooth adapter for
OnDeviceChanged
> events. Other services could be creating connections to the same phone.

I'm not sure what you meant by this comment.  I realize that it's many moons
later and you might also be unsure...

Here's my thinking: The Connection class is responsible for tracking its state,
and the BluetoothConnection class should be responsible for updating its status
if anything with the Bluetooth adapter changed.  The Throttler is not really
Bluetooth-specific, except in the sense that it's working around a bug in the
Bluetooth stack.  Hence, it doesn't make sense to me for the Throttler to
observe the Bluetooth adapter directly.  If there are events being missed, they
should probably be tracked by the BluetoothConnection or the
BluetoothConnectionFinder class.  WDYT?

[Tim Song, 2015-04-03 15:55:02]

We shouldn't call BluetoothThrottler::FindConnection() in the controller, as
this is workaround specific to BR/EDR, and we should keep the interface the same
for LE.

https://chromiumcodereview.appspot.com/663693002/diff/1/components/proximity_...
File components/proximity_auth/bluetooth_throttler.cc (right):

https://chromiumcodereview.appspot.com/663693002/diff/1/components/proximity_...
components/proximity_auth/bluetooth_throttler.cc:101: void
BluetoothThrottler::OnConnectionStatusChanged(
On 2015/04/02 22:59:35, Ilya Sherman wrote:
> On 2014/11/20 23:00:09, Tim Song wrote:
> > We should probably add the observer to the bluetooth adapter for
> OnDeviceChanged
> > events. Other services could be creating connections to the same phone.
> 
> I'm not sure what you meant by this comment.  I realize that it's many moons
> later and you might also be unsure...
> 
> Here's my thinking: The Connection class is responsible for tracking its
state,
> and the BluetoothConnection class should be responsible for updating its
status
> if anything with the Bluetooth adapter changed.  The Throttler is not really
> Bluetooth-specific, except in the sense that it's working around a bug in the
> Bluetooth stack.  Hence, it doesn't make sense to me for the Throttler to
> observe the Bluetooth adapter directly.  If there are events being missed,
they
> should probably be tracked by the BluetoothConnection or the
> BluetoothConnectionFinder class.  WDYT?

It's possible for the Chromebook to make a connection to the phone aside from
Smart Lock. It's also possible that BluetoothConnection only disconnects the
logical service connection, but the physical connection is still present for
another service. I don't think we should expand Connection to know about all
other possible services on the physical connection.

I also don't understand your point about the Throttler not being
Bluetooth-specific. The bug is Bluetooth-specific, so it makes sense to observe
the Bluetooth adapter to work around it.

[Ilya Sherman, 2015-04-17 06:10:03]

On 2015/04/03 15:55:02, Tim Song wrote:
> We shouldn't call BluetoothThrottler::FindConnection() in the controller,
> as this is workaround specific to BR/EDR, and we should keep the 
> interface the same for LE.

Hmm, I suppose I see your point.  Done.

https://chromiumcodereview.appspot.com/663693002/diff/1/components/proximity_...
File components/proximity_auth/bluetooth_throttler.cc (right):

https://chromiumcodereview.appspot.com/663693002/diff/1/components/proximity_...
components/proximity_auth/bluetooth_throttler.cc:101: void
BluetoothThrottler::OnConnectionStatusChanged(
On 2015/04/03 15:55:02, Tim Song wrote:
> On 2015/04/02 22:59:35, Ilya Sherman wrote:
> > On 2014/11/20 23:00:09, Tim Song wrote:
> > > We should probably add the observer to the bluetooth adapter for
> > OnDeviceChanged
> > > events. Other services could be creating connections to the same phone.
> > 
> > I'm not sure what you meant by this comment.  I realize that it's many moons
> > later and you might also be unsure...
> > 
> > Here's my thinking: The Connection class is responsible for tracking its
> state,
> > and the BluetoothConnection class should be responsible for updating its
> status
> > if anything with the Bluetooth adapter changed.  The Throttler is not really
> > Bluetooth-specific, except in the sense that it's working around a bug in
the
> > Bluetooth stack.  Hence, it doesn't make sense to me for the Throttler to
> > observe the Bluetooth adapter directly.  If there are events being missed,
> they
> > should probably be tracked by the BluetoothConnection or the
> > BluetoothConnectionFinder class.  WDYT?
> 
> It's possible for the Chromebook to make a connection to the phone aside from
> Smart Lock. It's also possible that BluetoothConnection only disconnects the
> logical service connection, but the physical connection is still present for
> another service. I don't think we should expand Connection to know about all
> other possible services on the physical connection.

So, the Bluetooth kernel bug is a race condition around socket reuse.  I don't
think there's any way to observe Bluetooth sockets being opened and closed, but
I think it's also reasonable to assume that our class is the only class that's
connecting to the Smart Lock UUIDs, and hence the only sockets we're concerned
about are the ones that we manage via the BluetoothConnection class.  Hence, I
still think that it's reasonable for the throttler to observe the connection,
rather than the Bluetooth classes.  Alternately, we could try to add an observer
interface for BluetoothSockets, but that sounds like a lot of overhead for very
little gain.  WDYT?

[Tim Song, 2015-04-17 22:34:31]

https://chromiumcodereview.appspot.com/663693002/diff/1/components/proximity_...
File components/proximity_auth/bluetooth_throttler.cc (right):

https://chromiumcodereview.appspot.com/663693002/diff/1/components/proximity_...
components/proximity_auth/bluetooth_throttler.cc:101: void
BluetoothThrottler::OnConnectionStatusChanged(
On 2015/04/17 06:10:03, Ilya Sherman wrote:
> On 2015/04/03 15:55:02, Tim Song wrote:
> > On 2015/04/02 22:59:35, Ilya Sherman wrote:
> > > On 2014/11/20 23:00:09, Tim Song wrote:
> > > > We should probably add the observer to the bluetooth adapter for
> > > OnDeviceChanged
> > > > events. Other services could be creating connections to the same phone.
> > > 
> > > I'm not sure what you meant by this comment.  I realize that it's many
moons
> > > later and you might also be unsure...
> > > 
> > > Here's my thinking: The Connection class is responsible for tracking its
> > state,
> > > and the BluetoothConnection class should be responsible for updating its
> > status
> > > if anything with the Bluetooth adapter changed.  The Throttler is not
really
> > > Bluetooth-specific, except in the sense that it's working around a bug in
> the
> > > Bluetooth stack.  Hence, it doesn't make sense to me for the Throttler to
> > > observe the Bluetooth adapter directly.  If there are events being missed,
> > they
> > > should probably be tracked by the BluetoothConnection or the
> > > BluetoothConnectionFinder class.  WDYT?
> > 
> > It's possible for the Chromebook to make a connection to the phone aside
from
> > Smart Lock. It's also possible that BluetoothConnection only disconnects the
> > logical service connection, but the physical connection is still present for
> > another service. I don't think we should expand Connection to know about all
> > other possible services on the physical connection.
> 
> So, the Bluetooth kernel bug is a race condition around socket reuse.  I don't
> think there's any way to observe Bluetooth sockets being opened and closed,
but
> I think it's also reasonable to assume that our class is the only class that's
> connecting to the Smart Lock UUIDs, and hence the only sockets we're concerned
> about are the ones that we manage via the BluetoothConnection class.  Hence, I
> still think that it's reasonable for the throttler to observe the connection,
> rather than the Bluetooth classes.  Alternately, we could try to add an
observer
> interface for BluetoothSockets, but that sounds like a lot of overhead for
very
> little gain.  WDYT?

Are you certain the bug is with respect to the service UUID and not the
Bluetooth address?

https://chromiumcodereview.appspot.com/663693002/diff/40001/components/proxim...
File components/proximity_auth/throttled_bluetooth_connection_finder.cc (right):

https://chromiumcodereview.appspot.com/663693002/diff/40001/components/proxim...
components/proximity_auth/throttled_bluetooth_connection_finder.cc:69:
throttler_->OnConnectionClosed();
Using the ThrottledBluetoothConnectionFinder instance as an observer may be
problematic if the controller does not keep the ConnectionFinder around after it
finds a connection. Wouldn't it be better for the throttler to be the observer?

[Ilya Sherman, 2015-04-22 00:48:59]

https://chromiumcodereview.appspot.com/663693002/diff/1/components/proximity_...
File components/proximity_auth/bluetooth_throttler.cc (right):

https://chromiumcodereview.appspot.com/663693002/diff/1/components/proximity_...
components/proximity_auth/bluetooth_throttler.cc:101: void
BluetoothThrottler::OnConnectionStatusChanged(
On 2015/04/17 22:34:31, Tim Song wrote:
> On 2015/04/17 06:10:03, Ilya Sherman wrote:
> > On 2015/04/03 15:55:02, Tim Song wrote:
> > > On 2015/04/02 22:59:35, Ilya Sherman wrote:
> > > > On 2014/11/20 23:00:09, Tim Song wrote:
> > > > > We should probably add the observer to the bluetooth adapter for
> > > > OnDeviceChanged
> > > > > events. Other services could be creating connections to the same
phone.
> > > > 
> > > > I'm not sure what you meant by this comment.  I realize that it's many
> moons
> > > > later and you might also be unsure...
> > > > 
> > > > Here's my thinking: The Connection class is responsible for tracking its
> > > state,
> > > > and the BluetoothConnection class should be responsible for updating its
> > > status
> > > > if anything with the Bluetooth adapter changed.  The Throttler is not
> really
> > > > Bluetooth-specific, except in the sense that it's working around a bug
in
> > the
> > > > Bluetooth stack.  Hence, it doesn't make sense to me for the Throttler
to
> > > > observe the Bluetooth adapter directly.  If there are events being
missed,
> > > they
> > > > should probably be tracked by the BluetoothConnection or the
> > > > BluetoothConnectionFinder class.  WDYT?
> > > 
> > > It's possible for the Chromebook to make a connection to the phone aside
> from
> > > Smart Lock. It's also possible that BluetoothConnection only disconnects
the
> > > logical service connection, but the physical connection is still present
for
> > > another service. I don't think we should expand Connection to know about
all
> > > other possible services on the physical connection.
> > 
> > So, the Bluetooth kernel bug is a race condition around socket reuse.  I
don't
> > think there's any way to observe Bluetooth sockets being opened and closed,
> but
> > I think it's also reasonable to assume that our class is the only class
that's
> > connecting to the Smart Lock UUIDs, and hence the only sockets we're
concerned
> > about are the ones that we manage via the BluetoothConnection class.  Hence,
I
> > still think that it's reasonable for the throttler to observe the
connection,
> > rather than the Bluetooth classes.  Alternately, we could try to add an
> observer
> > interface for BluetoothSockets, but that sounds like a lot of overhead for
> very
> > little gain.  WDYT?
> 
> Are you certain the bug is with respect to the service UUID and not the
> Bluetooth address?

Well, AFAICT from the bug, the issue is that sockets are reused.  Do you agree
that a socket is specific to the service UUID, and not to the Bluetooth address?

https://chromiumcodereview.appspot.com/663693002/diff/40001/components/proxim...
File components/proximity_auth/throttled_bluetooth_connection_finder.cc (right):

https://chromiumcodereview.appspot.com/663693002/diff/40001/components/proxim...
components/proximity_auth/throttled_bluetooth_connection_finder.cc:69:
throttler_->OnConnectionClosed();
On 2015/04/17 22:34:31, Tim Song wrote:
> Using the ThrottledBluetoothConnectionFinder instance as an observer may be
> problematic if the controller does not keep the ConnectionFinder around after
it
> finds a connection. Wouldn't it be better for the throttler to be the
observer?

Good point.  Done.

[Tim Song, 2015-04-22 18:07:33]

Let me double check whether the bug is connection or service specific.

I'll LGTM this CL for now.

[Ilya Sherman, 2015-04-22 21:21:02]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2015-04-22 21:25:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/663693002/60001

[commit-bot: I haz the power, 2015-04-22 22:34:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-22 22:34:38]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[Ilya Sherman, 2015-04-23 00:28:31]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-04-23 00:28:31]

The patchset sent to the CQ was uploaded after l-g-t-m from tengs@chromium.org
Link to the patchset: https://codereview.chromium.org/663693002/#ps80001 (title:
"Fix Windows compilation error?")

[commit-bot: I haz the power, 2015-04-23 00:29:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/663693002/80001

[commit-bot: I haz the power, 2015-04-23 02:15:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-23 02:15:02]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[Ilya Sherman, 2015-04-23 23:26:22]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-04-23 23:26:23]

The patchset sent to the CQ was uploaded after l-g-t-m from tengs@chromium.org
Link to the patchset: https://codereview.chromium.org/663693002/#ps100001
(title: "Fix Windows compilation error, take 2")

[commit-bot: I haz the power, 2015-04-23 23:26:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/663693002/100001

[commit-bot: I haz the power, 2015-04-24 00:44:06]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-24 00:44:57]

Patchset 6 (id:??) landed as
https://crrev.com/fd7c534b719de832eb7ce1e3aa9043ba063f68c4
Cr-Commit-Position: refs/heads/master@{#326706}