Skip to content

Commit

Permalink
[Extension SW] Fix a race in EventAckData
Browse files Browse the repository at this point in the history 
When EventAckData sees an event dispatch in IncrementInflightEvent on
UI [1] thread, it hops to IO thread to start the associated worker
and then returns to UI[2] to *record* that the event needs an ack from
the renderer process. Event ACKs arriving between UI[1] and
UI[2] would incorrectly fail to find the associated data in the
*record*. This results in bad message termination.

Move the data to IO thread instead, where both IncrementInflightEvent
and DecrementInflightEvent can access the unacked event data. Note
that both IncrementInflightEvent/DecrementInflightEvent has to pass
through IO thread anyway to access ServiceWorkerContext. The new
inner class IOEventData inside EventAckData contains the map for unacked
events.

This also fixes and enables an existing test's flakiness:
FilteredEventsAfterRestart.

Bug: 844821
Change-Id: Ia9df6c896abbae62c9f395f62ef67cab8e1bfb74
Reviewed-on: https://chromium-review.googlesource.com/1228994
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592674}
  • Loading branch information
Istiaque Ahmed authored and Commit Bot committed Sep 20, 2018
1 parent 04ae1fd commit f571252
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 60 deletions.
3 changes: 1 addition & 2 deletions chrome/browser/extensions/service_worker_apitest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1362,9 +1362,8 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerLazyBackgroundTest,
lazy_observer.Wait();
}

// Flaky. See https://crbug.com/844821.
IN_PROC_BROWSER_TEST_P(ServiceWorkerLazyBackgroundTest,
DISABLED_FilteredEventsAfterRestart) {
FilteredEventsAfterRestart) {
// Create a tab to a.html, expect it to navigate to b.html. The service worker
// will see two webNavigation.onCommitted events.
ASSERT_TRUE(StartEmbeddedTestServer());
Expand Down
4 changes: 2 additions & 2 deletions chrome/test/data/extensions/api_test/service_worker/filtered_events_after_restart/page.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

var readyPromise = new Promise(function(resolve, reject) {
var getReadyPromise = () => new Promise(function(resolve, reject) {
navigator.serviceWorker.register('sw.js').then(function() {
return navigator.serviceWorker.ready;
}).then(function(registration) {
Expand All @@ -14,7 +14,7 @@ var readyPromise = new Promise(function(resolve, reject) {

window.runServiceWorkerAsync = function() {
chrome.test.log('runServiceWorkerAsync');
readyPromise.then(function(message) {
getReadyPromise().then(function(message) {
chrome.test.sendMessage('listener-added');
}).catch(function(err) {
chrome.test.sendMessage('FAILURE');
Expand Down
115 changes: 68 additions & 47 deletions extensions/browser/events/event_ack_data.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,37 +12,83 @@
namespace extensions {

namespace {
// Information about an unacked event.
// std::string - GUID of the Service Worker's external request for the event.
// int - RenderProcessHost id.
using EventInfo = std::pair<std::string, int>;
} // namespace

// Class that holds map of unacked event information keyed by event id, accessed
// on IO thread.
// TODO(lazyboy): Could this potentially be owned exclusively (and deleted) on
// the IO thread, thus not requiring RefCounted?
class EventAckData::IOEventInfo
: public base::RefCountedThreadSafe<IOEventInfo> {
public:
IOEventInfo() = default;

// Map of event information keyed by event_id.
std::map<int, EventInfo> event_map;

private:
friend class base::RefCountedThreadSafe<IOEventInfo>;
~IOEventInfo() = default;

DISALLOW_COPY_AND_ASSIGN(IOEventInfo);
};

// Invokes |ui_callback| with the GUID of the external request if successful, or
// else nullptr.
void StartExternalRequestOnIO(
// static
void EventAckData::StartExternalRequestOnIO(
content::ServiceWorkerContext* context,
int render_process_id,
int64_t version_id,
int event_id,
base::OnceCallback<void(std::unique_ptr<std::string>)> ui_callback) {
scoped_refptr<IOEventInfo> unacked_events) {
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);

auto request_uuid = std::make_unique<std::string>(base::GenerateGUID());
if (!context->StartingExternalRequest(version_id, *request_uuid))
request_uuid = nullptr;
std::string request_uuid = base::GenerateGUID();

content::BrowserThread::PostTask(
content::BrowserThread::UI, FROM_HERE,
base::BindOnce(std::move(ui_callback), std::move(request_uuid)));
if (!context->StartingExternalRequest(version_id, request_uuid)) {
LOG(ERROR) << "StartExternalRequest failed";
return;
}

// TODO(lazyboy): Clean up |unacked_events_| if RenderProcessHost died before
// it got a chance to ack |event_id|. This shouldn't happen in common cases.
std::map<int, EventInfo>& unacked_events_map = unacked_events->event_map;
auto insert_result = unacked_events_map.insert(std::make_pair(
event_id, std::make_pair(request_uuid, render_process_id)));
DCHECK(insert_result.second) << "EventAckData: Duplicate event_id.";
}

void FinishExternalRequestOnIO(content::ServiceWorkerContext* context,
int64_t version_id,
const std::string& request_uuid,
base::OnceClosure failure_callback) {
// static
void EventAckData::FinishExternalRequestOnIO(
content::ServiceWorkerContext* context,
int render_process_id,
int64_t version_id,
int event_id,
scoped_refptr<IOEventInfo> unacked_events,
base::OnceClosure failure_callback) {
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);

std::map<int, EventInfo>& unacked_events_map = unacked_events->event_map;
auto request_info_iter = unacked_events_map.find(event_id);
if (request_info_iter == unacked_events_map.end() ||
request_info_iter->second.second != render_process_id) {
std::move(failure_callback).Run();
return;
}

std::string request_uuid = std::move(request_info_iter->second.first);
unacked_events_map.erase(request_info_iter);

if (!context->FinishedExternalRequest(version_id, request_uuid))
std::move(failure_callback).Run();
}

} // namespace

EventAckData::EventAckData() : weak_factory_(this) {}
EventAckData::EventAckData()
: unacked_events_(base::MakeRefCounted<IOEventInfo>()),
weak_factory_(this) {}
EventAckData::~EventAckData() = default;

void EventAckData::IncrementInflightEvent(
Expand All @@ -56,10 +102,8 @@ void EventAckData::IncrementInflightEvent(
content::BrowserThread::GetTaskRunnerForThread(
content::BrowserThread::IO),
FROM_HERE, context,
base::BindOnce(&StartExternalRequestOnIO, context, version_id, event_id,
base::BindOnce(&EventAckData::DidStartExternalRequest,
weak_factory_.GetWeakPtr(),
render_process_id, event_id)));
base::BindOnce(&EventAckData::StartExternalRequestOnIO, context,
render_process_id, version_id, event_id, unacked_events_));
}

void EventAckData::DecrementInflightEvent(
Expand All @@ -70,36 +114,13 @@ void EventAckData::DecrementInflightEvent(
base::OnceClosure failure_callback) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

auto request_info_iter = unacked_events_.find(event_id);
if (request_info_iter == unacked_events_.end() ||
request_info_iter->second.second != render_process_id) {
std::move(failure_callback).Run();
return;
}

std::string request_uuid = request_info_iter->second.first;
unacked_events_.erase(request_info_iter);

content::ServiceWorkerContext::RunTask(
content::BrowserThread::GetTaskRunnerForThread(
content::BrowserThread::IO),
FROM_HERE, context,
base::BindOnce(&FinishExternalRequestOnIO, context, version_id,
request_uuid, std::move(failure_callback)));
}

void EventAckData::DidStartExternalRequest(
int render_process_id,
int event_id,
std::unique_ptr<std::string> uuid_result) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
if (!uuid_result)
return;
// TODO(lazyboy): Clean up |unacked_events_| if RenderProcessHost died before
// it got a chance to ack |event_id|. This shouldn't happen in rare cases.
auto insert_result = unacked_events_.insert(std::make_pair(
event_id, std::make_pair(*uuid_result, render_process_id)));
DCHECK(insert_result.second) << "EventAckData: Duplicate event_id.";
base::BindOnce(&EventAckData::FinishExternalRequestOnIO, context,
render_process_id, version_id, event_id, unacked_events_,
std::move(failure_callback)));
}

} // namespace extensions
27 changes: 18 additions & 9 deletions extensions/browser/events/event_ack_data.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,26 @@ class EventAckData {
base::OnceClosure failure_callback);

private:
void DidStartExternalRequest(int render_process_id,
int event_id,
std::unique_ptr<std::string> uuid_result);
class IOEventInfo;

// Information about an unacked event.
// std::string - GUID for that particular event.
// int - RenderProcessHost id.
using UnackedEventInfo = std::pair<std::string, int>;
static void StartExternalRequestOnIO(
content::ServiceWorkerContext* context,
int render_process_id,
int64_t version_id,
int event_id,
scoped_refptr<EventAckData::IOEventInfo> unacked_events);

// A map of unacked event information keyed by event id.
std::map<int, UnackedEventInfo> unacked_events_;
static void FinishExternalRequestOnIO(
content::ServiceWorkerContext* context,
int render_process_id,
int64_t version_id,
int event_id,
scoped_refptr<IOEventInfo> unacked_events,
base::OnceClosure failure_callback);

// Contains map of unacked event information keyed by event id.
// Created on UI thread, but accessed only on IO thread.
scoped_refptr<IOEventInfo> unacked_events_;

base::WeakPtrFactory<EventAckData> weak_factory_;

Expand Down

0 comments on commit f571252

Please sign in to comment.