Skip to content

Commit

Permalink
ServiceWorker: Start the timer immediately after queuing an event
Browse files Browse the repository at this point in the history 
This CL tries to re-land the CL (1) which was reverted by the issue (2).

(1) https://chromium-review.googlesource.com/c/chromium/src/+/2423698
(2) https://crbug.com/1134552

This CL mainly does the following things:
1) Make the timer start immediately after an event is enqueued, and
erases the event from the queue when it has not started to run yet
and the timeout happens.
2) Update kUpdateInterval because the minimum possible timeout is 10
seconds for offline events.
3) Set a callback to a hashmap (e.g. fetch_event_callbacks_) before
calling EnqueueNormal/Pending/Offline() in SWGlobalScope.

The change 3) is needed to fix the issue (2). An abort callback is
possibly called before an event is started to run due to the change 1).
However, previously, an event_id and a callback were set to a hashmap
when a start_callback was called, so if an event was aborted before it
starts, a hashmap didn't have a callback yet.

Bug: 965802
Change-Id: Ic177e8aaab25527b3fa0b7a07aae51fca484bf85
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2460747
Commit-Queue: Asami Doi <asamidoi@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#817385}
  • Loading branch information
@d0iasm
d0iasm authored and Commit Bot committed Oct 15, 2020
1 parent 6757c99 commit afa39cc
Show file tree
Hide file tree
Showing 5 changed files with 286 additions and 151 deletions.
95 changes: 59 additions & 36 deletions third_party/blink/renderer/modules/service_worker/service_worker_event_queue.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

#include "third_party/blink/renderer/modules/service_worker/service_worker_event_queue.h"

#include "base/atomic_sequence_num.h"
#include "base/bind.h"
#include "base/stl_util.h"
#include "base/time/default_tick_clock.h"
Expand All @@ -14,19 +13,6 @@

namespace blink {

namespace {

int NextEventId() {
// Event id should not start from zero since HashMap in Blink requires
// non-zero keys.
static base::AtomicSequenceNumber s_event_id_sequence;
int next_event_id = s_event_id_sequence.GetNext() + 1;
CHECK_LT(next_event_id, std::numeric_limits<int>::max());
return next_event_id;
}

} // namespace

// static
constexpr base::TimeDelta ServiceWorkerEventQueue::kEventTimeout;
constexpr base::TimeDelta ServiceWorkerEventQueue::kUpdateInterval;
Expand Down Expand Up @@ -71,7 +57,6 @@ ServiceWorkerEventQueue::ServiceWorkerEventQueue(
tick_clock_(tick_clock) {}

ServiceWorkerEventQueue::~ServiceWorkerEventQueue() {
in_dtor_ = true;
// Abort all callbacks.
for (auto& event : id_event_map_) {
std::move(event.value->abort_callback)
Expand All @@ -91,30 +76,34 @@ void ServiceWorkerEventQueue::Start() {
}

void ServiceWorkerEventQueue::EnqueueNormal(
int event_id,
StartCallback start_callback,
AbortCallback abort_callback,
base::Optional<base::TimeDelta> custom_timeout) {
EnqueueEvent(std::make_unique<Event>(
Event::Type::Normal, std::move(start_callback), std::move(abort_callback),
std::move(custom_timeout)));
event_id, Event::Type::Normal, std::move(start_callback),
std::move(abort_callback), std::move(custom_timeout)));
}

void ServiceWorkerEventQueue::EnqueuePending(
int event_id,
StartCallback start_callback,
AbortCallback abort_callback,
base::Optional<base::TimeDelta> custom_timeout) {
EnqueueEvent(std::make_unique<Event>(
Event::Type::Pending, std::move(start_callback),
event_id, Event::Type::Pending, std::move(start_callback),
std::move(abort_callback), std::move(custom_timeout)));
}

void ServiceWorkerEventQueue::EnqueueOffline(
int event_id,
StartCallback start_callback,
AbortCallback abort_callback,
base::Optional<base::TimeDelta> custom_timeout) {
EnqueueEvent(std::make_unique<ServiceWorkerEventQueue::Event>(
ServiceWorkerEventQueue::Event::Type::Offline, std::move(start_callback),
std::move(abort_callback), std::move(custom_timeout)));
event_id, ServiceWorkerEventQueue::Event::Type::Offline,
std::move(start_callback), std::move(abort_callback),
std::move(custom_timeout)));
}

bool ServiceWorkerEventQueue::CanStartEvent(const Event& event) const {
Expand All @@ -127,9 +116,21 @@ bool ServiceWorkerEventQueue::CanStartEvent(const Event& event) const {

void ServiceWorkerEventQueue::EnqueueEvent(std::unique_ptr<Event> event) {
DCHECK(event->type != Event::Type::Pending || did_idle_timeout());
DCHECK(!HasEvent(event->event_id));
DCHECK(!HasEventInQueue(event->event_id));

bool can_start_processing_events =
!processing_events_ && event->type != Event::Type::Pending;
queue_.emplace_back(std::move(event));

// Start counting the timer when an event is enqueued.
id_event_map_.insert(
event->event_id,
std::make_unique<EventInfo>(
tick_clock_->NowTicks() +
event->custom_timeout.value_or(kEventTimeout),
WTF::Bind(std::move(event->abort_callback), event->event_id)));

queue_.emplace(event->event_id, std::move(event));

if (!can_start_processing_events)
return;
Expand All @@ -141,8 +142,11 @@ void ServiceWorkerEventQueue::EnqueueEvent(std::unique_ptr<Event> event) {
void ServiceWorkerEventQueue::ProcessEvents() {
DCHECK(!processing_events_);
processing_events_ = true;
while (!queue_.IsEmpty() && CanStartEvent(*queue_.front())) {
StartEvent(queue_.TakeFirst());
while (!queue_.empty() && CanStartEvent(*queue_.begin()->second)) {
int event_id = queue_.begin()->first;
std::unique_ptr<Event> event = std::move(queue_.begin()->second);
queue_.erase(queue_.begin());
StartEvent(event_id, std::move(event));
}
processing_events_ = false;

Expand All @@ -154,16 +158,10 @@ void ServiceWorkerEventQueue::ProcessEvents() {
OnNoInflightEvent();
}

void ServiceWorkerEventQueue::StartEvent(std::unique_ptr<Event> event) {
DCHECK(CanStartEvent(*event));
void ServiceWorkerEventQueue::StartEvent(int event_id,
std::unique_ptr<Event> event) {
DCHECK(HasEvent(event_id));
running_offline_events_ = event->type == Event::Type::Offline;
const int event_id = NextEventId();
DCHECK(!HasEvent(event_id));
id_event_map_.insert(
event_id, std::make_unique<EventInfo>(
tick_clock_->NowTicks() +
event->custom_timeout.value_or(kEventTimeout),
WTF::Bind(std::move(event->abort_callback), event_id)));
if (before_start_event_callback_)
before_start_event_callback_.Run(event->type == Event::Type::Offline);
std::move(event->start_callback).Run(event_id);
Expand All @@ -183,6 +181,10 @@ bool ServiceWorkerEventQueue::HasEvent(int event_id) const {
return id_event_map_.find(event_id) != id_event_map_.end();
}

bool ServiceWorkerEventQueue::HasEventInQueue(int event_id) const {
return queue_.find(event_id) != queue_.end();
}

std::unique_ptr<ServiceWorkerEventQueue::StayAwakeToken>
ServiceWorkerEventQueue::CreateStayAwakeToken() {
return std::make_unique<ServiceWorkerEventQueue::StayAwakeToken>(
Expand Down Expand Up @@ -226,21 +228,34 @@ void ServiceWorkerEventQueue::SetIdleDelay(base::TimeDelta idle_delay) {
void ServiceWorkerEventQueue::UpdateStatus() {
base::TimeTicks now = tick_clock_->NowTicks();

// Construct a new map because WTF::HashMap doesn't support deleting elements
// while iterating.
HashMap<int /* event_id */, std::unique_ptr<EventInfo>> new_id_event_map;

bool should_idle_delay_to_be_zero = false;
// Abort all events exceeding |kEventTimeout|.

// Time out all events exceeding `kEventTimeout`.
for (auto& it : id_event_map_) {
// Check if the event has timed out.
auto& event_info = it.value;
if (event_info->expiration_time > now) {
new_id_event_map.insert(it.key, std::move(event_info));
continue;
}

// The event might still be queued when it timed out. Remove it from the
// queue if so.
queue_.erase(it.key);

// Run the abort callback.
std::move(event_info->abort_callback)
.Run(blink::mojom::ServiceWorkerEventStatus::TIMEOUT);

should_idle_delay_to_be_zero = true;
}
id_event_map_.swap(new_id_event_map);

// Set idle delay to zero if needed.
if (should_idle_delay_to_be_zero) {
// Inflight events might be timed out and there might be no inflight event
// at this point.
Expand Down Expand Up @@ -280,7 +295,7 @@ void ServiceWorkerEventQueue::OnNoInflightEvent() {
running_offline_events_ = false;
// There might be events in the queue because offline (or non-offline) events
// can be enqueued during running non-offline (or offline) events.
if (!queue_.IsEmpty()) {
if (!queue_.empty()) {
ProcessEvents();
return;
}
Expand All @@ -289,7 +304,8 @@ void ServiceWorkerEventQueue::OnNoInflightEvent() {
}

bool ServiceWorkerEventQueue::HasInflightEvent() const {
return !id_event_map_.IsEmpty() || num_of_stay_awake_tokens_ > 0;
return id_event_map_.size() - queue_.size() > 0 ||
num_of_stay_awake_tokens_ > 0;
}

void ServiceWorkerEventQueue::ResetIdleTimeout() {
Expand All @@ -302,12 +318,19 @@ bool ServiceWorkerEventQueue::HasScheduledIdleCallback() const {
return idle_callback_handle_.IsActive();
}

int ServiceWorkerEventQueue::NextEventId() {
CHECK_LT(next_event_id_, std::numeric_limits<int>::max());
return next_event_id_++;
}

ServiceWorkerEventQueue::Event::Event(
int event_id,
ServiceWorkerEventQueue::Event::Type type,
StartCallback start_callback,
AbortCallback abort_callback,
base::Optional<base::TimeDelta> custom_timeout)
: type(type),
: event_id(event_id),
type(type),
start_callback(std::move(start_callback)),
abort_callback(std::move(abort_callback)),
custom_timeout(custom_timeout) {}
Expand Down
33 changes: 24 additions & 9 deletions third_party/blink/renderer/modules/service_worker/service_worker_event_queue.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,23 +90,29 @@ class MODULES_EXPORT ServiceWorkerEventQueue {

// Enqueues a Normal event. See ServiceWorkerEventQueue::Event to know the
// meaning of each parameter.
void EnqueueNormal(StartCallback start_callback,
void EnqueueNormal(int event_id,
StartCallback start_callback,
AbortCallback abort_callback,
base::Optional<base::TimeDelta> custom_timeout);

// Similar to EnqueueNormal(), but enqueues a Pending event.
void EnqueuePending(StartCallback start_callback,
void EnqueuePending(int event_id,
StartCallback start_callback,
AbortCallback abort_callback,
base::Optional<base::TimeDelta> custom_timeout);

// Similar to EnqueueNormal(), but enqueues an Offline event.
void EnqueueOffline(StartCallback start_callback,
void EnqueueOffline(int event_id,
StartCallback start_callback,
AbortCallback abort_callback,
base::Optional<base::TimeDelta> custom_timeout);

// Returns true if |event_id| was started and hasn't ended.
// Returns true if |event_id| was enqueued and hasn't ended.
bool HasEvent(int event_id) const;

// Returns true if |event_id| was enqueued and hasn't started.
bool HasEventInQueue(int event_id) const;

// Creates a StayAwakeToken to ensure that the idle callback won't be
// triggered while any of these are alive.
std::unique_ptr<StayAwakeToken> CreateStayAwakeToken();
Expand All @@ -121,14 +127,17 @@ class MODULES_EXPORT ServiceWorkerEventQueue {
// false again when StartEvent() is called.
bool did_idle_timeout() const { return did_idle_timeout_; }

// Returns the next event id, which is a monotonically increasing number.
int NextEventId();

// Duration of the long standing event timeout since StartEvent() has been
// called.
static constexpr base::TimeDelta kEventTimeout =
base::TimeDelta::FromMinutes(5);
// ServiceWorkerEventQueue periodically updates the timeout state by
// kUpdateInterval.
static constexpr base::TimeDelta kUpdateInterval =
base::TimeDelta::FromSeconds(30);
base::TimeDelta::FromSeconds(10);

private:
// Represents an event dispatch, which can be queued into |queue_|.
Expand All @@ -153,11 +162,13 @@ class MODULES_EXPORT ServiceWorkerEventQueue {
Offline,
};

Event(Type type,
Event(int event_id,
Type type,
StartCallback start_callback,
AbortCallback abort_callback,
base::Optional<base::TimeDelta> custom_timeout);
~Event();
const int event_id;
Type type;
// Callback which is run when the event queue starts this event. The
// callback receives |event_id|. When an event finishes,
Expand All @@ -177,7 +188,7 @@ class MODULES_EXPORT ServiceWorkerEventQueue {
bool CanStartEvent(const Event& event) const;

// Starts a single event.
void StartEvent(std::unique_ptr<Event> event);
void StartEvent(int event_id, std::unique_ptr<Event> event);

// Updates the internal states and fires the event timeout callbacks if any.
// TODO(shimazu): re-implement it by delayed tasks and cancelable callbacks.
Expand Down Expand Up @@ -246,7 +257,9 @@ class MODULES_EXPORT ServiceWorkerEventQueue {
bool did_idle_timeout_ = false;

// Event queue to where all events are enqueued.
Deque<std::unique_ptr<Event>> queue_;
// We use std::map as a task queue because it's ordered by the `event_id`
// and the entries can be effectively erased in random order.
std::map<int /* event_id */, std::unique_ptr<Event>> queue_;

// Set to true during running ProcessEvents(). This is used for avoiding to
// invoke |idle_callback_| or to re-enter ProcessEvents() when calling
Expand All @@ -265,7 +278,9 @@ class MODULES_EXPORT ServiceWorkerEventQueue {
// |tick_clock_| outlives |this|.
const base::TickClock* const tick_clock_;

bool in_dtor_ = false;
// Monotonically increasing number. Event id should not start from zero since
// HashMap in Blink requires non-zero keys.
int next_event_id_ = 1;

base::WeakPtrFactory<ServiceWorkerEventQueue> weak_factory_{this};
};
Expand Down
Loading

0 comments on commit afa39cc

Please sign in to comment.