Can you elaborate on what do you mean by breaking the protocol? If a fetch request never requests its body, it the load never finishes. That's is how the Fetch API works. This is just like how, in the browser process, we cannot assume any URLRequest got to the end. Doing so would mean buffering unbounded memory somewhere (and reducing Chrome memory usage is a rather important goal).

In general, this is a fundamental property of networking APIs. If the consumer is slow, no layer of the pipeline fast-forwards to the end. Doing so would require that layer buffer memory unboundedly.

This does not mean something is wrong with how Fetch API works. It's just that we used to always have loadingFinished before and all the clients of public Remote Debugging Protocol are already relying on it.

We should figure out a way forward. One short-term fix could be disabling backpressure mechanism while instrumenting network. WDYT?

That would mean that the tab suddenly becomes much less performant under instrumentation. That seems extremely poor for debuggability.

I believe you already couldn't rely on loadingFinished showing up because no-store responses had backpressure on or something like that (ricea@ can correct me if I'm wrong). It was just that, due to interactions with the HTTP cache, Blink couldn't implement the right behavior for most resources.

What exactly breaks here? In what way are they relying on this? Presumably they already had to deal with the event coming in at an arbitrarily late time, since the network could just be slow. (And since the webpage isn't requesting the body, no one would notice.) Or maybe the network was so slow that the request never completed at all.

I should add: I can't speak for the Blink implementation, but in, for instance, //net, it is not possible to simple "disable" the backpressure mechanism. Backpressure isn't a mechanism or feature. It's an underlying assumption of well-designed networking pipelines.

Presumably it's less woven into Blink, since they had to account for this workaround for a while, but I would hope that, with the limitations lifted, they could eventually redesign their stack and reduce its overhead accordingly.

> Can you elaborate on what do you mean by breaking the protocol?

@davidben: sure. For the `fetch` requests, DevTools protocol used to emit the following sequence of events:

  Network.requestWillBeSent
  Network.responseReceived
  Network.dataReceived
  ...
  Network.dataReceived
  Network.loadingFinished

With the latest patch, the `Network.loadingFinished` is never fired. As a result, it's impossible to get response body using the DevTools Protocol: the `Network.getResponseBody()` method doesn't work properly until the `Network.loadingFinished` event is received.

@pfeldman suggested enhancing `Network.getResponseBody` to force content loading. @dgozman, wdyt?

What happens if the response body normally never even lives in memory at once? This is the entire point of streaming APIs like fetch(). If it's a large resource that the webpage can stream (consider server-sent events), it will just download data at the rate that you can process (or that the sender can send, whichever is slower), process it, and at no point do you keep more than a bounded slice of it in memory.

It sounds to me like breaking that API is necessary to reduce memory usage.

@davidben: I'm sorry I missed your comment #4. 

> What exactly breaks here?

The `Network.getResponseBody` breaks. This surfaced in one of the puppeteer tests, and also in certain devtools-protocol tests that I see in the crrev.com/530070.

> What happens if the response body normally never even lives in memory at once?

The `Network.getResponseBody` is not streaming, so it's clients already assume the memory consumption. Additionally, buffer size is configurable with `Network.enable` command: https://aslushnikov.github.io/vanilla-protocol-viewer/#Network.enable

The compromise solution suggested in Comment #6 should satisfy both parties.

crrev.com/530070 needed to be reverted anyway, because of the performance regression in  issue 804320 . However, I will be putting it back once we find a way to avoid the performance regression.

What @davidben said in #4 is correct. This was already the behaviour for requests with no-store set. It was a bug that fetch() was loading the body when the page hadn't requested it in other cases.

Is Network.loadingFinished supposed to indicate that the last bytes have been read from the network? In that case, it would incorrect to assume that the body is available in the render process at that point.

Any test which doesn't request the body but still assumes the body will be loaded anyway is incorrect. Please fix the tests.

I would prefer it if you did not change Blink behaviour when DevTools is open. This causes intense confusion among web developers. Then they file bugs and cause confusion among Chromium developers as well.

No matter how we resolve this issue, let's not hurry with relanding :)

I suspect this may break the following as well:
- IdlenessDetector, which listens to loading started and loading finished;
- Virtual time, which pauses while network request is in progress;
- something else?

Let's ensure we have a good understanding before proceeding.

FYI: the revert of crrev.com/530070 has landed in M65

https://chromium-review.googlesource.com/c/chromium/src/+/896744

#11: If these things don't work in the presence of backpressure, then they are already broken on any page that uses a video tag. There may be other parts of the web platform already using backpressure as well.

I've not come across back pressure before, would this affect calls to  ResourceFetcher::HandleLoaderFinish?  If so that would be a problem for headless.

Friendly ping to get an update on this issue as it is marked as stable blocker fro M65.

Thanks..!

M65 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge  into the release branch ASAP. Thank you.

It's already merged to M65.

RE #c16, #c11, and #c4

We did recently run into headless / virtual time issues with backpressure in the presence of Cache-Control: no store. 

This is causing issues. In particular twitch.tv currently freezes under headless with virtual time enabled:

  cr run headless_shell https://www.twitch.tv/spy_evan --screenshot --virtual-time-budget=5000

(I can attach a smaller self-contained testcase if needed)

I would like to fix the virtual-time aspect of this issue for headless, and think it's fairly important even if it's just the "cache-control: no-store" case. Is anyone looking at this? 

#18 It would probably be better to file a separate issue against virtual time.

#18: agreed with @ricea, please file a separate issue for Virtual time.

Given the offending patch was reverted, this should be closed now.

Is there a bug open for fixing DevTools' dependency on this? That the fetch() function does not actually stream when it's supposed to isn't great for platform predictability. It's also not good for overall memory usage, since the renderer will use memory that it might not have needed.