Skip to content

Commit

Permalink
Fix ownership of BluetoothAdapter in BluetoothDeviceChooserController
Browse files Browse the repository at this point in the history
BluetoothAdapter is a reference counted object and so
BluetoothDeviceChooserController should own it using a scoped_refptr.
Fixing this requires also fixing BluetoothAdapterFactoryWrapper's
AcquireAdapterCallback to take a scoped_refptr rather than a raw
pointer. A test for proper ownership has been added.

(cherry picked from commit a255d1b)

Bug: 1024121
Change-Id: I6342322e059f9cbff2a0d5f073f6bccfb0ca7c36
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1914536
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#715206}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1916960
Reviewed-by: Krishna Govind <govind@chromium.org>
Cr-Commit-Position: refs/branch-heads/3967@{#3}
Cr-Branched-From: 4042bae-refs/heads/master@{#715079}
  • Loading branch information
reillyeon authored and Krishna Govind committed Nov 14, 2019
1 parent 198fc3b commit ee96769
Show file tree
Hide file tree
Showing 7 changed files with 33 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -232,8 +232,8 @@ void RecordScanningDuration(const base::TimeDelta& duration) {
BluetoothDeviceChooserController::BluetoothDeviceChooserController(
WebBluetoothServiceImpl* web_bluetooth_service,
RenderFrameHost* render_frame_host,
device::BluetoothAdapter* adapter)
: adapter_(adapter),
scoped_refptr<device::BluetoothAdapter> adapter)
: adapter_(std::move(adapter)),
web_bluetooth_service_(web_bluetooth_service),
render_frame_host_(render_frame_host),
web_contents_(WebContents::FromRenderFrameHost(render_frame_host_)),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class CONTENT_EXPORT BluetoothDeviceChooserController final {
BluetoothDeviceChooserController(
WebBluetoothServiceImpl* web_bluetooth_service_,
RenderFrameHost* render_frame_host,
device::BluetoothAdapter* adapter);
scoped_refptr<device::BluetoothAdapter> adapter);
~BluetoothDeviceChooserController();

// This function performs the following checks before starting a discovery
Expand Down Expand Up @@ -127,7 +127,7 @@ class CONTENT_EXPORT BluetoothDeviceChooserController final {
static int64_t scan_duration_;

// The adapter used to get existing devices and start a discovery session.
device::BluetoothAdapter* adapter_;
scoped_refptr<device::BluetoothAdapter> adapter_;
// The WebBluetoothServiceImpl that owns this instance.
WebBluetoothServiceImpl* web_bluetooth_service_;
// The RenderFrameHost that owns web_bluetooth_service_.
Expand Down
11 changes: 6 additions & 5 deletions content/browser/bluetooth/web_bluetooth_service_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,8 @@ void WebBluetoothServiceImpl::GetAvailability(
}

auto get_availability_impl = base::BindOnce(
[](GetAvailabilityCallback callback, device::BluetoothAdapter* adapter) {
[](GetAvailabilityCallback callback,
scoped_refptr<device::BluetoothAdapter> adapter) {
std::move(callback).Run(adapter->IsPresent());
},
std::move(callback));
Expand Down Expand Up @@ -1236,7 +1237,7 @@ void WebBluetoothServiceImpl::RequestScanningStartImpl(
mojo::AssociatedRemote<blink::mojom::WebBluetoothScanClient> client,
blink::mojom::WebBluetoothRequestLEScanOptionsPtr options,
RequestScanningStartCallback callback,
device::BluetoothAdapter* adapter) {
scoped_refptr<device::BluetoothAdapter> adapter) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);

// The renderer should never send invalid options.
Expand Down Expand Up @@ -1354,7 +1355,7 @@ void WebBluetoothServiceImpl::OnDiscoverySessionError() {
void WebBluetoothServiceImpl::RequestDeviceImpl(
blink::mojom::WebBluetoothRequestDeviceOptionsPtr options,
RequestDeviceCallback callback,
device::BluetoothAdapter* adapter) {
scoped_refptr<device::BluetoothAdapter> adapter) {
// The renderer should never send invalid options.
if (IsRequestDeviceOptionsInvalid(options)) {
CrashRendererAndClosePipe(bad_message::BDH_INVALID_OPTIONS);
Expand All @@ -1369,8 +1370,8 @@ void WebBluetoothServiceImpl::RequestDeviceImpl(
// the new one to make sure they can't conflict.
device_chooser_controller_.reset();

device_chooser_controller_.reset(
new BluetoothDeviceChooserController(this, render_frame_host_, adapter));
device_chooser_controller_.reset(new BluetoothDeviceChooserController(
this, render_frame_host_, std::move(adapter)));

// TODO(crbug.com/730593): Remove AdaptCallbackForRepeating() by updating
// the callee interface.
Expand Down
6 changes: 4 additions & 2 deletions content/browser/bluetooth/web_bluetooth_service_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ class CONTENT_EXPORT WebBluetoothServiceImpl
BluetoothDeviceScanningPromptController* prompt_controller);

private:
FRIEND_TEST_ALL_PREFIXES(WebBluetoothServiceImplTest,
ClearStateDuringRequestDevice);
FRIEND_TEST_ALL_PREFIXES(WebBluetoothServiceImplTest, PermissionAllowed);
FRIEND_TEST_ALL_PREFIXES(WebBluetoothServiceImplTest,
PermissionPromptCanceled);
Expand Down Expand Up @@ -245,13 +247,13 @@ class CONTENT_EXPORT WebBluetoothServiceImpl
void RequestDeviceImpl(
blink::mojom::WebBluetoothRequestDeviceOptionsPtr options,
RequestDeviceCallback callback,
device::BluetoothAdapter* adapter);
scoped_refptr<device::BluetoothAdapter> adapter);

void RequestScanningStartImpl(
mojo::AssociatedRemote<blink::mojom::WebBluetoothScanClient> client,
blink::mojom::WebBluetoothRequestLEScanOptionsPtr options,
RequestScanningStartCallback callback,
device::BluetoothAdapter* adapter);
scoped_refptr<device::BluetoothAdapter> adapter);

void OnStartDiscoverySession(
mojo::AssociatedRemote<blink::mojom::WebBluetoothScanClient> client,
Expand Down
14 changes: 14 additions & 0 deletions content/browser/bluetooth/web_bluetooth_service_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,20 @@ class WebBluetoothServiceImplTest : public RenderViewHostImplTestHarness {
DISALLOW_COPY_AND_ASSIGN(WebBluetoothServiceImplTest);
};

TEST_F(WebBluetoothServiceImplTest, ClearStateDuringRequestDevice) {
auto options = blink::mojom::WebBluetoothRequestDeviceOptions::New();
options->accept_all_devices = true;

base::RunLoop loop;
service_->RequestDevice(
std::move(options),
base::BindLambdaForTesting(
[&loop](blink::mojom::WebBluetoothResult,
blink::mojom::WebBluetoothDevicePtr) { loop.Quit(); }));
service_->ClearState();
loop.Run();
}

TEST_F(WebBluetoothServiceImplTest, PermissionAllowed) {
blink::mojom::WebBluetoothLeScanFilterPtr filter = CreateScanFilter("a", "b");
base::Optional<WebBluetoothServiceImpl::ScanFilters> filters;
Expand Down
7 changes: 3 additions & 4 deletions device/bluetooth/bluetooth_adapter_factory_wrapper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,9 @@ void BluetoothAdapterFactoryWrapper::AcquireAdapter(
DCHECK(!GetAdapter(observer));

AddAdapterObserver(observer);
if (adapter_.get()) {
if (adapter_) {
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(std::move(callback), base::Unretained(adapter_.get())));
FROM_HERE, base::BindOnce(std::move(callback), adapter_));
return;
}

Expand Down Expand Up @@ -99,7 +98,7 @@ void BluetoothAdapterFactoryWrapper::OnGetAdapter(
DCHECK(thread_checker_.CalledOnValidThread());

set_adapter(adapter);
std::move(continuation).Run(adapter_.get());
std::move(continuation).Run(adapter_);
}

bool BluetoothAdapterFactoryWrapper::HasAdapter(
Expand Down
3 changes: 2 additions & 1 deletion device/bluetooth/bluetooth_adapter_factory_wrapper.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ namespace device {
// http://crbug.com/603291
class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterFactoryWrapper {
public:
using AcquireAdapterCallback = base::OnceCallback<void(BluetoothAdapter*)>;
using AcquireAdapterCallback =
base::OnceCallback<void(scoped_refptr<BluetoothAdapter>)>;

~BluetoothAdapterFactoryWrapper();

Expand Down

0 comments on commit ee96769

Please sign in to comment.