Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2563423004:
Always send a fail or finish notification for each navigation. (Closed)
|
Created:
4 years ago by Nate Chapin Modified:
3 years, 11 months ago CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, loading-reviews_chromium.org, tyoshino+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAlways send a fail or finish notification for each navigation.
Historically, we have dropped these when navigations are interleaved.
BUG=656919
Committed: https://crrev.com/b22a26e4c12a8fc5c22d623e99674335e26cf5e3
Cr-Commit-Position: refs/heads/master@{#440862}
Patch Set 1 #Patch Set 2 : cleanup #Patch Set 3 : fix unit tests #
Total comments: 13
Patch Set 4 : Rebase #
Messages
Total messages: 38 (23 generated)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Always send a fail or finish notification for each navigation. Historically, we have dropped these when navigations are interleaved. BUG= ========== to ========== Always send a fail or finish notification for each navigation. Historically, we have dropped these when navigations are interleaved. BUG=656919 ==========
japhet@chromium.org changed reviewers: + dcheng@chromium.org, gsennton@chromium.org
dcheng: PTAL This is a quirk from time immemorial, dating to a time when the definitions of didFinishLoad/didFailLoad/didStopLoading weren't as delineated as they are now. gsennton: FYI https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:634: frameLoader()->loadFailed(this, ResourceError::cancelledError(url())); This feels a little hacky, but it ensures we always send a didFail(). I don't see a great alternative to it. https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.h (left): https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.h:277: MainResourceDone, This state isn't actually used for anything, it's functionally equivalent to Committed. https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (left): https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:89: documentLoader = DocumentLoader::create( Same as in BaseAudioContextTest. https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:116: childDocumentLoader = DocumentLoader::create( Same as in BaseAudioContextTest. https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:153: documentLoader = DocumentLoader::create( Same as in BaseAudioContextTest. https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left): https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:678: if (!frame->loader().stateMachine()->committedFirstRealDocumentLoad()) setSentDidFinishLoad() in init() instead. https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1476: m_progressTracker->progressCompleted(); progressCompleted() should be called if appropriate in the checkCompleted() call below, so long as setSentDidFinishLoad() is called. https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp (left): https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp:103: m_childDocumentLoader = DocumentLoader::create( This DocumentLoader isn't fully attached to the frame, and dies in loadFailed() during teardown. This test works just fine using the initial DocumentLoader created in init().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for looking at this Nate! Added a comment inline. https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:634: frameLoader()->loadFailed(this, ResourceError::cancelledError(url())); On 2016/12/12 21:40:37, Nate Chapin (slow until Jan 3) wrote: > This feels a little hacky, but it ensures we always send a didFail(). I don't > see a great alternative to it. I'm not sure I understand when we should receive a didFinishLoad and when we should receive a didFailLoad. Today we receive didFinishLoad for loads where the request fails but we do receive some content from the server (and here you suggest posting didFailLoad if the request fails but the response contains no data?). OnPageFinished is only posted if we receive didFinishLoad (and in some specific cases of didFailLoad - namely if we end up with the error NetError.ERR_ABORTED). Should the behaviour between failing requests with, and without content (we still receive a response, it just contains no data) from the server actually differ?
On 2016/12/13 13:59:56, gsennton wrote: > Thanks for looking at this Nate! Added a comment inline. > > https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): > > https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/DocumentLoader.cpp:634: > frameLoader()->loadFailed(this, ResourceError::cancelledError(url())); > On 2016/12/12 21:40:37, Nate Chapin (slow until Jan 3) wrote: > > This feels a little hacky, but it ensures we always send a didFail(). I don't > > see a great alternative to it. > > I'm not sure I understand when we should receive a didFinishLoad and when we > should receive a didFailLoad. > Today we receive didFinishLoad for loads where the request fails but we do > receive some content from the server (and here you suggest posting didFailLoad > if the request fails but the response contains no data?). OnPageFinished is only > posted if we receive didFinishLoad (and in some specific cases of didFailLoad - > namely if we end up with the error NetError.ERR_ABORTED). > > Should the behaviour between failing requests with, and without content (we > still receive a response, it just contains no data) from the server actually > differ? In blink-land, there are two case that result in a didFailLoad: (1) a network error for the main resource (i.e. a genuine failure to load, not merely a 4xx/5xx http status code), and (2) the navigation is cancelled before completion. This change is trying to do a better job of catching implicit cancellations and turning them in to didFailLoad() callbacks. In the specific case of a 4xx/5xx response with no content, as 656919 describes, the navigation is getting cancelled when the navigation to the error page commits, but it wasn't triggering a didFailLoad(). This is because historically we've only labelled navigations as a cancel when the main resource's network request hadn't completed (and thus we actually cancelled something) or the stop is explicit (i.e., js calling window.stop() or the user hitting the stop button). Did that answer your question of the "why" of this patch?
LGTM with some random thoughts (no action required) Unrelated though: I kind of wish we could forbid calls back into Blink for these notifications. I blocked didFinishLoad() during frame detach, but I guess didFailLoad(), etc are not blocked. Somewhat more related random thought... I'm kind of wondering if we can somehow document/clean up the either/or mechanism eventually. It does make sense if you're familiar with the system: if a DocumentLoader is marked as set sent did finish load, we don't resend, and we mark this bit in the load failed case. However it's also a bit hard to follow at the same time, since it's split between DocumentLoader and FrameLoader... https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left): https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1476: m_progressTracker->progressCompleted(); On 2016/12/12 21:40:38, Nate Chapin (slow until Jan 3) wrote: > progressCompleted() should be called if appropriate in the checkCompleted() call > below, so long as setSentDidFinishLoad() is called. Seems subtle, but I agree this should work. https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:230: // generate start notifications. Unrelated, but do you think it'd be problematic to send start / finish for initial empty documents? https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp (left): https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp:92: m_childDocumentLoader.clear(); Yay, simplification.
https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:230: // generate start notifications. On 2016/12/14 23:01:28, dcheng wrote: > Unrelated, but do you think it'd be problematic to send start / finish for > initial empty documents? Extensions might inject content scripts into initial empty documents? I haven't tested to see what if anything would break, it's just been the convention to keep the initial empty document as a mostly-invisible blink implementation detail.
On 2016/12/14 22:17:49, Nate Chapin (slow until Jan 3) wrote: > On 2016/12/13 13:59:56, gsennton wrote: > > Thanks for looking at this Nate! Added a comment inline. > > > > > https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): > > > > > https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/loader/DocumentLoader.cpp:634: > > frameLoader()->loadFailed(this, ResourceError::cancelledError(url())); > > On 2016/12/12 21:40:37, Nate Chapin (slow until Jan 3) wrote: > > > This feels a little hacky, but it ensures we always send a didFail(). I > don't > > > see a great alternative to it. > > > > I'm not sure I understand when we should receive a didFinishLoad and when we > > should receive a didFailLoad. > > Today we receive didFinishLoad for loads where the request fails but we do > > receive some content from the server (and here you suggest posting didFailLoad > > if the request fails but the response contains no data?). OnPageFinished is > only > > posted if we receive didFinishLoad (and in some specific cases of didFailLoad > - > > namely if we end up with the error NetError.ERR_ABORTED). > > > > Should the behaviour between failing requests with, and without content (we > > still receive a response, it just contains no data) from the server actually > > differ? > > In blink-land, there are two case that result in a didFailLoad: (1) a network > error for the main resource (i.e. a genuine failure to load, not merely a > 4xx/5xx http status code), and (2) the navigation is cancelled before > completion. This change is trying to do a better job of catching implicit > cancellations and turning them in to didFailLoad() callbacks. In the specific > case of a 4xx/5xx response with no content, as 656919 describes, the navigation > is getting cancelled when the navigation to the error page commits, but it > wasn't triggering a didFailLoad(). This is because historically we've only > labelled navigations as a cancel when the main resource's network request hadn't > completed (and thus we actually cancelled something) or the stop is explicit > (i.e., js calling window.stop() or the user hitting the stop button). > > Did that answer your question of the "why" of this patch? Yes, that makes this a lot clearer to me, thank you :). This is the long-term fix you mentioned to fix didFailLoad/didFinishLoad behaviour, right? It looks like this CL fixes the onPageFinished problem for empty responses because you are failing the load with ERR_ABORTED (so that it looks like the error was aborted through user-action) and in that case we post onPageFinished from AwWebContentsObserver.didFailLoad, see https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an... so that's great! :) A couple of points/questions: 1. Do we want this to act like a cancellation? (using the ERR_ABORTED error code doesn't seem super intuitive to me but on the other hand it does seem to fix the problem). 2. It annoys me a bit that we don't call onReceivedError for this failure-case, but the current implementation (AwContents.onReceivedError, here https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an... ) doesn't post onReceivedError for requests that fail with ERR_ABORTED anyway. 3. I assume we don't want to merge this fix to 56? So I guess the best we can do in 56 is to emulate this behaviour through WebView-specific code. I suspect listening to didFinishLoad(kUnreachableUrl) won't be equivalent to listening to didFailLoad(pageUrl, ERR_ABORTED)?
On 2016/12/15 16:37:48, gsennton wrote: > On 2016/12/14 22:17:49, Nate Chapin (slow until Jan 3) wrote: > > On 2016/12/13 13:59:56, gsennton wrote: > > > Thanks for looking at this Nate! Added a comment inline. > > > > > > > > > https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): > > > > > > > > > https://codereview.chromium.org/2563423004/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/loader/DocumentLoader.cpp:634: > > > frameLoader()->loadFailed(this, ResourceError::cancelledError(url())); > > > On 2016/12/12 21:40:37, Nate Chapin (slow until Jan 3) wrote: > > > > This feels a little hacky, but it ensures we always send a didFail(). I > > don't > > > > see a great alternative to it. > > > > > > I'm not sure I understand when we should receive a didFinishLoad and when we > > > should receive a didFailLoad. > > > Today we receive didFinishLoad for loads where the request fails but we do > > > receive some content from the server (and here you suggest posting > didFailLoad > > > if the request fails but the response contains no data?). OnPageFinished is > > only > > > posted if we receive didFinishLoad (and in some specific cases of > didFailLoad > > - > > > namely if we end up with the error NetError.ERR_ABORTED). > > > > > > Should the behaviour between failing requests with, and without content (we > > > still receive a response, it just contains no data) from the server actually > > > differ? > > > > In blink-land, there are two case that result in a didFailLoad: (1) a network > > error for the main resource (i.e. a genuine failure to load, not merely a > > 4xx/5xx http status code), and (2) the navigation is cancelled before > > completion. This change is trying to do a better job of catching implicit > > cancellations and turning them in to didFailLoad() callbacks. In the specific > > case of a 4xx/5xx response with no content, as 656919 describes, the > navigation > > is getting cancelled when the navigation to the error page commits, but it > > wasn't triggering a didFailLoad(). This is because historically we've only > > labelled navigations as a cancel when the main resource's network request > hadn't > > completed (and thus we actually cancelled something) or the stop is explicit > > (i.e., js calling window.stop() or the user hitting the stop button). > > > > Did that answer your question of the "why" of this patch? > > Yes, that makes this a lot clearer to me, thank you :). > This is the long-term fix you mentioned to fix didFailLoad/didFinishLoad > behaviour, right? > > It looks like this CL fixes the onPageFinished problem for empty responses > because you are failing the load with ERR_ABORTED (so that it looks like the > error was aborted through user-action) and in that case we post onPageFinished > from AwWebContentsObserver.didFailLoad, see > https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an... > > so that's great! :) > > A couple of points/questions: > 1. Do we want this to act like a cancellation? (using the ERR_ABORTED error code > doesn't seem super intuitive to me but on the other hand it does seem to fix the > problem). Maybe we should have a special error code for the case where we replace a navigation with an error page, but for now, cancellation is probably the closest thing we have. It is a cancellation in the sense that a page that hadn't gone through 100% of the navigation process was cancelled because we navigated again. > 2. It annoys me a bit that we don't call onReceivedError for this failure-case, > but the current implementation (AwContents.onReceivedError, here > https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an... > ) doesn't post onReceivedError for requests that fail with ERR_ABORTED anyway. I think that's because the network request for the main resource wasn't cancelled: it finished successfully (i.e., a 4xx/5xx status code isn't an "error"). The cancellation happens after we've finished loading and parsing the main resource, so the navigation fails, but no network request fails. This is counter-intuitive, if not actually crazy. > 3. I assume we don't want to merge this fix to 56? So I guess the best we can do > in 56 is to emulate this behaviour through WebView-specific code. I suspect > listening to didFinishLoad(kUnreachableUrl) won't be equivalent to listening to > didFailLoad(pageUrl, ERR_ABORTED)? Yeah, don't merge this patch. My experience with simple-looking changes to core callbacks like this is that the first attempt will almost certainly be reverted due to some untested assumption somewhere in the codebase :) I think you ought to be able to get the relevant information by stashing the committed document's url when a navigation to kUnreachableUrl starts? Not the exact same flow, but hopefully close enough?
> Yeah, don't merge this patch. My experience with simple-looking changes to core > callbacks like this is that the first attempt will almost certainly be reverted > due to some untested assumption somewhere in the codebase :) Alright, sounds reasonable :) > > I think you ought to be able to get the relevant information by stashing the > committed document's url when a navigation to kUnreachableUrl starts? Not the > exact same flow, but hopefully close enough? What I'm worrying about is any cases I might be missing - I'm not familiar with what can happen during a single navigation. I think the most likely failure scenario would be that we start posting more onPageFinshed calls from didStopLoading and that these will duplicate already posted calls from either didFailLoad or onReceivedError. In the case of didFailLoad this shouldn't be a problem as we could just avoid posting onPageFinished from didStopLoading if we just posted onPageFinished from didFailLoad but in the case of onReceivedError I'm not sure whether it's that easy (that callback is posted from another code location). A couple of additional questions: 1. could we call didFailLoad with error code ERR_ABORTED several times within a pair of didStartLoading/didStopLoading calls or would that only happen once, thus cancelling the entire navigation? 2. could we end up failing loads several times between one single pair of didStartLoading and didStopLoading? (e.g. loadUrl(X) -> failed to find server -> loadUrl(Y) -> failed to connect to server -> etc.).
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/16 18:17:30, gsennton (OOO Dec 24 - Jan 2) wrote: > > A couple of additional questions: > 1. could we call didFailLoad with error code ERR_ABORTED several times within a > pair of didStartLoading/didStopLoading calls or would that only happen once, > thus cancelling the entire navigation? Before this CL, I think there should be only one ERR_ABORTED per didStartLoading/didStopLoading pair. After this CL, multiple ERR_ABORTED codes are possible with the following seequence: didStartLoading foo.com/a didStartProvisionalLoad foo.com/b didStartProvisionalLoad foo.com/a didFailLoad -> ERR_ABORTED (when foo.com/b commits before foo.com/a completes) foo.com/b didFailLoad -> ERR_ABORTED (user clicks stop) didStopLoading > 2. could we end up failing loads several times between one single pair of > didStartLoading and didStopLoading? (e.g. loadUrl(X) -> failed to find server -> > loadUrl(Y) -> failed to connect to server -> etc.). I think you can have at most one non-ERR_ABORTED didFailLoad in a given didStartLoading/didStopLoading pair.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2563423004/#ps60001 (title: "Rebase")
The CQ bit was unchecked by japhet@chromium.org
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2563423004/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
rtoy@chromium.org changed reviewers: + rtoy@chromium.org
lgtm
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482947106860510,
"parent_rev": "198290437b5c0b3384f639dcea46f20ea9f22693", "commit_rev":
"37eff422205990a6e9f602e5ad0b2a8c86b6fbc7"}
Message was sent while issue was closed.
Description was changed from ========== Always send a fail or finish notification for each navigation. Historically, we have dropped these when navigations are interleaved. BUG=656919 ========== to ========== Always send a fail or finish notification for each navigation. Historically, we have dropped these when navigations are interleaved. BUG=656919 Review-Url: https://codereview.chromium.org/2563423004 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Always send a fail or finish notification for each navigation. Historically, we have dropped these when navigations are interleaved. BUG=656919 Review-Url: https://codereview.chromium.org/2563423004 ========== to ========== Always send a fail or finish notification for each navigation. Historically, we have dropped these when navigations are interleaved. BUG=656919 Committed: https://crrev.com/b22a26e4c12a8fc5c22d623e99674335e26cf5e3 Cr-Commit-Position: refs/heads/master@{#440862} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b22a26e4c12a8fc5c22d623e99674335e26cf5e3 Cr-Commit-Position: refs/heads/master@{#440862} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
