Attach timing info and URL to network errors, and report for fetch API

[noamr]

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 and this work.

Closes #1215

• [x ] At least two implementers are interested (and none opposed):

  • See discussion at Clarify which network errors create a resource timing entry #1215.

• Tests are written and can be reviewed and commented upon at:

  • Currently WIP, see Test that network errors create resource timing entries web-platform-tests/wpt#30968

• Implementation bugs are filed:

  • Chrome: …
  • Firefox: …
  • Safari: https://bugs.webkit.org/show_bug.cgi?id=193902

(See WHATWG Working Mode: Changes for more details.)

---

Preview | Diff

[annevk]

Generally this seems to be heading in the right direction. Thanks for tackling this!

I haven't checked in detail whether this overlooks any network error creation.

I think the creation algorithm needs some slight tweaks.

And I think we want to change things such that processResponseDone also runs in the event of a network error.

[annevk]

Looking at this again I think I missed something initially and this could be a lot simpler. All these network errors end up in "fetch finale" and it seems that at that point we could initialize them.

That leaves "finalize response". I suppose that for some network errors (in particular CORS) there could be a successful body stream so this would come later. For others that might not happen. Either way, we probably want to report them as soon as possible (i.e., as part of "fetch finale") to avoid leaking timing information. (Which also means "finalize response" (or its callers) has to filter out network errors or some such.)

Does that make sense or am I missing something here?

[noamr]

Looking at this again I think I missed something initially and this could be a lot simpler. All these network errors end up in "fetch finale" and it seems that at that point we could initialize them.

Yes, that would simplify things!

That leaves "finalize response". I suppose that for some network errors (in particular CORS) there could be a successful body stream so this would come later. For others that might not happen. Either way, we probably want to report them as soon as possible (i.e., as part of "fetch finale") to avoid leaking timing information. (Which also means "finalize response" (or its callers) has to filter out network errors or some such.)

Does that make sense or am I missing something here?

We're already reporting them in the fetch finale, by calling process response done which would schedule the caller to run some steps, one of which would be to report timing for the response with the correct initiatorType, and allowing the caller to decide the order of events / reporting.

[annevk]

As far as I can tell "fetch finale" does not invoke "process response done"; that's "finalize response".

[noamr]

As far as I can tell "fetch finale" does not invoke "process response done"; that's "finalize response".

It does now for the network error case (see new commit, a lot simpler indeed)

[annevk]

As I tried to point out, this doesn't prevent "finalize response" from being invoked and running "process response done" again. So you need to early exit for network errors there I think.

I also think we want to do something similar for "process response end-of-body". Where we check if the response's body is null, we should also check if it's a network error and do the same thing.

[noamr]

That leaves "finalize response". I suppose that for some network errors (in particular CORS) there could be a successful body stream so this would come later. For others that might not happen. Either way, we probably want to report them as soon as possible (i.e., as part of "fetch finale") to avoid leaking timing information. (Which also means "finalize response" (or its callers) has to filter out network errors or some such.)

Are you suggesting to omit the timing entry if the network error comes after the body stream has been opened?
I don't think it leaks any information. The fetch client knows when they get the error event, and the timing entry should correspond to that.

[noamr]

As I tried to point out, this doesn't prevent "finalize response" from being invoked and running "process response done" again. So you need to early exit for network errors there I think.

I've added a check for the done flag which should cover the case of double reporting.

[annevk]

The done flag check is nice!

I think there are still some problems:

• Finalize response is called too soon in fetch finale. The other callbacks need to happen first.
• (Repeating this one as I think you missed it.) I also think we want to do something similar for "process response end-of-body". Where we check if the response's body is null, we should also check if it's a network error and do the same thing.

[noamr]

• (Repeating this one as I think you missed it.) I also think we want to do something similar for "process response end-of-body". Where we check if the response's body is null, we should also check if it's a network error and do the same thing.

To understand this better, like if it's a network error that became a network after fetch finale?

[annevk]

@noamr maybe that one isn't needed. I was thinking that step 3 needed to start with "If response is a network error or response’s body is null" to ensure we don't end up reading a network error, but by definition a network error has no body so it should already do the right thing. In the case I was concerned with (CORS error) I forgot that we end up returning a new network error) and basically forget (and don't invoke callbacks) about the network response. If you could double check that I'd appreciate it though.

[noamr]

• Finalize response is called too soon in fetch finale. The other callbacks need to happen first.

It's there on purpose. I want to make sure processResposeDone is always called before the other ones for network errors, so that reporting etc. would happen before any onerror events.

[annevk]

We don't know that progressResponseDone is only used for reporting and callers might well rely on the order; it also seems wrongish for it to happen before the error event as that would be a new timing channel.

[noamr]

• Finalize response is called too soon in fetch finale. The other callbacks need to happen first.

It's there on purpose. I want to make sure processResposeDone is always called before the other ones for network errors, so that reporting etc. would happen before any onerror events.

Though I guess perhaps it would more sense to call done after, and to use the process response and process response end-of-body callbacks in the HTML spec callers.

[noamr]

We don't know that progressResponseDone is only used for reporting and callers might well rely on the order; it also seems wrongish for it to happen before the error event as that would be a new timing channel.

In any case, setting the URL list and timing info for the error response should be done before calling the other callbacks, in case they do the reporting, so I should split this condition into two.

[noamr]

@noamr maybe that one isn't needed. I was thinking that step 3 needed to start with "If response is a network error or response’s body is null" to ensure we don't end up reading a network error, but by definition a network error has no body so it should already do the right thing. In the case I was concerned with (CORS error) I forgot that we end up returning a new network error) and basically forget (and don't invoke callbacks) about the network response. If you could double check that I'd appreciate it though.

I double checked, a network error's body is always null, that's explicit in the definition of network error.

[noamr]

We don't know that progressResponseDone is only used for reporting and callers might well rely on the order; it also seems wrongish for it to happen before the error event as that would be a new timing channel.

In any case, setting the URL list and timing info for the error response should be done before calling the other callbacks, in case they do the reporting, so I should split this condition into two.

Fixed in new PR, @annevk

[annevk]

That's not what I meant. Basically when there's a request we get a response from the server. Then we start processing the response while also processing its body in parallel. Processing its body in parallel eventually calls "process response done" (through finalize response), but if processing the response turns into a network error that will also want to call "process response done" (through finalize response). The question is whether the latter always happens before the former.

And I don't think that's necessarily the case when integrity is involved as that also awaits the body.

Two questions then:

1. Is that the only problematic case?
2. How do we solve this? The fundamental problem here is the finalize response call from the place where we are receiving bytes from the network as we are not certain at that point whether that is the final response. Here is a rough idea:
   • Fetch finale adds a listener to response if the response is not a network error. (If it is a network error it can invoke finalize response itself.)
   • Where we are receiving bytes from the network we check if a listener was added and only then do we invoke it. (I suppose this could also be a flag of sorts.)
   • To avoid race conditions, where we are receiving bytes from the network also sets the done flag.
   • Fetch finale also checks the done flag and if the done flag is already set it doesn't add a listener and invokes finalize response directly. It also sets the done flag for network errors.
   • Finalize response no longer deals in the done flag.

I'd love review from @yutakahirano and maybe @ricea on this, as well as you @noamr and @yoavweiss.

[noamr]

That's not what I meant. Basically when there's a request we get a response from the server. Then we start processing the response while also processing its body in parallel. Processing its body in parallel eventually calls "process response done" (through finalize response), but if processing the response turns into a network error that will also want to call "process response done" (through finalize response). The question is whether the latter always happens before the former.

And I don't think that's necessarily the case when integrity is involved as that also awaits the body.

Two questions then:

1. Is that the only problematic case?

2. How do we solve this? The fundamental problem here is the finalize response call from the place where we are receiving bytes from the network as we are not certain at that point whether that is the final response. Here is a rough idea:

   • Fetch finale adds a listener to response if the response is not a network error. (If it is a network error it can invoke finalize response itself.)
   • Where we are receiving bytes from the network we check if a listener was added and only then do we invoke it. (I suppose this could also be a flag of sorts.)
   • To avoid race conditions, where we are receiving bytes from the network also sets the done flag.
   • Fetch finale also checks the done flag and if the done flag is already set it doesn't add a listener and invokes finalize response directly. It also sets the done flag for network errors.
   • Finalize response no longer deals in the done flag.

I'd love review from @yutakahirano and maybe @ricea on this, as well as you @noamr and @yoavweiss.

OK now I understand what you meant.

I tried something similar in the new PR amendment.
finalize becomes a callback associated with response, that is only set during finale for the non network-error case. This should cover the integrity case, and any potential future case where the body is read before the finale.

[annevk]

I like the idea of using a task to synchronize. Nice. Couple of remaining nits and it would be really great if we could secure at least one other reviewer.

[noamr]

I like the idea of using a task to synchronize. Nice. Couple of remaining nits and it would be really great if we could secure at least one other reviewer.

Fixed nits. Who should we ask to review this?

[annevk]

As for who else to review: ideally the folks I mentioned earlier.

[noamr]

After making an exception for aborts and non-HTTP(s) URLs, this now matches the Chrome/Firefox behavior + reported bugs like this one. Corresponding WPT PR is also ready to be merged.

[yutakahirano]

Chrome is going to implement this once this and tests are ready.

[annevk]

Using streams in this way seems really nice and reduces the overall complexity a lot. Great work! I do think there are some logic errors here and there, but they seem fixable.

[noamr]

Revised in the spirit of the comments.