Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1227823010:
Handle empty error pages in DidFinishDocumentLoad (Closed)
|
Created:
5 years, 5 months ago by Nate Chapin Modified:
5 years, 5 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle empty error pages in DidFinishDocumentLoad
We ought to show an error message when a 4xx/5xx http response has no body, because the alternative is a blank page. This logic is currently broken.
Rather than trying to go guess whether the document is non-empty, plumb that bit out in DidFinishDocumentLoad.
BUG=331745
Committed: https://crrev.com/0ee02e6e52469f9260395f2ddd22bf4d6e75b828
Cr-Commit-Position: refs/heads/master@{#339286}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : +test and comment in didFinishDocumentLoad #
Total comments: 6
Patch Set 4 : Fix comments #Patch Set 5 : Disable test on android, like the other error page tests #
Messages
Total messages: 44 (19 generated)
japhet@chromium.org changed reviewers: + creis@chromium.org, dcheng@chromium.org
This depends on https://codereview.chromium.org/1229263003
No big objections to the approach, but I don't understand the bug or how this fixes it. Can you clarify? Please also add a test for it. https://codereview.chromium.org/1227823010/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1227823010/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:2825: if (!empty) This condition isn't clear. It's hard to see how |empty| is related to whether there's an error page or not-- it sounded like the error page itself might or might not have things in it, and an empty document might not have an error page. Can you clarify the name and/or add a comment showing the connection between them? https://codereview.chromium.org/1227823010/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:2849: LoadNavigationErrorPage(frame->dataSource()->request(), error, true); What's the difference between calling this here vs didFinishResourceLoad? I'm not sure how far apart those are in time, or what the implications are.
The case described by the bug is: * Server returns a 4xx/5xx response code for a main resource. * The main resource response body is empty. * We therefore commit an empty document. * It's a bad user experience to show a blank page without explanation when we error, so we want to replace the empty document with an error document. (This is the part that is currently broken) We currently use a "needs error page" bit to get the desired error page in this case. It is set when a provisional load starts; the bug is that we incorrectly unset at commit time. The previous correct behavior was to unset it in a now-removed didReceiveDocumentData() callback. What we ultimately need to know is whether main resource data was ever received, and the most straightforward way to (re-)plumb that information is to add a "did we ever receive main resource data" bit to the message that we send when we finish parsing the main resource, didFinishDocumentLoad(). In terms of the move from didFinishResourceLoad(), it's a bit strange to have the error page policy decision there. That function is called for every resource load, not just main resources, even though the policy decision only applies to main resources. However, we do have a function for main resource completion, didFinishDocumentLoad(). I moved it for two reasons, (1) put logic specific to main resource loading together, and (2) put the error page logic in a place where it can easily and directly read whether the document is empty, rather than needing to stash a bit that approximates blink's internal state. It's worth noting that this is not the only place we trigger an error page load. It is, however, the only case where we do so without a traditional failure callback (i.e., a didFailLoad or didFailProvisionalLoad).
On 2015/07/13 17:40:33, Nate Chapin wrote: > The case described by the bug is: > * Server returns a 4xx/5xx response code for a main resource. > * The main resource response body is empty. > * We therefore commit an empty document. > * It's a bad user experience to show a blank page without explanation when we > error, so we want to replace the empty document with an error document. (This is > the part that is currently broken) > > We currently use a "needs error page" bit to get the desired error page in this > case. It is set when a provisional load starts; the bug is that we incorrectly > unset at commit time. The previous correct behavior was to unset it in a > now-removed didReceiveDocumentData() callback. What we ultimately need to know > is whether main resource data was ever received, and the most straightforward > way to (re-)plumb that information is to add a "did we ever receive main > resource data" bit to the message that we send when we finish parsing the main > resource, didFinishDocumentLoad(). > > In terms of the move from didFinishResourceLoad(), it's a bit strange to have > the error page policy decision there. That function is called for every resource > load, not just main resources, even though the policy decision only applies to > main resources. However, we do have a function for main resource completion, > didFinishDocumentLoad(). I moved it for two reasons, (1) put logic specific to > main resource loading together, and (2) put the error page logic in a place > where it can easily and directly read whether the document is empty, rather than > needing to stash a bit that approximates blink's internal state. > > It's worth noting that this is not the only place we trigger an error page load. > It is, however, the only case where we do so without a traditional failure > callback (i.e., a didFailLoad or didFailProvisionalLoad). CL updated. Let me know if things are still obtuse.
Thanks, this helps. Also glad we have a test for it now! LGTM with nits. On 2015/07/13 17:40:33, Nate Chapin wrote: > The case described by the bug is: > * Server returns a 4xx/5xx response code for a main resource. > * The main resource response body is empty. > * We therefore commit an empty document. > * It's a bad user experience to show a blank page without explanation when we > error, so we want to replace the empty document with an error document. (This is > the part that is currently broken) > > We currently use a "needs error page" bit to get the desired error page in this > case. It is set when a provisional load starts; the bug is that we incorrectly > unset at commit time. The previous correct behavior was to unset it in a > now-removed didReceiveDocumentData() callback. What we ultimately need to know > is whether main resource data was ever received, and the most straightforward > way to (re-)plumb that information is to add a "did we ever receive main > resource data" bit to the message that we send when we finish parsing the main > resource, didFinishDocumentLoad(). So the connection between "whether main resource data was ever received" and whether we show an error page is that we'll always show content if it's received (even for error pages), but we might additionally show an error page if there's no content. Ok, works for me. > > In terms of the move from didFinishResourceLoad(), it's a bit strange to have > the error page policy decision there. That function is called for every resource > load, not just main resources, even though the policy decision only applies to > main resources. However, we do have a function for main resource completion, > didFinishDocumentLoad(). I moved it for two reasons, (1) put logic specific to > main resource loading together, and (2) put the error page logic in a place > where it can easily and directly read whether the document is empty, rather than > needing to stash a bit that approximates blink's internal state. This sounds reasonable to me. > It's worth noting that this is not the only place we trigger an error page load. > It is, however, the only case where we do so without a traditional failure > callback (i.e., a didFailLoad or didFailProvisionalLoad). https://codereview.chromium.org/1227823010/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1227823010/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2856: // we may want to displaying our own error page, so the user doesn't end up s/displaying/display/ https://codereview.chromium.org/1227823010/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2871: // Display error page, if appropriate. nit: Display an error page instead of a blank page, if appropriate. https://codereview.chromium.org/1227823010/diff/40001/content/renderer/render... File content/renderer/render_view_browsertest.cc (right): https://codereview.chromium.org/1227823010/diff/40001/content/renderer/render... content/renderer/render_view_browsertest.cc:2095: main_frame->didFinishDocumentLoad(web_frame, true); Sanity check: This won't get flaky if the actual data URL later finishes, right?
https://codereview.chromium.org/1227823010/diff/40001/content/renderer/render... File content/renderer/render_view_browsertest.cc (right): https://codereview.chromium.org/1227823010/diff/40001/content/renderer/render... content/renderer/render_view_browsertest.cc:2095: main_frame->didFinishDocumentLoad(web_frame, true); On 2015/07/13 20:42:43, Charlie Reis wrote: > Sanity check: This won't get flaky if the actual data URL later finishes, right? It shouldn't. didFinishDocumentLoad() should synchronously start the navigation to the error page, which should cancel the data url navigation.
https://codereview.chromium.org/1227823010/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1227823010/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2856: // we may want to displaying our own error page, so the user doesn't end up On 2015/07/13 20:42:43, Charlie Reis wrote: > s/displaying/display/ Done. https://codereview.chromium.org/1227823010/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2871: // Display error page, if appropriate. On 2015/07/13 20:42:43, Charlie Reis wrote: > nit: Display an error page instead of a blank page, if appropriate. Done.
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1227823010/#ps60001 (title: "Fix comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1227823010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
lgtm for components/test_runner
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1227823010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1227823010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1227823010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1227823010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1227823010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1227823010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/1227823010/#ps80001 (title: "Disable test on android, like the other error page tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1227823010/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0ee02e6e52469f9260395f2ddd22bf4d6e75b828 Cr-Commit-Position: refs/heads/master@{#339286} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
