From 9e0a01fb205e7eec3624d5a813da19c2897e14ba Mon Sep 17 00:00:00 2001 From: sunil mayya Date: Thu, 13 Jun 2024 06:27:19 +0000 Subject: [PATCH] Bug 1871377 - block FetchParent from cancelling fetch operation during shutdown for keepalive requests. r=edenchuang,necko-reviewers,jesup Depends on D205720 Differential Revision: https://phabricator.services.mozilla.com/D207862 --- dom/fetch/FetchChild.cpp | 20 ++++++++++++++-- dom/fetch/FetchParent.cpp | 10 +++++++- .../meta/fetch/api/idlharness.any.js.ini | 24 ------------------- .../redirect-keepalive.https.any.js.ini | 18 -------------- .../new-window.tentative.https.window.js.ini | 12 +++++----- 5 files changed, 33 insertions(+), 51 deletions(-) delete mode 100644 testing/web-platform/meta/fetch/api/redirect/redirect-keepalive.https.any.js.ini diff --git a/dom/fetch/FetchChild.cpp b/dom/fetch/FetchChild.cpp index dee0c4b658b4..e373adb5c31a 100644 --- a/dom/fetch/FetchChild.cpp +++ b/dom/fetch/FetchChild.cpp @@ -416,7 +416,7 @@ void FetchChild::Shutdown() { mIsShutdown.Flip(); // If mWorkerRef is nullptr here, that means Recv__delete__() must be called - if (!mWorkerRef || !mIsKeepAliveRequest) { + if (!mWorkerRef) { return; } mPromise = nullptr; @@ -424,7 +424,23 @@ void FetchChild::Shutdown() { Unfollow(); mSignalImpl = nullptr; mCSPEventListener = nullptr; - Unused << SendAbortFetchOp(); + // TODO + // For workers we need to skip aborting the fetch requests if keepalive is set + // This is just a quick fix for Worker. + // Usually, we want FetchChild to get destroyed while FetchParent calls + // Senddelete(). When Worker shutdown, FetchChild must call + // FetchChild::SendAbortFetchOp() to parent, and let FetchParent decide if + // canceling the underlying fetch() or not. But currently, we have no good way + // to distinguish whether the abort is intent by script or by Worker/Window + // shutdown. So, we provide a quick fix here, which makes + // FetchChild/FetchParent live a bit longer, but corresponding resources are + // released in FetchChild::Shutdown(), so this quick fix should not cause any + // leaking. + // And we will fix it in Bug 1901082 + if (!mIsKeepAliveRequest) { + Unused << SendAbortFetchOp(); + } + mWorkerRef = nullptr; } diff --git a/dom/fetch/FetchParent.cpp b/dom/fetch/FetchParent.cpp index a627f09ad469..31f56f5d9df8 100644 --- a/dom/fetch/FetchParent.cpp +++ b/dom/fetch/FetchParent.cpp @@ -332,9 +332,17 @@ void FetchParent::ActorDestroy(ActorDestroyReason aReason) { entry.Remove(); FETCH_LOG(("FetchParent::ActorDestroy entry [%p] removed", this)); } + // mRequest can be null when FetchParent has not yet received RecvFetchOp() + if (!mRequest) { + return; + } // Force to abort the existing fetch. // Actor can be destoried by shutdown when still fetching. - RecvAbortFetchOp(); + if (mRequest->GetKeepalive()) { + FETCH_LOG(("Skip aborting fetch as the request is marked keepalive")); + } else { + RecvAbortFetchOp(); + } // mBackgroundEventTarget = nullptr; } diff --git a/testing/web-platform/meta/fetch/api/idlharness.any.js.ini b/testing/web-platform/meta/fetch/api/idlharness.any.js.ini index eff812316798..c5f38e2be9b6 100644 --- a/testing/web-platform/meta/fetch/api/idlharness.any.js.ini +++ b/testing/web-platform/meta/fetch/api/idlharness.any.js.ini @@ -2,15 +2,9 @@ [Request interface: new Request('about:blank') must inherit property "isHistoryNavigation" with the proper type] expected: FAIL - [Request interface: attribute keepalive] - expected: FAIL - [Request interface: attribute isHistoryNavigation] expected: FAIL - [Request interface: new Request('about:blank') must inherit property "keepalive" with the proper type] - expected: FAIL - [Request interface: attribute isReloadNavigation] expected: FAIL @@ -34,15 +28,9 @@ [Request interface: new Request('about:blank') must inherit property "isHistoryNavigation" with the proper type] expected: FAIL - [Request interface: attribute keepalive] - expected: FAIL - [Request interface: attribute isHistoryNavigation] expected: FAIL - [Request interface: new Request('about:blank') must inherit property "keepalive" with the proper type] - expected: FAIL - [Request interface: attribute isReloadNavigation] expected: FAIL @@ -66,15 +54,9 @@ [Request interface: new Request('about:blank') must inherit property "isHistoryNavigation" with the proper type] expected: FAIL - [Request interface: attribute keepalive] - expected: FAIL - [Request interface: attribute isHistoryNavigation] expected: FAIL - [Request interface: new Request('about:blank') must inherit property "keepalive" with the proper type] - expected: FAIL - [Request interface: attribute isReloadNavigation] expected: FAIL @@ -98,15 +80,9 @@ [Request interface: new Request('about:blank') must inherit property "isHistoryNavigation" with the proper type] expected: FAIL - [Request interface: attribute keepalive] - expected: FAIL - [Request interface: attribute isHistoryNavigation] expected: FAIL - [Request interface: new Request('about:blank') must inherit property "keepalive" with the proper type] - expected: FAIL - [Request interface: attribute isReloadNavigation] expected: FAIL diff --git a/testing/web-platform/meta/fetch/api/redirect/redirect-keepalive.https.any.js.ini b/testing/web-platform/meta/fetch/api/redirect/redirect-keepalive.https.any.js.ini deleted file mode 100644 index 3ab5cf62a1d9..000000000000 --- a/testing/web-platform/meta/fetch/api/redirect/redirect-keepalive.https.any.js.ini +++ /dev/null @@ -1,18 +0,0 @@ -[redirect-keepalive.https.any.html] - expected: - if (os == "linux") and fission and not debug and asan: [TIMEOUT, OK] - if (os == "win") and not debug and (processor == "x86"): OK - if (os == "linux") and fission and debug: [TIMEOUT, OK] - if (os == "mac") and debug: TIMEOUT - if (os == "linux") and not fission: OK - if os == "android": OK - [OK, TIMEOUT] - [[keepalive\][iframe\][load\] mixed content redirect; setting up] - expected: - if (os == "linux") and fission and not debug and asan: [TIMEOUT, PASS] - if (os == "win") and not debug and (processor == "x86"): PASS - if (os == "linux") and fission and debug: [TIMEOUT, PASS] - if (os == "mac") and debug: TIMEOUT - if (os == "linux") and not fission: PASS - if os == "android": PASS - [PASS, TIMEOUT] diff --git a/testing/web-platform/meta/fetch/fetch-later/new-window.tentative.https.window.js.ini b/testing/web-platform/meta/fetch/fetch-later/new-window.tentative.https.window.js.ini index 350ca32215b5..6853c673076f 100644 --- a/testing/web-platform/meta/fetch/fetch-later/new-window.tentative.https.window.js.ini +++ b/testing/web-platform/meta/fetch/fetch-later/new-window.tentative.https.window.js.ini @@ -6,7 +6,7 @@ [A same-origin window[target=''\][features='0'\] can trigger fetchLater.] expected: if os == "linux": [TIMEOUT, FAIL] - TIMEOUT + FAIL [A cross-origin window[target=''\][features='0'\] can trigger fetchLater.] expected: TIMEOUT @@ -17,7 +17,7 @@ [A same-origin window[target=''\][features='1'\] can trigger fetchLater.] expected: if os == "linux": [TIMEOUT, FAIL] - TIMEOUT + FAIL [A cross-origin window[target=''\][features='1'\] can trigger fetchLater.] expected: TIMEOUT @@ -28,7 +28,7 @@ [A same-origin window[target=''\][features='2'\] can trigger fetchLater.] expected: if os == "linux": [TIMEOUT, FAIL] - TIMEOUT + FAIL [A cross-origin window[target=''\][features='2'\] can trigger fetchLater.] expected: TIMEOUT @@ -39,7 +39,7 @@ [A same-origin window[target='_blank'\][features='0'\] can trigger fetchLater.] expected: if os == "linux": [TIMEOUT, FAIL] - TIMEOUT + FAIL [A cross-origin window[target='_blank'\][features='0'\] can trigger fetchLater.] expected: TIMEOUT @@ -50,7 +50,7 @@ [A same-origin window[target='_blank'\][features='1'\] can trigger fetchLater.] expected: if os == "linux": [TIMEOUT, FAIL] - TIMEOUT + FAIL [A cross-origin window[target='_blank'\][features='1'\] can trigger fetchLater.] expected: TIMEOUT @@ -61,7 +61,7 @@ [A same-origin window[target='_blank'\][features='2'\] can trigger fetchLater.] expected: if os == "linux": [TIMEOUT, FAIL] - TIMEOUT + FAIL [A cross-origin window[target='_blank'\][features='2'\] can trigger fetchLater.] expected: TIMEOUT