Removing error page commits within the renderer

[Rakina Zata Amni]

Hi navigation-dev,

We're currently looking into removing error page commits within the renderer (the ones that start and immediately commit within the renderer, only notifying browser through DidCommitProvisionalLoad), as we found that it's blocking our work to remove fields from DidCommitProvisionalLoad_Params.

It seems like there's only one case left for this (CMIIW), and we have two potential solutions here: send the commit to the browser, or detect the case in the browser. Currently I'm leaning towards the first solution because the second solution might result in potential delays to CommitNavigation.

I made a doc that explains more, PTAL :)

[Daniel Cheng]

Would it make sense to just suppress the DidCommit to the browser? Or is this additional call significant for updating some state on the browser side?

Daniel

--
You received this message because you are subscribed to the Google Groups "navigation-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to navigation-de...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CACPC1r7z0ExsVT8boSxZ083HmeGdRHbp2GyWCh7VV2ea%2BD%2BzZA%40mail.gmail.com.

[Arthur Sonzogni]

Thanks Rakina!

From the browser process point of view, when receiving a blank 404 page. It sends CommitNavigation and receives DidCommitNavigation as usual. Later blink replaces the blank document by a sort of error page and sends a second DidCommitNavigation. 

The browser process usually kills the renderer when receiving this unexpected DidCommitNavigation. However we allow this edge case for now in DidCommitNavigationInternal.

```

// 3) Error pages implementations in Chrome can commit twice.

// TODO(clamy): Fix this.

is_commit_allowed_to_proceed |= params->url_is_unreachable;

``` This would be a huge win removing it!

 

Would it make sense to just suppress the DidCommit to the browser? Or is this additional call significant for updating some state on the browser side?

~Daniel

This looks appealing. However, the DidCommit tells the browser the current document in the renderer has changed its origin.
It also has the side effect of changing the address-bar indicator. See:

• Custom 404 error page
• Chrome error page

Ideally, in a not so far future, we would compute the origin of the document to commit, before sending CommitNavigation. We can then better decide what kind of URLLoaderFactories / capabilities are given to the document.

---

I think I am leaning toward either:
1) On 4XX status code, the browser starts reading the response_body and decides depending on its size if it should commit an error page or continue with the normal page sent by the server.
2) Blink commits the normal page (as it does today), but instead of sending a second DidCommit, it requests the browser process to navigate to a "real" error page.

(2) is probably way easier than (1).

[Rakina Zata Amni]

On Fri, Sep 25, 2020 at 5:37 PM Arthur Sonzogni <arthurs...@google.com> wrote:

Thanks Rakina!

From the browser process point of view, when receiving a blank 404 page. It sends CommitNavigation and receives DidCommitNavigation as usual. Later blink replaces the blank document by a sort of error page and sends a second DidCommitNavigation. 

The browser process usually kills the renderer when receiving this unexpected DidCommitNavigation. However we allow this edge case for now in DidCommitNavigationInternal.

```

// 3) Error pages implementations in Chrome can commit twice.

// TODO(clamy): Fix this.

is_commit_allowed_to_proceed |= params->url_is_unreachable;

``` This would be a huge win removing it!

 

Would it make sense to just suppress the DidCommit to the browser? Or is this additional call significant for updating some state on the browser side?

~Daniel

This looks appealing. However, the DidCommit tells the browser the current document in the renderer has changed its origin.
It also has the side effect of changing the address-bar indicator. See:

• Custom 404 error page
• Chrome error page

Ideally, in a not so far future, we would compute the origin of the document to commit, before sending CommitNavigation. We can then better decide what kind of URLLoaderFactories / capabilities are given to the document.

To add to this, I think not letting the browser know if it committed a new error page will not work with RenderDocument. 

---

I think I am leaning toward either:
1) On 4XX status code, the browser starts reading the response_body and decides depending on its size if it should commit an error page or continue with the normal page sent by the server.
2) Blink commits the normal page (as it does today), but instead of sending a second DidCommit, it requests the browser process to navigate to a "real" error page.

(2) is probably way easier than (1).

Is my understanding correct that (2) corresponds to proposal #1 in the doc, "Send the commit to the browser", where we will keep the empty page, and then ask the browser to commit a chrome error page?

Or do you mean for (2) it should "commit" the chrome error page, but should not send a DidCommit call to the browser and instead it will create a new navigation request for the chrome error page? This would not have the additional round trip delay of showing the final error page like the proposal in the doc, so it does sound interesting. However, doesn't this pose the same problems as not sending DidCommit at all (although just for a few moments before the browser gets updated I guess), and maybe additional inconsistencies between browser and renderer for that brief period?

[Arthur Sonzogni]

I think I am leaning toward either:

1) On 4XX status code, the browser starts reading the response_body and decides depending on its size if it should commit an error page or continue with the normal page sent by the server.
2) Blink commits the normal page (as it does today), but instead of sending a second DidCommit, it requests the browser process to navigate to a "real" error page.

(2) is probably way easier than (1).

Is my understanding correct that (2) corresponds to proposal #1 in the doc, "Send the commit to the browser", where we will keep the empty page, and then ask the browser to commit a chrome error page?

Or do you mean for (2) it should "commit" the chrome error page, but should not send a DidCommit call to the browser and instead it will create a new navigation request for the chrome error page?

Yes, sorry, I reversed the order. 

 

This would not have the additional round trip delay of showing the final error page like the proposal in the doc, so it does sound interesting.

However, doesn't this pose the same problems as not sending DidCommit at all (although just for a few moments before the browser gets updated I guess), and maybe additional inconsistencies between browser and renderer for that brief period?

Yes they both have their pro/cons. I am not quite sure which one is the best. I was a bit reluctant to implement something new in the NavigationRequest (reading the response_body on 4XX) and was thinking it might be easier to rely on what we already have (submitting new navigations).
Maybe reading the response_body on 4XX isn't that hard. This avoids committing the empty 404 document before the chrome 404 document.

[Nasko Oskov]

Does it actually require reading the response body? Can't we rely on the Content-Length: header, which should be available? This should allow us to know that we need to commit a Chrome error page instead of what the server sent at ReadyToCommit time and adjust accordingly.

[Rakina Zata Amni]

Yeah we were wondering about that, but weren't sure if Content-Length is always present and reliable for this use case or not. +Yutaka Hirano mentioned that it might not be trustworthy when chunked transfer encoding is used, and service workers are involved. Also from this RFC it seems like Content-Length might not always be there (it's only required for requests). Do you have other info on this?

[Nasko Oskov]

On Fri, Sep 25, 2020 at 10:50 AM Rakina Zata Amni <rak...@chromium.org> wrote:

Yeah we were wondering about that, but weren't sure if Content-Length is always present and reliable for this use case or not. +Yutaka Hirano mentioned that it might not be trustworthy when chunked transfer encoding is used, and service workers are involved.

Would we expect to get 404 from a ServiceWorker? I don't have much experience in the area, so happy to learn more!

In general, I would be very surprised if we see in practice a 404 error page use chunked encoding. We can put some UMA stats to try and detect how often this happens in practice and use it as a guide.

 

Also from this RFC it seems like Content-Length might not always be there (it's only required for requests). Do you have other info on this?

Yes, it is not a required field. Maybe we should see if we have any metrics on its usage in practice? I would suspect someone on Chrome has experience in this area, but I'm not sure who that would be.

[Matt Falkenhagen]

Thanks, I placed some comments in the doc.

2020年9月26日(土) 5:48 Nasko Oskov <na...@chromium.org>:

On Fri, Sep 25, 2020 at 10:50 AM Rakina Zata Amni <rak...@chromium.org> wrote:

Yeah we were wondering about that, but weren't sure if Content-Length is always present and reliable for this use case or not. +Yutaka Hirano mentioned that it might not be trustworthy when chunked transfer encoding is used, and service workers are involved.

Would we expect to get 404 from a ServiceWorker? I don't have much experience in the area, so happy to learn more!

In general, I would be very surprised if we see in practice a 404 error page use chunked encoding. We can put some UMA stats to try and detect how often this happens in practice and use it as a guide.

I'm not sure service workers change the equation much here. From the Chrome implementation side, we forward all the headers they provide to the client, as if they came from the server.

It's possible developers are writing service workers that are more badly behaved than servers with respect to providing the Content-Length header, but I don't know that it makes a big difference.

I agree that 404 pages and incorrect Content-Length header may be a niche case. From my understanding, this whole problem space is about how to know whether to show the embedder-provided error page (ContentRendererClient::HasErrorPage()). If we don't show the embedded-provided error page, what happens... does it show a blank page or some default one provided by //content?

 

 

Also from this RFC it seems like Content-Length might not always be there (it's only required for requests). Do you have other info on this?

Yes, it is not a required field. Maybe we should see if we have any metrics on its usage in practice? I would suspect someone on Chrome has experience in this area, but I'm not sure who that would be.

 

On Sat, Sep 26, 2020 at 2:17 AM Nasko Oskov <na...@chromium.org> wrote:

On Fri, Sep 25, 2020 at 3:11 AM Arthur Sonzogni <arthurs...@google.com> wrote:

I think I am leaning toward either:

1) On 4XX status code, the browser starts reading the response_body and decides depending on its size if it should commit an error page or continue with the normal page sent by the server.
2) Blink commits the normal page (as it does today), but instead of sending a second DidCommit, it requests the browser process to navigate to a "real" error page.

(2) is probably way easier than (1).

Is my understanding correct that (2) corresponds to proposal #1 in the doc, "Send the commit to the browser", where we will keep the empty page, and then ask the browser to commit a chrome error page?

Or do you mean for (2) it should "commit" the chrome error page, but should not send a DidCommit call to the browser and instead it will create a new navigation request for the chrome error page?

Yes, sorry, I reversed the order. 

 

This would not have the additional round trip delay of showing the final error page like the proposal in the doc, so it does sound interesting.

However, doesn't this pose the same problems as not sending DidCommit at all (although just for a few moments before the browser gets updated I guess), and maybe additional inconsistencies between browser and renderer for that brief period?

Yes they both have their pro/cons. I am not quite sure which one is the best. I was a bit reluctant to implement something new in the NavigationRequest (reading the response_body on 4XX) and was thinking it might be easier to rely on what we already have (submitting new navigations).
Maybe reading the response_body on 4XX isn't that hard. This avoids committing the empty 404 document before the chrome 404 document.

Does it actually require reading the response body? Can't we rely on the Content-Length: header, which should be available? This should allow us to know that we need to commit a Chrome error page instead of what the server sent at ReadyToCommit time and adjust accordingly.

--

You received this message because you are subscribed to the Google Groups "navigation-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to navigation-de...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CAA%3DmyAvcg7W2UOkkzDXK-58AXk1wsK3u717ZzobcYwe%3D-2mRNg%40mail.gmail.com.

[Rakina Zata Amni]

On Mon, Sep 28, 2020 at 11:32 AM Matt Falkenhagen <fal...@chromium.org> wrote:

Thanks, I placed some comments in the doc.

2020年9月26日(土) 5:48 Nasko Oskov <na...@chromium.org>:

On Fri, Sep 25, 2020 at 10:50 AM Rakina Zata Amni <rak...@chromium.org> wrote:

Yeah we were wondering about that, but weren't sure if Content-Length is always present and reliable for this use case or not. +Yutaka Hirano mentioned that it might not be trustworthy when chunked transfer encoding is used, and service workers are involved.

Would we expect to get 404 from a ServiceWorker? I don't have much experience in the area, so happy to learn more!

In general, I would be very surprised if we see in practice a 404 error page use chunked encoding. We can put some UMA stats to try and detect how often this happens in practice and use it as a guide.

I'm not sure service workers change the equation much here. From the Chrome implementation side, we forward all the headers they provide to the client, as if they came from the server.

It's possible developers are writing service workers that are more badly behaved than servers with respect to providing the Content-Length header, but I don't know that it makes a big difference.

I agree that 404 pages and incorrect Content-Length header may be a niche case. From my understanding, this whole problem space is about how to know whether to show the embedder-provided error page (ContentRendererClient::HasErrorPage()). If we don't show the embedded-provided error page, what happens... does it show a blank page or some default one provided by //content?

 

If we don't show the error page, we will show a blank page, which is probably bad UX (see original bug). We can definitely try to measure how often error pages with incorrect/non-existent content-length show up, but I'm wondering how comfortable are we with missing these cases?

Also, how long should we gather the data for (weeks? months?). If we need a long time to be confident we've captured enough data, I'm wondering if it's better to do this in parallel with implementing the simpler proposal (to unblock the current work) and change it later if the data shows we can rely on Content-Length most of the time.

[Matt Falkenhagen]

2020年9月28日(月) 22:28 Rakina Zata Amni <rak...@chromium.org>:

On Mon, Sep 28, 2020 at 11:32 AM Matt Falkenhagen <fal...@chromium.org> wrote:

Thanks, I placed some comments in the doc.

2020年9月26日(土) 5:48 Nasko Oskov <na...@chromium.org>:

On Fri, Sep 25, 2020 at 10:50 AM Rakina Zata Amni <rak...@chromium.org> wrote:

Yeah we were wondering about that, but weren't sure if Content-Length is always present and reliable for this use case or not. +Yutaka Hirano mentioned that it might not be trustworthy when chunked transfer encoding is used, and service workers are involved.

Would we expect to get 404 from a ServiceWorker? I don't have much experience in the area, so happy to learn more!

In general, I would be very surprised if we see in practice a 404 error page use chunked encoding. We can put some UMA stats to try and detect how often this happens in practice and use it as a guide.

I'm not sure service workers change the equation much here. From the Chrome implementation side, we forward all the headers they provide to the client, as if they came from the server.

It's possible developers are writing service workers that are more badly behaved than servers with respect to providing the Content-Length header, but I don't know that it makes a big difference.

I agree that 404 pages and incorrect Content-Length header may be a niche case. From my understanding, this whole problem space is about how to know whether to show the embedder-provided error page (ContentRendererClient::HasErrorPage()). If we don't show the embedded-provided error page, what happens... does it show a blank page or some default one provided by //content?

 

If we don't show the error page, we will show a blank page, which is probably bad UX (see original bug). We can definitely try to measure how often error pages with incorrect/non-existent content-length show up, but I'm wondering how comfortable are we with missing these cases?

Also, how long should we gather the data for (weeks? months?). If we need a long time to be confident we've captured enough data, I'm wondering if it's better to do this in parallel with implementing the simpler proposal (to unblock the current work) and change it later if the data shows we can rely on Content-Length most of the time.

Thanks I see. Agree we don't want to show the blank page. I also agree with doing the simpler proposal and not over-engineering this though it's not really clear which one is simpler, is that Proposal #1 in your doc?

I think I had an idea similar to dcheng's at the top of this thread. Would it be possible for the renderer to just not send the initial DidCommit ACK until it knows whether it wants to commit the error page or not? And if it turns out it wants the browser to navigate to another error page, it tells the browser that as part of the NavigationCommit callback / in place of DidCommit?

[Nasko Oskov]

On Mon, Sep 28, 2020 at 6:59 AM Matt Falkenhagen <fal...@chromium.org> wrote:

2020年9月28日(月) 22:28 Rakina Zata Amni <rak...@chromium.org>:

On Mon, Sep 28, 2020 at 11:32 AM Matt Falkenhagen <fal...@chromium.org> wrote:

Thanks, I placed some comments in the doc.

2020年9月26日(土) 5:48 Nasko Oskov <na...@chromium.org>:

On Fri, Sep 25, 2020 at 10:50 AM Rakina Zata Amni <rak...@chromium.org> wrote:

Yeah we were wondering about that, but weren't sure if Content-Length is always present and reliable for this use case or not. +Yutaka Hirano mentioned that it might not be trustworthy when chunked transfer encoding is used, and service workers are involved.

Would we expect to get 404 from a ServiceWorker? I don't have much experience in the area, so happy to learn more!

In general, I would be very surprised if we see in practice a 404 error page use chunked encoding. We can put some UMA stats to try and detect how often this happens in practice and use it as a guide.

I'm not sure service workers change the equation much here. From the Chrome implementation side, we forward all the headers they provide to the client, as if they came from the server.

It's possible developers are writing service workers that are more badly behaved than servers with respect to providing the Content-Length header, but I don't know that it makes a big difference.

I agree that 404 pages and incorrect Content-Length header may be a niche case. From my understanding, this whole problem space is about how to know whether to show the embedder-provided error page (ContentRendererClient::HasErrorPage()). If we don't show the embedded-provided error page, what happens... does it show a blank page or some default one provided by //content?

 

If we don't show the error page, we will show a blank page, which is probably bad UX (see original bug). We can definitely try to measure how often error pages with incorrect/non-existent content-length show up, but I'm wondering how comfortable are we with missing these cases?

Also, how long should we gather the data for (weeks? months?). If we need a long time to be confident we've captured enough data, I'm wondering if it's better to do this in parallel with implementing the simpler proposal (to unblock the current work) and change it later if the data shows we can rely on Content-Length most of the time.

Doing data gathering in parallel is ideal in my view. I don't know how long we need to collect data to know, but a week on Canary channel can potentially give us an easy go/no-go. Let's say 20% of 404 responses contain on Content-Length but have a body with non-zero length, then we can't really take that approach.

We should also evaluate whether we are ok with some breaking behavior. Let's say we decide to go with a Content-Length approach. If there is no header, we render Chrome error page, if there is, we render the provided response body. The implication of doing this is that there will be some 404 pages that supply the body but no header which we will not render. Would this be something we are ok with?

 

Thanks I see. Agree we don't want to show the blank page. I also agree with doing the simpler proposal and not over-engineering this though it's not really clear which one is simpler, is that Proposal #1 in your doc?

I think I had an idea similar to dcheng's at the top of this thread. Would it be possible for the renderer to just not send the initial DidCommit ACK until it knows whether it wants to commit the error page or not? And if it turns out it wants the browser to navigate to another error page, it tells the browser that as part of the NavigationCommit callback / in place of DidCommit?

DidCommit these days is a Mojo callback and is 1:1 with each navigation, so I'm slightly confused whether "in place of DidCommit" can actually happen. Can we alter the response parameters for DidCommit ... I think so, but the whole goal is to not rely in the long run on any of the DidCommit parameters. 

[Fergal Daly]

On Mon, 28 Sep 2020 at 22:59, Matt Falkenhagen <fal...@chromium.org> wrote:

2020年9月28日(月) 22:28 Rakina Zata Amni <rak...@chromium.org>:

On Mon, Sep 28, 2020 at 11:32 AM Matt Falkenhagen <fal...@chromium.org> wrote:

Thanks, I placed some comments in the doc.

2020年9月26日(土) 5:48 Nasko Oskov <na...@chromium.org>:

On Fri, Sep 25, 2020 at 10:50 AM Rakina Zata Amni <rak...@chromium.org> wrote:

Yeah we were wondering about that, but weren't sure if Content-Length is always present and reliable for this use case or not. +Yutaka Hirano mentioned that it might not be trustworthy when chunked transfer encoding is used, and service workers are involved.

Would we expect to get 404 from a ServiceWorker? I don't have much experience in the area, so happy to learn more!

In general, I would be very surprised if we see in practice a 404 error page use chunked encoding. We can put some UMA stats to try and detect how often this happens in practice and use it as a guide.

I'm not sure service workers change the equation much here. From the Chrome implementation side, we forward all the headers they provide to the client, as if they came from the server.

It's possible developers are writing service workers that are more badly behaved than servers with respect to providing the Content-Length header, but I don't know that it makes a big difference.

I agree that 404 pages and incorrect Content-Length header may be a niche case. From my understanding, this whole problem space is about how to know whether to show the embedder-provided error page (ContentRendererClient::HasErrorPage()). If we don't show the embedded-provided error page, what happens... does it show a blank page or some default one provided by //content?

 

If we don't show the error page, we will show a blank page, which is probably bad UX (see original bug). We can definitely try to measure how often error pages with incorrect/non-existent content-length show up, but I'm wondering how comfortable are we with missing these cases?

Also, how long should we gather the data for (weeks? months?). If we need a long time to be confident we've captured enough data, I'm wondering if it's better to do this in parallel with implementing the simpler proposal (to unblock the current work) and change it later if the data shows we can rely on Content-Length most of the time.

Thanks I see. Agree we don't want to show the blank page. I also agree with doing the simpler proposal and not over-engineering this though it's not really clear which one is simpler, is that Proposal #1 in your doc?

I think I had an idea similar to dcheng's at the top of this thread. Would it be possible for the renderer to just not send the initial DidCommit ACK until it knows whether it wants to commit the error page or not? And if it turns out it wants the browser to navigate to another error page, it tells the browser that as part of the NavigationCommit callback / in place of DidCommit?

We are trying to get rid of the browser's dependence on the callback and make it so that the browser can swap render frames and finish committing as soon as it sends the commit. So having it just do a renderer-initiated navigation would be compatible with that but I worry that it would be more complicated to commit a blank page and then navigate again to a error page,

F

[Matt Falkenhagen]

2020年9月29日(火) 13:53 'Fergal Daly' via navigation-dev <navigat...@chromium.org>:

On Mon, 28 Sep 2020 at 22:59, Matt Falkenhagen <fal...@chromium.org> wrote:

2020年9月28日(月) 22:28 Rakina Zata Amni <rak...@chromium.org>:

On Mon, Sep 28, 2020 at 11:32 AM Matt Falkenhagen <fal...@chromium.org> wrote:

Thanks, I placed some comments in the doc.

2020年9月26日(土) 5:48 Nasko Oskov <na...@chromium.org>:

On Fri, Sep 25, 2020 at 10:50 AM Rakina Zata Amni <rak...@chromium.org> wrote:

Yeah we were wondering about that, but weren't sure if Content-Length is always present and reliable for this use case or not. +Yutaka Hirano mentioned that it might not be trustworthy when chunked transfer encoding is used, and service workers are involved.

Would we expect to get 404 from a ServiceWorker? I don't have much experience in the area, so happy to learn more!

In general, I would be very surprised if we see in practice a 404 error page use chunked encoding. We can put some UMA stats to try and detect how often this happens in practice and use it as a guide.

I'm not sure service workers change the equation much here. From the Chrome implementation side, we forward all the headers they provide to the client, as if they came from the server.

It's possible developers are writing service workers that are more badly behaved than servers with respect to providing the Content-Length header, but I don't know that it makes a big difference.

I agree that 404 pages and incorrect Content-Length header may be a niche case. From my understanding, this whole problem space is about how to know whether to show the embedder-provided error page (ContentRendererClient::HasErrorPage()). If we don't show the embedded-provided error page, what happens... does it show a blank page or some default one provided by //content?

 

If we don't show the error page, we will show a blank page, which is probably bad UX (see original bug). We can definitely try to measure how often error pages with incorrect/non-existent content-length show up, but I'm wondering how comfortable are we with missing these cases?

Also, how long should we gather the data for (weeks? months?). If we need a long time to be confident we've captured enough data, I'm wondering if it's better to do this in parallel with implementing the simpler proposal (to unblock the current work) and change it later if the data shows we can rely on Content-Length most of the time.

Thanks I see. Agree we don't want to show the blank page. I also agree with doing the simpler proposal and not over-engineering this though it's not really clear which one is simpler, is that Proposal #1 in your doc?

I think I had an idea similar to dcheng's at the top of this thread. Would it be possible for the renderer to just not send the initial DidCommit ACK until it knows whether it wants to commit the error page or not? And if it turns out it wants the browser to navigate to another error page, it tells the browser that as part of the NavigationCommit callback / in place of DidCommit?

We are trying to get rid of the browser's dependence on the callback and make it so that the browser can swap render frames and finish committing as soon as it sends the commit. So having it just do a renderer-initiated navigation would be compatible with that but I worry that it would be more complicated to commit a blank page and then navigate again to a error page,

I see. I thought if we are going with Proposal #1 in Rakina's doc then suppressing the initial DidCommit could simplify things, but maybe not.

For Proposal #2, I suppose using Content-Length or, if we do need to read the response body in the browser, something like MimeSniffingThrottle except a throttle that just checks if the response body is empty, could work.

 

F

 

 

 

 

Also from this RFC it seems like Content-Length might not always be there (it's only required for requests). Do you have other info on this?

Yes, it is not a required field. Maybe we should see if we have any metrics on its usage in practice? I would suspect someone on Chrome has experience in this area, but I'm not sure who that would be.

 

On Sat, Sep 26, 2020 at 2:17 AM Nasko Oskov <na...@chromium.org> wrote:

On Fri, Sep 25, 2020 at 3:11 AM Arthur Sonzogni <arthurs...@google.com> wrote:

I think I am leaning toward either:

1) On 4XX status code, the browser starts reading the response_body and decides depending on its size if it should commit an error page or continue with the normal page sent by the server.
2) Blink commits the normal page (as it does today), but instead of sending a second DidCommit, it requests the browser process to navigate to a "real" error page.

(2) is probably way easier than (1).

Is my understanding correct that (2) corresponds to proposal #1 in the doc, "Send the commit to the browser", where we will keep the empty page, and then ask the browser to commit a chrome error page?

Or do you mean for (2) it should "commit" the chrome error page, but should not send a DidCommit call to the browser and instead it will create a new navigation request for the chrome error page?

Yes, sorry, I reversed the order. 

 

This would not have the additional round trip delay of showing the final error page like the proposal in the doc, so it does sound interesting.

However, doesn't this pose the same problems as not sending DidCommit at all (although just for a few moments before the browser gets updated I guess), and maybe additional inconsistencies between browser and renderer for that brief period?

Yes they both have their pro/cons. I am not quite sure which one is the best. I was a bit reluctant to implement something new in the NavigationRequest (reading the response_body on 4XX) and was thinking it might be easier to rely on what we already have (submitting new navigations).
Maybe reading the response_body on 4XX isn't that hard. This avoids committing the empty 404 document before the chrome 404 document.

Does it actually require reading the response body? Can't we rely on the Content-Length: header, which should be available? This should allow us to know that we need to commit a Chrome error page instead of what the server sent at ReadyToCommit time and adjust accordingly.

--
You received this message because you are subscribed to the Google Groups "navigation-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to navigation-de...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CAA%3DmyAvcg7W2UOkkzDXK-58AXk1wsK3u717ZzobcYwe%3D-2mRNg%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "navigation-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to navigation-de...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CAAozHLnp9DG294JY2F%3D9sXs4HwJDfgzMDxXW7SOphkWLY9Qv1w%40mail.gmail.com.

[Fergal Daly]

On Tue, 29 Sep 2020 at 16:20, Matt Falkenhagen <fal...@chromium.org> wrote:

2020年9月29日(火) 13:53 'Fergal Daly' via navigation-dev <navigat...@chromium.org>:

On Mon, 28 Sep 2020 at 22:59, Matt Falkenhagen <fal...@chromium.org> wrote:

2020年9月28日(月) 22:28 Rakina Zata Amni <rak...@chromium.org>:

On Mon, Sep 28, 2020 at 11:32 AM Matt Falkenhagen <fal...@chromium.org> wrote:

Thanks, I placed some comments in the doc.

2020年9月26日(土) 5:48 Nasko Oskov <na...@chromium.org>:

On Fri, Sep 25, 2020 at 10:50 AM Rakina Zata Amni <rak...@chromium.org> wrote:

Yeah we were wondering about that, but weren't sure if Content-Length is always present and reliable for this use case or not. +Yutaka Hirano mentioned that it might not be trustworthy when chunked transfer encoding is used, and service workers are involved.

Would we expect to get 404 from a ServiceWorker? I don't have much experience in the area, so happy to learn more!

In general, I would be very surprised if we see in practice a 404 error page use chunked encoding. We can put some UMA stats to try and detect how often this happens in practice and use it as a guide.

I'm not sure service workers change the equation much here. From the Chrome implementation side, we forward all the headers they provide to the client, as if they came from the server.

It's possible developers are writing service workers that are more badly behaved than servers with respect to providing the Content-Length header, but I don't know that it makes a big difference.

I agree that 404 pages and incorrect Content-Length header may be a niche case. From my understanding, this whole problem space is about how to know whether to show the embedder-provided error page (ContentRendererClient::HasErrorPage()). If we don't show the embedded-provided error page, what happens... does it show a blank page or some default one provided by //content?

 

If we don't show the error page, we will show a blank page, which is probably bad UX (see original bug). We can definitely try to measure how often error pages with incorrect/non-existent content-length show up, but I'm wondering how comfortable are we with missing these cases?

Also, how long should we gather the data for (weeks? months?). If we need a long time to be confident we've captured enough data, I'm wondering if it's better to do this in parallel with implementing the simpler proposal (to unblock the current work) and change it later if the data shows we can rely on Content-Length most of the time.

Thanks I see. Agree we don't want to show the blank page. I also agree with doing the simpler proposal and not over-engineering this though it's not really clear which one is simpler, is that Proposal #1 in your doc?

I think I had an idea similar to dcheng's at the top of this thread. Would it be possible for the renderer to just not send the initial DidCommit ACK until it knows whether it wants to commit the error page or not? And if it turns out it wants the browser to navigate to another error page, it tells the browser that as part of the NavigationCommit callback / in place of DidCommit?

We are trying to get rid of the browser's dependence on the callback and make it so that the browser can swap render frames and finish committing as soon as it sends the commit. So having it just do a renderer-initiated navigation would be compatible with that but I worry that it would be more complicated to commit a blank page and then navigate again to a error page,

I see. I thought if we are going with Proposal #1 in Rakina's doc then suppressing the initial DidCommit could simplify things, but maybe not.

For Proposal #2, I suppose using Content-Length or, if we do need to read the response body in the browser, something like MimeSniffingThrottle except a throttle that just checks if the response body is empty, could work.

I was initially in favour of the sniffing version but I don't like it's failure mode. For servers that send no body and no content-length with a 404, we cannot commit the navigation until we receive the FIN packet because without content-length that's the only way we know there is no body. So even though we know it's a 404 we're stuck on the old page until that packet arrives.

So how about:

- if we have content-length: 0 or some other definite evidence that we have no body, we commit an error page

- otherwise commit and then if the body ends up empty, do a renderer-initiated navigation to an error page

?

For all modern correctly behaving servers we get 1 commit only.

For legacy stuff, nothing breaks although we might show an empty page before the built-in error.

We eliminate the renderer-initiated commit,

F

[Yutaka Hirano]

I'm not following this - it is legitimate for a server to not attach a content-length header.

[Rakina Zata Amni]

Thanks all for the responses!

Trying to summarize the current discussions for proposal #2:

• Content-Length is already not guaranteed by the spec, which includes cases where Transfer-Encoding is defined, and in HTTP/2. However, we suspect that this might be rare for errors.
• We're unsure how comfortable we are with false positives (showing chrome error page but the site actually was sending its own non-empty page), but we want to avoid them as much as possible (or even always, in which case we need to wait for the response body).
• Additionally, even if we don't rely on Content-Length and wait to read the response body, there's some worry that waiting for the first byte or the connection closing will result in slower navigations.

Also, I spent some time today looking into how we can implement proposal #1 and ended up with a working implementation for it (CL). This approach turns out to be very simple because we can mostly just reuse the path for LoadPostCommitErrorPage.

Seeing that the implementation for proposal #1 is very simple and the downside is just the additional IPC roundtrip, I'm definitely leaning towards proposal #1 now :) We can still start gathering data for proposal #2 if there's still strong interest though (the CL for that is actually also ready).

[Arthur Sonzogni]

> . This approach turns out to be very simple because we can mostly just reuse the path for LoadPostCommitErrorPage.

Reissuing a navigation was my initial preference for simplicity. I assumed we would use the existing navigation IPC or build a specialized one. I didn't know LoadPostCommitErrorPage() already existed. Even better!

[Charlie Reis]

(Just reading this now.)  I mentioned this to Nasko, but we should not use LoadPostCommitErrorPage for this.  The "post-commit" part is explicitly for a different case, where we show an interstitial-like error after a normal page has committed.  That triggers a different flow in NavigationController which preserves the old committed NavigationEntry to the side and puts it back when you navigate elsewhere, forgetting the interstitial page's entry.  This wouldn't be compatible with normal 404-like error pages.

In general, it seems like it would be nicer to move from these error page navigations looking like 1.5 navigations to looking like 1 navigation (where we only send a single DidCommit), rather than looking like 2 full navigations (where we finish the first navigation and then kick off an entirely separate navigation).  I suppose it's hard to know before CommitNavigation whether we need to show the error content, at least without delaying things?  (Would it still be a delay if you factor in the lack of a second commit?)  Anyway, I don't feel too strongly here, so feel free to pick what works best.

Charlie

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CAAzos5GeKNghANaVzs5emoYTx_Wx%2Bn08Fy4-Nor%2Befn5DFOXQA%40mail.gmail.com.

[Nasko Oskov]

I have one more question here, since I would really like it if we can handle this entirely between the Network Service and the browser process, leaving the renderer process out of the loop completely.

We do have MIME sniffing running in the network service, right? It should be able to tell us whether we have any data or not. fergal@ mentioned earlier that one of the downsides to sniffing is that we don't know when we will get the FIN packet if that is how the server ends the connection. However, don't we have to already handle that case with the MIME sniffing code? If the server sends all headers and then delays the FIN packet, we won't respond back to the browser process with OnReceiveResponse as far as I understand the event flow.

So my question is: don't we get a conclusive signal in the Network Service with the sniffing logic whether we have 0 sized vs non-0 sized response body? I might be missing something and I would like to understand the full picture.

Thanks!

[Fergal Daly]

On Tue, 6 Oct 2020 at 08:08, Nasko Oskov <na...@chromium.org> wrote:

I have one more question here, since I would really like it if we can handle this entirely between the Network Service and the browser process, leaving the renderer process out of the loop completely.

We do have MIME sniffing running in the network service, right? It should be able to tell us whether we have any data or not. fergal@ mentioned earlier that one of the downsides to sniffing is that we don't know when we will get the FIN packet if that is how the server ends the connection. However, don't we have to already handle that case with the MIME sniffing code? If the server sends all headers and then delays the FIN packet, we won't respond back to the browser process with OnReceiveResponse as far as I understand the event flow.

Does MIME sniffing occur on 404s?

F

[Łukasz Anforowicz]

Yes - see how URLLoader::ContinueOnResponseStarted makes the |is_more_mime_sniffing_needed_| decision (ignoring the HTTP status code) as long as 1) net::ShouldSniffMimeType returns true and 2) mojom::kURLLoadOptionSniffMimeType is specified (it *is* specified in NavigationURLLoaderImpl::GetURLLoaderOptions).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CAAozHLmRVjTNkMV4t3DP_hhp90wMNmAbZSMzWT33NLYRd%3DsMEA%40mail.gmail.com.

[Fergal Daly]

On Tue, 6 Oct 2020 at 10:10, Łukasz Anforowicz <luk...@chromium.org> wrote:

Yes - see how URLLoader::ContinueOnResponseStarted makes the |is_more_mime_sniffing_needed_| decision (ignoring the HTTP status code) as long as 1) net::ShouldSniffMimeType returns true and 2) mojom::kURLLoadOptionSniffMimeType is specified (it *is* specified in NavigationURLLoaderImpl::GetURLLoaderOptions).

Thanks. It looks like text/html is not sniffable. So waiting for the FIN would slow down 404s that

- don't include content-length

- do include content-type: text/html

I don't know whether that's OK,

F

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CAA_NCUFypTv6v0wsOpKW6yre2HxmLuvy8qGtYi15DjbsHhgtLw%40mail.gmail.com.

[Charlie Reis]

We do confirmation sniffing of text/html files for CORB in a lot of cases, so I wouldn't rely on just that function to know whether we sniff the response.

Also, 404s don't seem like a particularly perf-sensitive case to me, and the addition of sniffing there (even in previous nosniff cases) might be ok if it significantly simplifies the implementation.  It seems like it might be an option worth considering, at least.

Charlie

[Fergal Daly]

On Tue, 6 Oct 2020 at 14:45, Charlie Reis <cr...@chromium.org> wrote:

We do confirmation sniffing of text/html files for CORB in a lot of cases, so I wouldn't rely on just that function to know whether we sniff the response.

Also, 404s don't seem like a particularly perf-sensitive case to me, and the addition of sniffing there (even in previous nosniff cases) might be ok if it significantly simplifies the implementation.  It seems like it might be an option worth considering, at least.

The user wouldn't know it's a 404 until the FIN arrives so the fact that it's a 404 doesn't really let you dismiss the performance.

I tested the current behaviour when there is no content, no content length and a long delay. As expected, as soon as the blank line after the headers was received, it navigated to a blank page. Nothing gave any indication that it was an error page (e.g. no 404 in the title). Then if I closed the connection from the server, the blank was replaced by a built-in error page.

So

1 waiting for X.com, still on old page

2 blank page

3 error

So this would change that to be 

1 waiting for X.com, still on old page

2 error

Doesn't seem awful.

Does something report back stats on response sizes and time to header, time to body etc?

F

[Rakina Zata Amni]

Just adding things for consideration here. Here's the CLs for the first approach: CL1, CL2 (Note that we found another renderer-inititated error page case with extensions, handled in CL1). They aren't that bad, although having two navigations is a bit weird.

If we solve the empty-body-with-error-HTTP-status-code case by waiting for the response body, we will:

1. Remove the new NavigateToErrorPage method added in frame.mojom
2. Remove RenderFrameImpl::RunScriptsAtDocumentReady
3. Add a new NavigationThrottle that waits for the response body and turn the navigation into a failed navigation. (is there a better way to do this?)

Most of the other parts (added in CL1) will stay the same because it's still needed by the other case in CL1.  Maybe we can change most of it to be more specific. Also, there's an open question on whether CL1 needs to do an error page navigation at all (see comment). If it turns out we don't need that, then we can remove most of the code.

[Nasko Oskov]

Just chiming in on one point, so it can be timely, since I might not have time to review the CLs today.

On Tue, Oct 6, 2020 at 6:56 AM Rakina Zata Amni <rak...@chromium.org> wrote:

Just adding things for consideration here. Here's the CLs for the first approach: CL1, CL2 (Note that we found another renderer-inititated error page case with extensions, handled in CL1). They aren't that bad, although having two navigations is a bit weird.

If we solve the empty-body-with-error-HTTP-status-code case by waiting for the response body, we will:

1. Remove the new NavigateToErrorPage method added in frame.mojom
2. Remove RenderFrameImpl::RunScriptsAtDocumentReady
3. Add a new NavigationThrottle that waits for the response body and turn the navigation into a failed navigation. (is there a better way to do this?)

I don't think we need a NavigationThrottle for this. The Network Service is performing the sniffing and will know whether the body is empty or not. This boolean value can be passed back to the NavigationURLLoader code, in theory. In practice, I'm not sure whether the interface is easy enough to extend, but this would be the first thing I would look at.

In general, I think the UI thread navigation code does not need to wait any further than the OnReceiveResponse and should be able to make a decision based on what the Network Service has concluded - either body or no body, depending on the various factors it knows about.

[Nasko Oskov]

On Tue, Oct 6, 2020 at 10:28 AM Nasko Oskov <na...@chromium.org> wrote:

Just chiming in on one point, so it can be timely, since I might not have time to review the CLs today.

On Tue, Oct 6, 2020 at 6:56 AM Rakina Zata Amni <rak...@chromium.org> wrote:

Just adding things for consideration here. Here's the CLs for the first approach: CL1, CL2 (Note that we found another renderer-inititated error page case with extensions, handled in CL1). They aren't that bad, although having two navigations is a bit weird.

If we solve the empty-body-with-error-HTTP-status-code case by waiting for the response body, we will:

1. Remove the new NavigateToErrorPage method added in frame.mojom
2. Remove RenderFrameImpl::RunScriptsAtDocumentReady
3. Add a new NavigationThrottle that waits for the response body and turn the navigation into a failed navigation. (is there a better way to do this?)

I don't think we need a NavigationThrottle for this. The Network Service is performing the sniffing and will know whether the body is empty or not. This boolean value can be passed back to the NavigationURLLoader code, in theory. In practice, I'm not sure whether the interface is easy enough to extend, but this would be the first thing I would look at.

Just did a quick skim over ULRResponseHead and it has encoded_data_lenght. I don't know whether it actually behaves the way we want it to, maybe it does? The struct has some other lenght members, but I'm not versed on exactly how they work.

[Rakina Zata Amni]

On Wed, Oct 7, 2020 at 2:32 AM Nasko Oskov <na...@chromium.org> wrote:

On Tue, Oct 6, 2020 at 10:28 AM Nasko Oskov <na...@chromium.org> wrote:

Just chiming in on one point, so it can be timely, since I might not have time to review the CLs today.

On Tue, Oct 6, 2020 at 6:56 AM Rakina Zata Amni <rak...@chromium.org> wrote:

Just adding things for consideration here. Here's the CLs for the first approach: CL1, CL2 (Note that we found another renderer-inititated error page case with extensions, handled in CL1). They aren't that bad, although having two navigations is a bit weird.

If we solve the empty-body-with-error-HTTP-status-code case by waiting for the response body, we will:

1. Remove the new NavigateToErrorPage method added in frame.mojom
2. Remove RenderFrameImpl::RunScriptsAtDocumentReady
3. Add a new NavigationThrottle that waits for the response body and turn the navigation into a failed navigation. (is there a better way to do this?)

I don't think we need a NavigationThrottle for this. The Network Service is performing the sniffing and will know whether the body is empty or not. This boolean value can be passed back to the NavigationURLLoader code, in theory. In practice, I'm not sure whether the interface is easy enough to extend, but this would be the first thing I would look at.

Just did a quick skim over ULRResponseHead and it has encoded_data_lenght. I don't know whether it actually behaves the way we want it to, maybe it does? The struct has some other lenght members, but I'm not versed on exactly how they work.

That is populated very early when we got the headers, so I don't think it has any information on the body. Reading the body for sniffing is further down. I think we can't depend on CORB & MIME sniffing to fully cover all the cases we want (CORB skips some cases like same-origin document I think) - CMIIW.

So we need a separate flag (similar to is_more_mime_sniffing_needed_) and also a new field in URLResponseHead to indicate that we know the body is empty/not-empty. Those aren't that hard to add I think (as we can follow the CORB/MIME sniffing path), except that we want it to be set only on our specific case (otherwise it'd slow down more things than we'd like): It's for a navigation, it's for the main frame, and it has an error status code. The last part is easy to get from the headers, but I don't know how to know about the others from within the URLLoader. Maybe there's a way to pass that info somehow? Does that sound reasonable? 

[Kinuko Yasuda]

On Wed, Oct 7, 2020 at 8:35 AM Rakina Zata Amni <rak...@chromium.org> wrote:

On Wed, Oct 7, 2020 at 2:32 AM Nasko Oskov <na...@chromium.org> wrote:

On Tue, Oct 6, 2020 at 10:28 AM Nasko Oskov <na...@chromium.org> wrote:

Just chiming in on one point, so it can be timely, since I might not have time to review the CLs today.

On Tue, Oct 6, 2020 at 6:56 AM Rakina Zata Amni <rak...@chromium.org> wrote:

Just adding things for consideration here. Here's the CLs for the first approach: CL1, CL2 (Note that we found another renderer-inititated error page case with extensions, handled in CL1). They aren't that bad, although having two navigations is a bit weird.

If we solve the empty-body-with-error-HTTP-status-code case by waiting for the response body, we will:

1. Remove the new NavigateToErrorPage method added in frame.mojom
2. Remove RenderFrameImpl::RunScriptsAtDocumentReady
3. Add a new NavigationThrottle that waits for the response body and turn the navigation into a failed navigation. (is there a better way to do this?)

I don't think we need a NavigationThrottle for this. The Network Service is performing the sniffing and will know whether the body is empty or not. This boolean value can be passed back to the NavigationURLLoader code, in theory. In practice, I'm not sure whether the interface is easy enough to extend, but this would be the first thing I would look at.

Just did a quick skim over ULRResponseHead and it has encoded_data_lenght. I don't know whether it actually behaves the way we want it to, maybe it does? The struct has some other lenght members, but I'm not versed on exactly how they work.

That is populated very early when we got the headers, so I don't think it has any information on the body. Reading the body for sniffing is further down. I think we can't depend on CORB & MIME sniffing to fully cover all the cases we want (CORB skips some cases like same-origin document I think) - CMIIW. 

So we need a separate flag (similar to is_more_mime_sniffing_needed_) and also a new field in URLResponseHead to indicate that we know the body is empty/not-empty. Those aren't that hard to add I think (as we can follow the CORB/MIME sniffing path), except that we want it to be set only on our specific case (otherwise it'd slow down more things than we'd like): It's for a navigation, it's for the main frame, and it has an error status code. The last part is easy to get from the headers, but I don't know how to know about the others from within the URLLoader. Maybe there's a way to pass that info somehow? Does that sound reasonable? 

Are we trying to wait for it in network service or somewhere else? (Sorry, haven't read all the thread yet)

URLLoader is created per request/response, and whether it's for navigation or not can be somewhat distinguished by the 'destination' field in the resource request.  There's also a flag is_main_frame in the request.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CACPC1r7erXtqJaqc%3DZLwSPqN_xQjRJo4b7AYdUaXQgnX0k3spw%40mail.gmail.com.

[Matt Falkenhagen]

2020年10月7日(水) 14:49 Kinuko Yasuda <kin...@chromium.org>:

On Wed, Oct 7, 2020 at 8:35 AM Rakina Zata Amni <rak...@chromium.org> wrote:

On Wed, Oct 7, 2020 at 2:32 AM Nasko Oskov <na...@chromium.org> wrote:

On Tue, Oct 6, 2020 at 10:28 AM Nasko Oskov <na...@chromium.org> wrote:

Just chiming in on one point, so it can be timely, since I might not have time to review the CLs today.

On Tue, Oct 6, 2020 at 6:56 AM Rakina Zata Amni <rak...@chromium.org> wrote:

Just adding things for consideration here. Here's the CLs for the first approach: CL1, CL2 (Note that we found another renderer-inititated error page case with extensions, handled in CL1). They aren't that bad, although having two navigations is a bit weird.

If we solve the empty-body-with-error-HTTP-status-code case by waiting for the response body, we will:

1. Remove the new NavigateToErrorPage method added in frame.mojom
2. Remove RenderFrameImpl::RunScriptsAtDocumentReady
3. Add a new NavigationThrottle that waits for the response body and turn the navigation into a failed navigation. (is there a better way to do this?)

I don't think we need a NavigationThrottle for this. The Network Service is performing the sniffing and will know whether the body is empty or not. This boolean value can be passed back to the NavigationURLLoader code, in theory. In practice, I'm not sure whether the interface is easy enough to extend, but this would be the first thing I would look at.

Just did a quick skim over ULRResponseHead and it has encoded_data_lenght. I don't know whether it actually behaves the way we want it to, maybe it does? The struct has some other lenght members, but I'm not versed on exactly how they work.

That is populated very early when we got the headers, so I don't think it has any information on the body. Reading the body for sniffing is further down. I think we can't depend on CORB & MIME sniffing to fully cover all the cases we want (CORB skips some cases like same-origin document I think) - CMIIW. 

So we need a separate flag (similar to is_more_mime_sniffing_needed_) and also a new field in URLResponseHead to indicate that we know the body is empty/not-empty. Those aren't that hard to add I think (as we can follow the CORB/MIME sniffing path), except that we want it to be set only on our specific case (otherwise it'd slow down more things than we'd like): It's for a navigation, it's for the main frame, and it has an error status code. The last part is easy to get from the headers, but I don't know how to know about the others from within the URLLoader. Maybe there's a way to pass that info somehow? Does that sound reasonable? 

Are we trying to wait for it in network service or somewhere else? (Sorry, haven't read all the thread yet)

URLLoader is created per request/response, and whether it's for navigation or not can be somewhat distinguished by the 'destination' field in the resource request.  There's also a flag is_main_frame in the request.

Naively it seems to me it's better to do this as a URLLoaderThrottle since not all responses come from the network service. For example, a service worker can provide the response.

As Rakina pointed out in a chat to this code pointer, for mime sniffing we seem to either do it as a URLLoaderThrottle for "intercepted" requests and rely on the network service to do its own sniffing for "non-intercepted" requests. I'm not sure why we do it in two different places, seems more complex than just always using a throttle.

[Rakina Zata Amni]

Hi all,

I've gathered some more information, and I think we should be ready to make a decision on which approach we should take. I've updated the doc, but here's our choices and a bit more details:

Proposal 1: Wait for body in the renderer -> call NavigateToErrorPage

Pros:

• No delays for non-empty-body cases
• Implementation is somewhat simpler? Most of the parts from LoadPostCommitErrorPage can be reused (CL)
• The other error case (MimeHandlerView) can reuse this path and commit an actual error page instead of showing a blank page.

Cons:

• Needs 2 commits
• "automatic error navigation" might be confusing?
• Need to be very clear on the difference with LoadPostCommitErrorPage

Proposal 2: Wait for body in the browser

Pros:

• Needs only 1 commit
• Less confusing in general as there will be no "automatic error navigations".

Cons:

• Impl is a bit more complex as we need a new MainFrameErrorURLLoaderThrottle and MainFrameErrorURLLoader (probably we can generalize parts of MimeSniffingThrottle and MimeSniffingURLLoader)
• The MimeHandlerView case will navigate to about:blank instead (this is actually OK, but less nice than proposal 1)
• Some delays for non-empty-body cases, but maybe not significant as most likely we'll already do CORB & MIME sniffing.

Both of the approaches sounds reasonable to me, although I might be missing some details on the URLLoaderThrottle/Proposal 2 Impl. If I got it right, Proposal 1 is simpler, but Proposal 2 has less surprises. If adding a new URLLoaderThrottle sounds ok and doesn't have hidden pitfalls, probably it's better to go with that approach. Let me know your thoughts!