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

Clarify which network errors create a resource timing entry #1215

Closed
Tracked by #38
noamr opened this issue Apr 13, 2021 · 21 comments · Fixed by #1311
Closed
Tracked by #38

Clarify which network errors create a resource timing entry #1215

noamr opened this issue Apr 13, 2021 · 21 comments · Fixed by #1311
Labels
topic: timing Issues and PR that touch on the infrastructure that is used by ResourceTiming, NavigationTiming, etc

Comments

@noamr
Copy link
Contributor

noamr commented Apr 13, 2021

It's not clear from the spec/implementation which errors should or shouldn't create an RT entry.
CORS errors explicitly don't, though in the original Resource Timing spec they should create an RT entry. However, the discrepancy between different error types might create a case where it could be detected whether an error is of CORS nature, which should be avoided.

See #1202 (review)

@noamr
Copy link
Contributor Author

noamr commented Apr 13, 2021

See w3c/resource-timing#12

@noamr
Copy link
Contributor Author

noamr commented Apr 15, 2021

See comment: #1202 (comment)

@noamr
Copy link
Contributor Author

noamr commented Apr 15, 2021

One simplified option would be that the caller is responsible to report the RT entry in case of a network/CORS error.
The caller (e.g. a script tag) knows when the call to FETCH was made and when it has received an error, so it could report those timing as the start/end time to resource timing, without exposing anything about the connection info.

Not sure if we want to expose connection info and other things when there was a network error, probably not.

@annevk
Copy link
Member

annevk commented Apr 15, 2021

Yeah, if we report network errors we want to act as if they are cross-origin and lack TAO. We could store the relevant information on the network error for the caller (it's a type of response after all, albeit rather special). That's my option 1.

Option 2 would be reporting all responses that are not network errors.

Option 3 would be reporting all responses that the caller considers actionable (i.e., would exclude non-2xx in case of <script>).

They are not in a preference order. I don't really know what would help web developers the most.

Whichever option we decide to go with should not provide a new information channel.

cc @yoavweiss @npm1 @rniwa @bdekoz @sefeng211

@annevk
Copy link
Member

annevk commented Apr 15, 2021

I noticed another problem, due to network errors having an empty URL list, finalize and report timing always returns early. So as currently specified option 1 does not work.

@npm1
Copy link
Contributor

npm1 commented Apr 15, 2021

We briefly discussed this in the WebPerf WG call today, it wasn't clear to me if there's a use case for entries from network errors. Also @nicjansma pointed out that there's already overlap with the Reporting API.

@annevk
Copy link
Member

annevk commented Apr 19, 2021

You mean with https://w3c.github.io/network-error-logging/? That's true. If everyone is fine to align on no entries for network errors, let's do that.

annevk pushed a commit that referenced this issue Apr 21, 2021
And hand processResponseDone a response.

Resource Timing PR: w3c/resource-timing#266.

Closes #1201. Follow-up: #1215.
@noamr
Copy link
Contributor Author

noamr commented Apr 22, 2021

@yoavweiss what's needed to reach consensus/conclusions on this?

@yoavweiss yoavweiss added the topic: timing Issues and PR that touch on the infrastructure that is used by ResourceTiming, NavigationTiming, etc label May 11, 2021
@nicjansma
Copy link

nicjansma commented May 13, 2021

While there is overlap with the Reporting API (Network Error Logging), I think there's value in keeping network-level errors, since the ergonomics and delivery are different (e.g. out-of-band reports). If we're reporting network-level errors via Reporting API, is there still concern about surfacing them through ResourceTiming as well (which is what most browsers are doing today)?

This is a lot of what the issue w3c/resource-timing#12 was about, getting consistency on network-level and non-200 errors among the browsers.

I just did a quick refresh to see where browsers are today, and except for Safari, network-level errors are reported everywhere else:

w3c/resource-timing#12 (comment)

What we have today is aligned closer with @annevk 's Option 1, "... to act as if they are cross-origin and lack TAO". The entries exist (except in Safari), but have all breakdown timestamps zero'd. Just fetchStart/responseEnd/duration are set.

@noamr
Copy link
Contributor Author

noamr commented May 18, 2021

While there is overlap with the Reporting API (Network Error Logging), I think there's value in keeping network-level errors, since the ergonomics and delivery are different (e.g. out-of-band reports). If we're reporting network-level errors via Reporting API, is there still concern about surfacing them through ResourceTiming as well (which is what most browsers are doing today)?

This is a lot of what the issue w3c/resource-timing#12 was about, getting consistency on network-level and non-200 errors among the browsers.

I just did a quick refresh to see where browsers are today, and except for Safari, network-level errors are reported everywhere else:

w3c/resource-timing#12 (comment)

What we have today is aligned closer with @annevk 's Option 1, "... to act as if they are cross-origin and lack TAO". The entries exist (except in Safari), but have all breakdown timestamps zero'd. Just fetchStart/responseEnd/duration are set.

I also find that option the most sensible.

@yoavweiss
Copy link
Collaborator

^^ @achristensen07

@achristensen07
Copy link

Anne's suggestion seems reasonable, but that still makes it easier to distinguish a fast network error from a timeout. Relatedly, should we also add an entry as if it were cross-origin and lacking TAO if there is no network error but the request times out?
This is being tracked in WebKit as https://bugs.webkit.org/show_bug.cgi?id=193902

@yoavweiss
Copy link
Collaborator

Relatedly, should we also add an entry as if it were cross-origin and lacking TAO if there is no network error but the request times out?

@achristensen07 - We could add an entry in such a case, but it won't be difficult to guess it's a timeout, would it?

Yeah, if we report network errors we want to act as if they are cross-origin and lack TAO

@annevk - that sounds overall reasonable to me as well, but I'd like for us to spell out the reasoning. Since cross-origin network errors lack TAO anyway, I'm guessing you also want same-origin network errors to not expose details. Is that because you're concerned they may expose details on the user's network? Or some other reason?

@annevk
Copy link
Member

annevk commented May 31, 2021

Yeah, basically until you have established a TLS connection, you don't really know if it's same-origin or not. And post-connection network errors are always about policy enforcement and we do not want to surface that to the caller either.

I think a system timeout should be the same as a network error, information-wise.

@noamr
Copy link
Contributor Author

noamr commented Sep 22, 2021

Have we reached consensus on this, @yoavweiss?

@yoavweiss
Copy link
Collaborator

I believe we have agreement to expose all network errors as if they are cross-origin with no TAO.

@noamr
Copy link
Contributor Author

noamr commented Sep 23, 2021

Closing the issue, as it doesn't have direct impact on the FETCH spec at this point, but rather on its callers.

@noamr noamr closed this as completed Sep 23, 2021
@annevk
Copy link
Member

annevk commented Sep 23, 2021

I don't think that's correct. We'd have to at least start storing some information on network errors related to timing.

@annevk annevk reopened this Sep 23, 2021
@noamr
Copy link
Contributor Author

noamr commented Sep 23, 2021

I don't think that's correct. We'd have to at least start storing some information on network errors related to timing.

I don't believe so. The start time is stored on the request, and the end time is the time of calling the report function. But we can leave this open until we have the first reporter of network error (I guess that would be the fetch API, which we can track here).

@annevk
Copy link
Member

annevk commented Sep 23, 2021

I'm pretty sure we don't store anything about timing on requests, except for the timing allow failed flag bookkeeping detail.

noamr added a commit to web-platform-tests/wpt that referenced this issue Sep 26, 2021
noamr added a commit to noamr/fetch that referenced this issue Sep 28, 2021
Also, report timing info for network errors for the fetch() APIs.
The timing info attached to a network error is always "opaque",
containing only start/end time and the original request URL.

This change currently only applies to the fetch API, and should be
applied to the different callers of FETCCH as part of
[this](whatwg/html#6542) and
[this](whatwg/xhr#319) work.

Closes whatwg#1215
annevk pushed a commit that referenced this issue Sep 30, 2021
noamr added a commit to noamr/fetch that referenced this issue Sep 30, 2021
Set an opaque timing info and the original request URL to the error
resposne, and call the processResponseDone callback to allow the caller
to report it. This should already be the case for reporting network
errors in the fetch() API.

Closes whatwg#1215
noamr added a commit to noamr/fetch that referenced this issue Sep 30, 2021
Set an opaque timing info and the original request URL to the error
resposne, and call the processResponseDone callback to allow the caller
to report it. This should already be the case for reporting network
errors in the fetch() API.

Closes whatwg#1215
noamr added a commit to noamr/fetch that referenced this issue Oct 10, 2021
Set an opaque timing info and the original request URL to the error
resposne, and call the processResponseDone callback to allow the caller
to report it. This should already be the case for reporting network
errors in the fetch() API.

Closes whatwg#1215
noamr added a commit to noamr/fetch that referenced this issue Oct 15, 2021
Set an opaque timing info and the original request URL to the error
resposne, and call the processResponseDone callback to allow the caller
to report it. This should already be the case for reporting network
errors in the fetch() API.

Closes whatwg#1215
noamr added a commit to noamr/fetch that referenced this issue Oct 27, 2021
Set an opaque timing info and the original request URL to the error
resposne, and call the processResponseDone callback to allow the caller
to report it. This should already be the case for reporting network
errors in the fetch() API.

Closes whatwg#1215

Set network error timing info and URL at the start of fetch finale

Rearrange fetch finale step

Make sure we don't report network errors twice

Use queues for synchronization
noamr added a commit to web-platform-tests/wpt that referenced this issue Nov 14, 2021
noamr added a commit to noamr/fetch that referenced this issue Nov 22, 2021
Set an opaque timing info and the original request URL to the error
resposne, and call the processResponseDone callback to allow the caller
to report it. This should already be the case for reporting network
errors in the fetch() API.

Closes whatwg#1215

Set network error timing info and URL at the start of fetch finale

Rearrange fetch finale step

Make sure we don't report network errors twice

Use queues for synchronization
noamr added a commit to web-platform-tests/wpt that referenced this issue Dec 1, 2021
* Create resource timing entries for fetch network errors

An initial test for whatwg/fetch#1215

* Remove a few ambiguous errors

* Remove timeout scenario

* Remove unnecessary lines

* Added mixed content case

* Try to make test less flaky

* Make test not flaky

* Use attribute_test

* lint
noamr added a commit to noamr/fetch that referenced this issue Dec 3, 2021
Set an opaque timing info and the original request URL to the error
resposne, and call the processResponseDone callback to allow the caller
to report it. This should already be the case for reporting network
errors in the fetch() API.

Closes whatwg#1215

Set network error timing info and URL at the start of fetch finale

Rearrange fetch finale step

Make sure we don't report network errors twice

Use queues for synchronization
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Dec 9, 2021
… network errors, a=testonly

Automatic update from web-platform-tests
Create resource timing entries for fetch network errors (#30970)

* Create resource timing entries for fetch network errors

An initial test for whatwg/fetch#1215

* Remove a few ambiguous errors

* Remove timeout scenario

* Remove unnecessary lines

* Added mixed content case

* Try to make test less flaky

* Make test not flaky

* Use attribute_test

* lint
--

wpt-commits: 029d8d6672ceaed11b0cfef841621ece520a78ec
wpt-pr: 30970
aosmond pushed a commit to aosmond/gecko that referenced this issue Dec 18, 2021
… network errors, a=testonly

Automatic update from web-platform-tests
Create resource timing entries for fetch network errors (#30970)

* Create resource timing entries for fetch network errors

An initial test for whatwg/fetch#1215

* Remove a few ambiguous errors

* Remove timeout scenario

* Remove unnecessary lines

* Added mixed content case

* Try to make test less flaky

* Make test not flaky

* Use attribute_test

* lint
--

wpt-commits: 029d8d6672ceaed11b0cfef841621ece520a78ec
wpt-pr: 30970
ericorth pushed a commit to ericorth/fetch that referenced this issue Feb 18, 2022
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 16, 2022
 - Some tests in wpt/preload use Resource Timing entries to make sure
   that no requests are made. We're changing that (Resource Timing
   entries should be created even when blocked by CSP - see
   whatwg/fetch#1215). Stop using
   Resource Timing entries and check that with server side scripts.
 - http/tests/preload/preload-csp.html is covered by some WPTs. Let's
   remove it.

Change-Id: I3c2cdfa2459d212657be7569c5290c48b39d6f05
Bug: 1275564
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 29, 2022
 - Some tests in wpt/preload use Resource Timing entries to make sure
   that no requests are made. We're changing that (Resource Timing
   entries should be created even when blocked by CSP - see
   whatwg/fetch#1215). Stop using
   Resource Timing entries and check that with server side scripts.
 - http/tests/preload/preload-csp.html is covered by some WPTs. Let's
   remove it.

Change-Id: I3c2cdfa2459d212657be7569c5290c48b39d6f05
Bug: 1275564
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 30, 2022
 - Some tests in wpt/preload use Resource Timing entries to make sure
   that no requests are made. We're changing that (Resource Timing
   entries should be created even when blocked by CSP - see
   whatwg/fetch#1215). Stop using
   Resource Timing entries and check that with server side scripts.
 - http/tests/preload/preload-csp.html is covered by some WPTs. Let's
   remove it.

Change-Id: I3c2cdfa2459d212657be7569c5290c48b39d6f05
Bug: 1275564
aarongable pushed a commit to chromium/chromium that referenced this issue Jun 30, 2022
 - Some tests in wpt/preload use Resource Timing entries to make sure
   that no requests are made. We're changing that (Resource Timing
   entries should be created even when blocked by CSP - see
   whatwg/fetch#1215). Stop using
   Resource Timing entries and check that with server side scripts.
 - http/tests/preload/preload-csp.html is covered by some WPTs. Let's
   remove it.

Change-Id: I3c2cdfa2459d212657be7569c5290c48b39d6f05
Bug: 1275564
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3708287
Reviewed-by: Yoav Weiss <[email protected]>
Commit-Queue: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1019490}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 30, 2022
 - Some tests in wpt/preload use Resource Timing entries to make sure
   that no requests are made. We're changing that (Resource Timing
   entries should be created even when blocked by CSP - see
   whatwg/fetch#1215). Stop using
   Resource Timing entries and check that with server side scripts.
 - http/tests/preload/preload-csp.html is covered by some WPTs. Let's
   remove it.

Change-Id: I3c2cdfa2459d212657be7569c5290c48b39d6f05
Bug: 1275564
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3708287
Reviewed-by: Yoav Weiss <[email protected]>
Commit-Queue: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1019490}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 30, 2022
 - Some tests in wpt/preload use Resource Timing entries to make sure
   that no requests are made. We're changing that (Resource Timing
   entries should be created even when blocked by CSP - see
   whatwg/fetch#1215). Stop using
   Resource Timing entries and check that with server side scripts.
 - http/tests/preload/preload-csp.html is covered by some WPTs. Let's
   remove it.

Change-Id: I3c2cdfa2459d212657be7569c5290c48b39d6f05
Bug: 1275564
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3708287
Reviewed-by: Yoav Weiss <[email protected]>
Commit-Queue: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1019490}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 5, 2022
Automatic update from web-platform-tests
Fix up wpt/preload

 - Some tests in wpt/preload use Resource Timing entries to make sure
   that no requests are made. We're changing that (Resource Timing
   entries should be created even when blocked by CSP - see
   whatwg/fetch#1215). Stop using
   Resource Timing entries and check that with server side scripts.
 - http/tests/preload/preload-csp.html is covered by some WPTs. Let's
   remove it.

Change-Id: I3c2cdfa2459d212657be7569c5290c48b39d6f05
Bug: 1275564
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3708287
Reviewed-by: Yoav Weiss <[email protected]>
Commit-Queue: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1019490}

--

wpt-commits: ba22f229dfafa51c637aa02957f8b9f330f1cfa3
wpt-pr: 34462
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jul 7, 2022
Automatic update from web-platform-tests
Fix up wpt/preload

 - Some tests in wpt/preload use Resource Timing entries to make sure
   that no requests are made. We're changing that (Resource Timing
   entries should be created even when blocked by CSP - see
   whatwg/fetch#1215). Stop using
   Resource Timing entries and check that with server side scripts.
 - http/tests/preload/preload-csp.html is covered by some WPTs. Let's
   remove it.

Change-Id: I3c2cdfa2459d212657be7569c5290c48b39d6f05
Bug: 1275564
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3708287
Reviewed-by: Yoav Weiss <[email protected]>
Commit-Queue: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1019490}

--

wpt-commits: ba22f229dfafa51c637aa02957f8b9f330f1cfa3
wpt-pr: 34462
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
 - Some tests in wpt/preload use Resource Timing entries to make sure
   that no requests are made. We're changing that (Resource Timing
   entries should be created even when blocked by CSP - see
   whatwg/fetch#1215). Stop using
   Resource Timing entries and check that with server side scripts.
 - http/tests/preload/preload-csp.html is covered by some WPTs. Let's
   remove it.

Change-Id: I3c2cdfa2459d212657be7569c5290c48b39d6f05
Bug: 1275564
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3708287
Reviewed-by: Yoav Weiss <[email protected]>
Commit-Queue: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1019490}
NOKEYCHECK=True
GitOrigin-RevId: aae9fb0d93e91afcc6ad38ea668bcc9f905f0fe6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: timing Issues and PR that touch on the infrastructure that is used by ResourceTiming, NavigationTiming, etc
Development

Successfully merging a pull request may close this issue.

6 participants