Skip to content

Commit

Permalink
chore: cherry-pick 3cbd5973d704 from chromium (#35002)
Browse files Browse the repository at this point in the history
* chore: [18-x-y] cherry-pick 3cbd5973d704 from chromium

* chore: update patches

Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
  • Loading branch information
nornagon and patchup[bot] committed Jul 21, 2022
1 parent b1093f4 commit 5059502
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 0 deletions.
1 change: 1 addition & 0 deletions patches/chromium/.patches
Expand Up @@ -132,3 +132,4 @@ posix_replace_doubleforkandexec_with_forkandspawn.patch
cherry-pick-f427936d32db.patch
cherry-pick-22c61cfae5d1.patch
remove_default_window_title.patch
cherry-pick-3cbd5973d704.patch
102 changes: 102 additions & 0 deletions patches/chromium/cherry-pick-3cbd5973d704.patch
@@ -0,0 +1,102 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Date: Mon, 18 Jul 2022 20:44:41 +0000
Subject: Keep refptr to ServiceWorkerVersion in MaybeTimeoutRequest

The callback in ServiceWorkerVersion::MaybeTimeoutRequest may reduce the
reference to the version object, which can be the last reference to it.
In thet case, the object can be destroyed and the `inflight_requests_`
field access after the callback become invalid.
This CL keeps this to avoid the object destruction.

(cherry picked from commit 5926fa916d9ad53c77e31ee757e1979275d7466c)

Bug: 1339844
Change-Id: I6564627bad0527dea007ca73261c5636dab56755
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3739330
Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Sergei Glazunov <glazunov@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1023475}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3766241
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Srinivas Sista <srinivassista@chromium.org>
Cr-Commit-Position: refs/branch-heads/5005@{#1263}
Cr-Branched-From: 5b4d9450fee01f821b6400e947b3839727643a71-refs/heads/main@{#992738}

diff --git a/content/browser/service_worker/service_worker_version.cc b/content/browser/service_worker/service_worker_version.cc
index 0b4b32d13db8958f579085a41bc5e20b26543f61..5797e3f954ae0769a91c8f259e19e50e2bf31a94 100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -1993,18 +1993,17 @@ void ServiceWorkerVersion::OnTimeoutTimer() {

MarkIfStale();

+ // Global `this` protecter.
+ // callbacks initiated by this function sometimes reduce refcnt to 0
+ // to make this instance freed.
+ scoped_refptr<ServiceWorkerVersion> protect_this(this);
+
// Stopping the worker hasn't finished within a certain period.
if (GetTickDuration(stop_time_) > kStopWorkerTimeout) {
DCHECK_EQ(EmbeddedWorkerStatus::STOPPING, running_status());
ReportError(blink::ServiceWorkerStatusCode::kErrorTimeout,
"DETACH_STALLED_IN_STOPPING");

- // Detach the worker. Remove |this| as a listener first; otherwise
- // OnStoppedInternal might try to restart before the new worker
- // is created. Also, protect |this|, since swapping out the
- // EmbeddedWorkerInstance could destroy our ServiceWorkerHost which could in
- // turn destroy |this|.
- scoped_refptr<ServiceWorkerVersion> protect_this(this);
embedded_worker_->RemoveObserver(this);
embedded_worker_->Detach();
embedded_worker_ = std::make_unique<EmbeddedWorkerInstance>(this);
@@ -2031,7 +2030,6 @@ void ServiceWorkerVersion::OnTimeoutTimer() {
DCHECK(running_status() == EmbeddedWorkerStatus::STARTING ||
running_status() == EmbeddedWorkerStatus::STOPPING)
<< static_cast<int>(running_status());
- scoped_refptr<ServiceWorkerVersion> protect(this);
FinishStartWorker(blink::ServiceWorkerStatusCode::kErrorTimeout);
if (running_status() == EmbeddedWorkerStatus::STARTING)
embedded_worker_->Stop();
@@ -2040,17 +2038,22 @@ void ServiceWorkerVersion::OnTimeoutTimer() {

// Requests have not finished before their expiration.
bool stop_for_timeout = false;
- auto timeout_iter = request_timeouts_.begin();
- while (timeout_iter != request_timeouts_.end()) {
+ std::set<InflightRequestTimeoutInfo> request_timeouts;
+ request_timeouts.swap(request_timeouts_);
+ auto timeout_iter = request_timeouts.begin();
+ while (timeout_iter != request_timeouts.end()) {
const InflightRequestTimeoutInfo& info = *timeout_iter;
- if (!RequestExpired(info.expiration))
+ if (!RequestExpired(info.expiration)) {
break;
+ }
if (MaybeTimeoutRequest(info)) {
stop_for_timeout =
stop_for_timeout || info.timeout_behavior == KILL_ON_TIMEOUT;
}
- timeout_iter = request_timeouts_.erase(timeout_iter);
+ timeout_iter = request_timeouts.erase(timeout_iter);
}
+ DCHECK(request_timeouts_.empty());
+ request_timeouts_.swap(request_timeouts);
if (stop_for_timeout && running_status() != EmbeddedWorkerStatus::STOPPING)
embedded_worker_->Stop();

diff --git a/content/browser/service_worker/service_worker_version.h b/content/browser/service_worker/service_worker_version.h
index dcf0dfaf515f8058438d727b30fb4c508a7211a9..a88bc2513f988a00b538baa596a1e713c2600cf8 100644
--- a/content/browser/service_worker/service_worker_version.h
+++ b/content/browser/service_worker/service_worker_version.h
@@ -863,6 +863,8 @@ class CONTENT_EXPORT ServiceWorkerVersion
bool is_browser_startup_complete,
blink::ServiceWorkerStatusCode status);

+ // The caller of MaybeTimeoutRequest must increase reference count of |this|
+ // to avoid it deleted during the execution.
bool MaybeTimeoutRequest(const InflightRequestTimeoutInfo& info);
void SetAllRequestExpirations(const base::TimeTicks& expiration);

0 comments on commit 5059502

Please sign in to comment.