bluetooth: Clean up WebBluetoothServiceImpl when adapter is removed

bluetooth: Clean up WebBluetoothServiceImpl when adapter is removed

There was a use-after-free because the adapter in BluetoothDispatcherHost
was destroyed before notify sessions in WebBluetoothServiceImpl were
destroyed.

This CL calls AdapterPresentChange on the adapter observers to notify
that the adapter has been destroyed and that all state should be cleaned.

BUG=604318
Committed: https://crrev.com/e2d1eb7c37a9a08934b35e92dcc4b9658a59799c
Cr-Commit-Position: refs/heads/master@{#388378}

[ortuno, 2016-04-19 20:27:13]

ortuno@chromium.org changed reviewers:
+ jyasskin@chromium.org

[ortuno, 2016-04-19 20:27:14]

jyasskin: WDYT?

[Jeffrey Yasskin, 2016-04-19 21:08:27]

Looks great! This fixes the asan/msan problem?

https://codereview.chromium.org/1898303003/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1898303003/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:490:
observer->AdapterPresentChanged(nullptr, false);
To be strictly correct, this should probably pass the old adapter rather than
nullptr. It's not going to matter any time soon, because we don't deal with
having multiple adapters, but it might matter eventually. 

We should probably send this for every adapter change (with unequal pointers?)
so that notifications never hold dangling references to the old adapter, but I
like your ClearState() implementation, and that only works when changing the
adapter to null, so I endorse limiting the artificial AdapterPresentChange to
this subset of actual changes.

[ortuno, 2016-04-19 21:36:33]

Yup! This way the notify sessions are delete before the adapter is destroyed so
we don't get a use-after-free from the notify sessions.

https://codereview.chromium.org/1898303003/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1898303003/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:490:
observer->AdapterPresentChanged(nullptr, false);
On 2016/04/19 at 21:08:27, Jeffrey Yasskin wrote:
> To be strictly correct, this should probably pass the old adapter rather than
nullptr. It's not going to matter any time soon, because we don't deal with
having multiple adapters, but it might matter eventually. 
> 

In practice there will never be more than one adapter object, no matter how many
physical adapters the device has (BluetoothAdapter is actually a misnomer. It
doesn't represent a single physical adpater, it manages all adapters and allows
you to access whichever one is available). So this event always called with a
pointer to that adapter object.

> We should probably send this for every adapter change (with unequal pointers?)
so that notifications never hold dangling references to the old adapter, but I
like your ClearState() implementation, and that only works when changing the
adapter to null, so I endorse limiting the artificial AdapterPresentChange to
this subset of actual changes.

Acknowledged.

[Jeffrey Yasskin, 2016-04-19 21:54:20]

Update ASANExpectations and MSANExpectations and then LGTM. Thanks!

[ortuno, 2016-04-19 22:28:20]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-04-19 22:28:21]

Done.

Thanks!

[ortuno, 2016-04-19 22:28:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/1898303003/#ps40001
(title: "Re-enable tests")

[commit-bot: I haz the power, 2016-04-19 22:28:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1898303003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1898303003/40001

[commit-bot: I haz the power, 2016-04-20 00:33:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-22 19:18:55]

Description was changed from

==========
bluetooth: Clean up WebBluetoothServiceImpl when adapter is removed

There was a use-after-free because the adapter in BluetoothDispatcherHost
was destroyed before notify sessions in WebBluetoothServiceImpl were
destroyed.

This CL calls AdapterPresentChange on the adapter observers to notify
that the adapter has been destroyed and that all state should be cleaned.

BUG=604318
==========

to

==========
bluetooth: Clean up WebBluetoothServiceImpl when adapter is removed

There was a use-after-free because the adapter in BluetoothDispatcherHost
was destroyed before notify sessions in WebBluetoothServiceImpl were
destroyed.

This CL calls AdapterPresentChange on the adapter observers to notify
that the adapter has been destroyed and that all state should be cleaned.

BUG=604318

Committed: https://crrev.com/e2d1eb7c37a9a08934b35e92dcc4b9658a59799c
Cr-Commit-Position: refs/heads/master@{#388378}
==========

[commit-bot: I haz the power, 2016-04-22 19:18:56]

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