Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create resource timing entries for fetch network errors #30970

Merged
merged 9 commits into from
Dec 1, 2021

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Sep 26, 2021

An initial tests for whatwg/fetch#1215

Added test cases for several network error scenarios. The test cases verify that a resource timing entry is created for those resources, without the "internal" timing info (redirects, networking), as if the resource was cross-origin no-cors and lacked TAO.

The error test cases include:

  • CORS fail (which is also covered in existing TAO tests)
  • about URLs
  • invalid blobs
  • Posting to blobs
  • Only-if-cached header for a fresh resource
  • Abort HTTP/blob request
  • Invalid scheme
  • Too many redirects
  • Invalid certificate
  • Host not found
  • CSP blocked

These tests come in conjunction with whatwg/fetch#1311

Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!! A few comments

resource-timing/csp-block-fetch.js.headers Outdated Show resolved Hide resolved
resource-timing/resources/entry-invariants.js Outdated Show resolved Hide resolved
resource-timing/resources/entry-invariants.js Outdated Show resolved Hide resolved
Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@noamr
Copy link
Contributor Author

noamr commented Sep 27, 2021

LGTM

Thanks!
Should probably wait until the spec change is approved and merge at the same time.

@noamr noamr force-pushed the fetch-network-error branch from 13f67ce to c2496a3 Compare November 14, 2021 14:05
@noamr
Copy link
Contributor Author

noamr commented Nov 14, 2021

Modified test to include only 5 scenarios for now:

  • CORS failure
  • DNS failure
  • Timeout (IP address not found)
  • Only-if-cached-resource that wasn't cached
  • Too many redirects

Currently Chrome & Firefox are inconsistent around which of these create a resource timing entry.
Safari does not create RT entries for network errors.

@noamr
Copy link
Contributor Author

noamr commented Nov 14, 2021

This actually catches a Firefox bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1523275

Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % nit and suggestion for another test

@noamr
Copy link
Contributor Author

noamr commented Nov 22, 2021

The Firefox failures are legit, e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1523275. How does this get merged when it fails firefox nightly?

@noamr
Copy link
Contributor Author

noamr commented Nov 22, 2021

LGTM % nit and suggestion for another test

Added a test for mixed content.

@noamr noamr merged commit 029d8d6 into master Dec 1, 2021
@noamr noamr deleted the fetch-network-error branch December 1, 2021 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants