Commit 40a4732d authored by Hayato Ito's avatar Hayato Ito Committed by Commit Bot

Don't use std::set, which is banned in Blink, in ServiceWorkerTimeoutTimer

ServiceWorkerTimeoutTimer is using std::set, which is banned in Blink.

See the bug 1028982 for the details.

This issue is rising in https://crrev.com/c/1906452/. We are renaming
ServiceWorkerTimeoutTimer to ServiceWorkerEventQueue there, however,
the renaming triggers presubmit warnings for existing usages of std::set.

So I've decided to remove the usage of std::set in this CL.

Since there is no std::set equivalent in WTF, we have to compromise.
|id_event_map_| and |inflight_events_| are now merged, and the code
became simpler.

Regarding the performance impact, I think it's okay because UpdateStatus()
is not called frequently.

BUG: 1028982
Change-Id: I9f1c5a141d755772b5c96acb313a6c5fe3f27374
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1939161Reviewed-by: default avatarMakoto Shimazu <shimazu@chromium.org>
Reviewed-by: default avatarMatt Falkenhagen <falken@chromium.org>
Commit-Queue: Hayato Ito <hayato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719899}
parent e1b80797
...@@ -64,8 +64,8 @@ ServiceWorkerTimeoutTimer::ServiceWorkerTimeoutTimer( ...@@ -64,8 +64,8 @@ ServiceWorkerTimeoutTimer::ServiceWorkerTimeoutTimer(
ServiceWorkerTimeoutTimer::~ServiceWorkerTimeoutTimer() { ServiceWorkerTimeoutTimer::~ServiceWorkerTimeoutTimer() {
in_dtor_ = true; in_dtor_ = true;
// Abort all callbacks. // Abort all callbacks.
for (auto& event : inflight_events_) { for (auto& event : id_event_map_) {
std::move(event.abort_callback) std::move(event.value->abort_callback)
.Run(blink::mojom::ServiceWorkerEventStatus::ABORTED); .Run(blink::mojom::ServiceWorkerEventStatus::ABORTED);
} }
} }
...@@ -101,23 +101,17 @@ int ServiceWorkerTimeoutTimer::StartEventWithCustomTimeout( ...@@ -101,23 +101,17 @@ int ServiceWorkerTimeoutTimer::StartEventWithCustomTimeout(
idle_time_ = base::TimeTicks(); idle_time_ = base::TimeTicks();
const int event_id = NextEventId(); const int event_id = NextEventId();
std::set<EventInfo>::iterator iter; auto add_result = id_event_map_.insert(
bool is_inserted; event_id, std::make_unique<EventInfo>(
std::tie(iter, is_inserted) = tick_clock_->NowTicks() + timeout,
inflight_events_.emplace(event_id, tick_clock_->NowTicks() + timeout, WTF::Bind(std::move(abort_callback), event_id)));
WTF::Bind(std::move(abort_callback), event_id)); DCHECK(add_result.is_new_entry);
DCHECK(is_inserted);
id_event_map_.insert(event_id, iter);
return event_id; return event_id;
} }
void ServiceWorkerTimeoutTimer::EndEvent(int event_id) { void ServiceWorkerTimeoutTimer::EndEvent(int event_id) {
DCHECK(HasEvent(event_id)); DCHECK(HasEvent(event_id));
id_event_map_.erase(event_id);
auto iter = id_event_map_.find(event_id);
inflight_events_.erase(iter->value);
id_event_map_.erase(iter);
if (!HasInflightEvent()) if (!HasInflightEvent())
OnNoInflightEvent(); OnNoInflightEvent();
} }
...@@ -147,19 +141,22 @@ void ServiceWorkerTimeoutTimer::SetIdleTimerDelayToZero() { ...@@ -147,19 +141,22 @@ void ServiceWorkerTimeoutTimer::SetIdleTimerDelayToZero() {
void ServiceWorkerTimeoutTimer::UpdateStatus() { void ServiceWorkerTimeoutTimer::UpdateStatus() {
base::TimeTicks now = tick_clock_->NowTicks(); base::TimeTicks now = tick_clock_->NowTicks();
HashMap<int /* event_id */, std::unique_ptr<EventInfo>> new_id_event_map;
// Abort all events exceeding |kEventTimeout|. // Abort all events exceeding |kEventTimeout|.
auto iter = inflight_events_.begin(); for (auto& it : id_event_map_) {
while (iter != inflight_events_.end() && iter->expiration_time <= now) { auto& event_info = it.value;
int event_id = iter->id; if (event_info->expiration_time > now) {
base::OnceCallback<void(blink::mojom::ServiceWorkerEventStatus)> callback = new_id_event_map.insert(it.key, std::move(event_info));
std::move(iter->abort_callback); continue;
iter = inflight_events_.erase(iter); }
id_event_map_.erase(event_id); std::move(event_info->abort_callback)
std::move(callback).Run(blink::mojom::ServiceWorkerEventStatus::TIMEOUT); .Run(blink::mojom::ServiceWorkerEventStatus::TIMEOUT);
// Shut down the worker as soon as possible since the worker may have gone // Shut down the worker as soon as possible since the worker may have gone
// into bad state. // into bad state.
zero_idle_timer_delay_ = true; zero_idle_timer_delay_ = true;
} }
id_event_map_.swap(new_id_event_map);
// If the worker is now idle, set the |idle_time_| and possibly trigger the // If the worker is now idle, set the |idle_time_| and possibly trigger the
// idle callback. // idle callback.
...@@ -191,26 +188,17 @@ void ServiceWorkerTimeoutTimer::OnNoInflightEvent() { ...@@ -191,26 +188,17 @@ void ServiceWorkerTimeoutTimer::OnNoInflightEvent() {
} }
bool ServiceWorkerTimeoutTimer::HasInflightEvent() const { bool ServiceWorkerTimeoutTimer::HasInflightEvent() const {
return !inflight_events_.empty() || running_pending_tasks_ || return !id_event_map_.IsEmpty() || running_pending_tasks_ ||
num_of_stay_awake_tokens_ > 0; num_of_stay_awake_tokens_ > 0;
} }
ServiceWorkerTimeoutTimer::EventInfo::EventInfo( ServiceWorkerTimeoutTimer::EventInfo::EventInfo(
int id,
base::TimeTicks expiration_time, base::TimeTicks expiration_time,
base::OnceCallback<void(blink::mojom::ServiceWorkerEventStatus)> base::OnceCallback<void(blink::mojom::ServiceWorkerEventStatus)>
abort_callback) abort_callback)
: id(id), : expiration_time(expiration_time),
expiration_time(expiration_time),
abort_callback(std::move(abort_callback)) {} abort_callback(std::move(abort_callback)) {}
ServiceWorkerTimeoutTimer::EventInfo::~EventInfo() = default; ServiceWorkerTimeoutTimer::EventInfo::~EventInfo() = default;
bool ServiceWorkerTimeoutTimer::EventInfo::operator<(
const EventInfo& other) const {
if (expiration_time == other.expiration_time)
return id < other.id;
return expiration_time < other.expiration_time;
}
} // namespace blink } // namespace blink
...@@ -133,26 +133,18 @@ class MODULES_EXPORT ServiceWorkerTimeoutTimer { ...@@ -133,26 +133,18 @@ class MODULES_EXPORT ServiceWorkerTimeoutTimer {
bool HasInflightEvent() const; bool HasInflightEvent() const;
struct EventInfo { struct EventInfo {
EventInfo(int id, EventInfo(base::TimeTicks expiration_time,
base::TimeTicks expiration_time,
base::OnceCallback<void(mojom::blink::ServiceWorkerEventStatus)> base::OnceCallback<void(mojom::blink::ServiceWorkerEventStatus)>
abort_callback); abort_callback);
~EventInfo(); ~EventInfo();
// Compares |expiration_time|, or |id| if |expiration_time| is the same.
bool operator<(const EventInfo& other) const;
const int id;
const base::TimeTicks expiration_time; const base::TimeTicks expiration_time;
mutable base::OnceCallback<void(mojom::blink::ServiceWorkerEventStatus)> base::OnceCallback<void(mojom::blink::ServiceWorkerEventStatus)>
abort_callback; abort_callback;
}; };
// For long standing event timeouts. Ordered by expiration time. // For long standing event timeouts. This is used to look up an EventInfo
std::set<EventInfo> inflight_events_; // by event id.
HashMap<int /* event_id */, std::unique_ptr<EventInfo>> id_event_map_;
// For long standing event timeouts. This is used to look up an event in
// |inflight_events_| by its id.
HashMap<int /* event_id */, std::set<EventInfo>::iterator> id_event_map_;
// For idle timeouts. The time the service worker started being considered // For idle timeouts. The time the service worker started being considered
// idle. This time is null if there are any inflight events. // idle. This time is null if there are any inflight events.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment