Skip to content

Commit

Permalink
bluetooth: Clean up WebBluetoothServiceImpl when adapter is removed
Browse files Browse the repository at this point in the history
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

Review URL: https://codereview.chromium.org/1898303003

Cr-Commit-Position: refs/heads/master@{#388378}
  • Loading branch information
ortuno authored and Commit bot committed Apr 20, 2016
1 parent 7169294 commit e2d1eb7
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 24 deletions.
6 changes: 6 additions & 0 deletions content/browser/bluetooth/bluetooth_dispatcher_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,12 @@ void BluetoothDispatcherHost::set_adapter(
for (device::BluetoothAdapter::Observer* observer : adapter_observers_) {
adapter_->AddObserver(observer);
}
} else {
// Notify that the adapter has been removed and observers should clean up
// their state.
for (device::BluetoothAdapter::Observer* observer : adapter_observers_) {
observer->AdapterPresentChanged(nullptr, false);
}
}
}

Expand Down
14 changes: 13 additions & 1 deletion content/browser/bluetooth/web_bluetooth_service_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,15 @@ void WebBluetoothServiceImpl::DidFinishNavigation(
navigation_handle->GetRenderFrameHost() == render_frame_host_ &&
!navigation_handle->IsSamePage()) {
// After navigation we need to clear the frame's state.
characteristic_id_to_notify_session_.clear();
ClearState();
}
}

void WebBluetoothServiceImpl::AdapterPresentChanged(
device::BluetoothAdapter* adapter,
bool present) {
if (!present) {
ClearState();
}
}

Expand Down Expand Up @@ -376,4 +384,8 @@ url::Origin WebBluetoothServiceImpl::GetOrigin() {
return render_frame_host_->GetLastCommittedOrigin();
}

void WebBluetoothServiceImpl::ClearState() {
characteristic_id_to_notify_session_.clear();
}

} // namespace content
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 @@ -60,6 +60,8 @@ class WebBluetoothServiceImpl : public blink::mojom::WebBluetoothService,
void DidFinishNavigation(NavigationHandle* navigation_handle) override;

// BluetoothAdapter::Observer:
void AdapterPresentChanged(device::BluetoothAdapter* adapter,
bool present) override;
void GattCharacteristicValueChanged(
device::BluetoothAdapter* adapter,
device::BluetoothGattCharacteristic* characteristic,
Expand Down Expand Up @@ -123,8 +125,8 @@ class WebBluetoothServiceImpl : public blink::mojom::WebBluetoothService,
void CrashRendererAndClosePipe(bad_message::BadMessageReason reason);
url::Origin GetOrigin();

// All state (maps, sets, etc.) should be cleaned after navigations
// that are not in the same page.
// Clears all state (maps, sets, etc).
void ClearState();

// Map to keep track of the characteristics' notify sessions.
std::unordered_map<std::string,
Expand Down
13 changes: 0 additions & 13 deletions third_party/WebKit/LayoutTests/ASANExpectations
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,6 @@ crbug.com/145940 [ Linux ] plugins/iframe-shims.html [ Skip ]
crbug.com/144118 [ Linux ] plugins/destroy-on-setwindow.html [ Skip ]


# heap-use-after-free in bluethooth notifications tests. http://crbug.com/604318
#0 0xd038118 in size buildtools/third_party/libc++/trunk/include/vector:639:46
#1 0xd038118 in size base/observer_list.h:114:0
#2 0xd038118 in might_have_observers base/observer_list.h:232:0
#3 0xd038118 in DoNotify device/bluetooth/test/mock_bluetooth_gatt_notify_session.cc:44:0
crbug.com/604318 bluetooth/notifications/concurrent-starts.html [ Skip ]
crbug.com/604318 bluetooth/notifications/start-before-stop-resolves.html [ Skip ]
crbug.com/604318 bluetooth/notifications/add-listener-after-promise.html [ Skip ]
crbug.com/604318 bluetooth/notifications/gc-with-pending-start.html [ Skip ]
crbug.com/604318 bluetooth/notifications/start-twice-in-a-row.html [ Skip ]
crbug.com/604318 bluetooth/notifications/start-succeeds.html [ Skip ]


# Use-after-free in NPP_DestroyStream, http://crbug.com/166932
# ==17332== ERROR: AddressSanitizer: heap-use-after-free on address 0x7f48e8a05a58
# WRITE of size 1 at 0x7f48e8a05a58 thread T0
Expand Down
8 changes: 0 additions & 8 deletions third_party/WebKit/LayoutTests/MSANExpectations
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,6 @@ crbug.com/420606 [ Linux ] fast/workers/worker-constructor.html [ Skip ]
crbug.com/420606 [ Linux ] virtual/slimmingpaint/fast/workers/worker-constructor.html [ Skip ]
crbug.com/420606 [ Linux ] virtual/sharedarraybuffer/fast/workers/worker-constructor.html [ Skip ]

# heap-use-after-free in bluethooth notifications tests. http://crbug.com/604318
crbug.com/604318 bluetooth/notifications/concurrent-starts.html [ Skip ]
crbug.com/604318 bluetooth/notifications/start-before-stop-resolves.html [ Skip ]
crbug.com/604318 bluetooth/notifications/add-listener-after-promise.html [ Skip ]
crbug.com/604318 bluetooth/notifications/gc-with-pending-start.html [ Skip ]
crbug.com/604318 bluetooth/notifications/start-twice-in-a-row.html [ Skip ]
crbug.com/604318 bluetooth/notifications/start-succeeds.html [ Skip ]

# Flaky under MSan (hang forever).
crbug.com/422982 [ Linux ] virtual/threaded [ Skip ]

Expand Down

0 comments on commit e2d1eb7

Please sign in to comment.