don't freeze urlList for opaque filtered responses #4656
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4656 +/- ##
==========================================
+ Coverage 92.84% 92.88% +0.03%
==========================================
Files 106 106
Lines 33272 33272
==========================================
+ Hits 30892 30904 +12
+ Misses 2380 2368 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I believe it was wrong to classify status 0 as a network error, but I'm unsure which way is correct. diff --git a/lib/web/fetch/index.js b/lib/web/fetch/index.js
index 0f0a94d3..da58a0dc 100644
--- a/lib/web/fetch/index.js
+++ b/lib/web/fetch/index.js
@@ -646,7 +646,7 @@ async function mainFetch (fetchParams, recursive) {
// 13. If response is not a network error and response is not a filtered
// response, then:
- if (response.status !== 0 && !response.internalResponse) {
+ if (response.type !== 'error' && !response.internalResponse) {
// If request’s response tainting is "cors", then:
if (request.responseTainting === 'cors') {
// 1. Let headerNames be the result of extracting header list values
@@ -676,8 +676,7 @@ async function mainFetch (fetchParams, recursive) {
// 14. Let internalResponse be response, if response is a network error,
// and response’s internal response otherwise.
- let internalResponse =
- response.status === 0 ? response : response.internalResponse
+ let internalResponse = response.type === 'error' ? response : (response.internalResponse ?? response)
// 15. If internalResponse’s URL list is empty, then set it to a clone of
// request’s URL list.
@@ -717,7 +716,7 @@ async function mainFetch (fetchParams, recursive) {
// set internalResponse’s body to null and disregard any enqueuing toward
// it (if any).
if (
- response.status !== 0 &&
+ response.type !== 'error' &&
(request.method === 'HEAD' ||
request.method === 'CONNECT' ||
nullBodyStatus.includes(internalResponse.status))
|
|
both are correct |
I did notice what you are saying when working on it, but it's not relevant to the bug so I didn't bother. We are technically detecting network errors incorrectly, but if it's not causing bugs... 🤷 |
fixes #4647
the offending line was added in 9e5316c but I don't see any rationale for why urlList was frozen, the spec doesn't mention freezing the list.
the issue was caused by freezing the list which would prevent us from pushing to it which would swallow the error