Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1440933004:
Don't use didStopLoading in failed navigation when other nav in progress (Closed)
|
Created:
5 years, 1 month ago by gsennton Modified:
5 years ago CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, loading-reviews_chromium.org, mnaganov (inactive), tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't use didStopLoading in failed navigation when other nav in progress
For example if a JS redirect would fail while the original navigation is
still in progress we would call didStopLoading before the original
navigation calls didFinishLoading (breaking the assumption that
didFinishLoading is always called before didStopLoading).
BUG=557100, 298193
Committed: https://crrev.com/e1da704f67f34022b7aaf3afe277c86217844114
Cr-Commit-Position: refs/heads/master@{#362954}
Patch Set 1 #Patch Set 2 : Don't dispatch didFailLoad if already dispatched a call declaring the load finished. #Patch Set 3 : Fix layout tests after help from mkwst@ and add WebView instrumentation test. #Patch Set 4 : Remove unnecessary DidStopLoadingDetailsWithPending browser test. #Patch Set 5 : Remove DidStopLoadingDetails as well #
Total comments: 1
Patch Set 6 : Move OnFailedLoadHelper to TestAwContentsClient. #Messages
Total messages: 32 (13 generated)
gsennton@chromium.org changed reviewers: + japhet@chromium.org
Hi Japhet, so here is the fix you suggested, AFAICT it fixes the problem we have in WebView with didFinishLoading being dispatched after didStopLoading (which breaks the CL in https://codereview.chromium.org/1432083004/ and prevents us from firing onPageFinished).
I am also creating a WebView instrumentation test to make sure we post an onPageFinished for the original page when a JS redirect fails. The test doesn't entirely suit our needs -- we only want to make sure that we post an onPageFinished when the JS redirect fails because of shouldOverrideUrlLoading, but afaict there is no way to make the shouldOverrideUrlLoading call wait until the redirect has reached its provisional state, without reaching a deadlock (and if we don't wait until the provisional state is reached the test can be flaky). It is probably simpler to add the test in another CL?
Description was changed from ========== Don't use didStopLoading in failed navigation when other nav in progress For example if a JS redirect would fail while the original navigation is still in progress we would call didStopLoading before the original navigation calls didFinishLoading (breaking the assumption that didFinishLoading is always called before didStopLoading). BUG= ========== to ========== Don't use didStopLoading in failed navigation when other nav in progress For example if a JS redirect would fail while the original navigation is still in progress we would call didStopLoading before the original navigation calls didFinishLoading (breaking the assumption that didFinishLoading is always called before didStopLoading). BUG=557100 ==========
Code looks good Can we test callback ordering closer to FrameLoader? We do something similiar for the success case in WebFrameTest.CallbackOrdering.
On 2015/11/17 18:03:18, Nate Chapin wrote: > Code looks good > > Can we test callback ordering closer to FrameLoader? We do something similiar > for the success case in WebFrameTest.CallbackOrdering. The case we want to test here is: Load a page with a JS redirect and then fail the JS redirect in its provisional state. We would need to make sure that the original navigation does not finish until the JS redirect has failed (otherwise we will call didFinish for the original page and won't see the bug we want to fix here). When it comes to the failing tests: for the couple of tests I have looked at (e.g. fast/events/onload-webkit-before-webcore.html) it seems we early out of checkCompleted because allDescendentsAreComplete returns false (some child is loading), but the child never calls checkCompleted() for its parent...
Ok, so it seems the problem lies in what is happening in the provisional case. At least in the following test cases fast/events/focusingUnloadedFrame.html fast/events/onloadFrameCrash.html fast/events/onload-webkit-before-webcore.html where we receive an error for the provisional loader but then later in shouldSendCompleteNotification(), sentDidFinishLoad() is still false for the document loader. I'm not sure how we should proceed here; it seems weird to fiddle with the document loader when the provisional loader fails, but on the other hand, in these cases the document loader doesn't seem to finish...
The CQ bit was checked by knn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440933004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440933004/40001
knn@chromium.org changed reviewers: - knn@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
gsennton@chromium.org changed reviewers: + creis@chromium.org, mnaganov@chromium.org
Ok, so the only test that is failing now is the good old WebContentsImplBrowserTest.DidStopLoadingDetailsWithPending (actually I'm failing both that and DidStopLoadingDetails). After taking a closer look it seems the test is essentially waiting for a didStopLoad when a new navigation replaces an old one. (it is waiting for a notification for the message NOTIFICATION_LOAD_STOP which is dispatched from WebContentsImpl::DidStopLoading -> WebContentsImpl::SetIsLoading -> NotificationService::current()->Notify(NOTIFICATION_LOAD_STOP, ...) So AFAICT now that we don't call didStopLoad directly when a load fails the test is waiting until the second navigation finishes and then tries to match the first URL (expected) to the second one (actual url after didStopLoading). So one potential way of fixing the test would be to observe the current URL after didFailLoad instead of didStopLoad (whether or not this is testing what it should be I'm not sure). The CL that added this test was https://chromiumcodereview.appspot.com/14066022 so I'm adding creis@ here. creis@ do you have any input regarding what the DidStopLoadingDetails/DidStopLoadingDetailsWithPending tests are testing? Context: The meaning of didFinishLoad / didStopLoad has changed with https://codereview.chromium.org/1381003004/ so that didFailLoad/didFinishLoad is dispatched when a load finishes while didStopLoad is only dispatched when there are no navigations in progress. This CL is fixing a case which that CL missed: one where didFail causes a didStopLoad even when there are navigations still in progress.
On 2015/12/01 18:37:58, gsennton wrote: > Ok, so the only test that is failing now is the good old > WebContentsImplBrowserTest.DidStopLoadingDetailsWithPending (actually I'm > failing both that and DidStopLoadingDetails). After taking a closer look it > seems the test is essentially waiting for a didStopLoad when a new navigation > replaces an old one. (it is waiting for a notification for the message > NOTIFICATION_LOAD_STOP which is dispatched from WebContentsImpl::DidStopLoading > -> WebContentsImpl::SetIsLoading -> > NotificationService::current()->Notify(NOTIFICATION_LOAD_STOP, ...) > > > So AFAICT now that we don't call didStopLoad directly when a load fails the test > is waiting until the second navigation finishes and then tries to match the > first URL (expected) to the second one (actual url after didStopLoading). > > So one potential way of fixing the test would be to observe the current URL > after didFailLoad instead of didStopLoad (whether or not this is testing what it > should be I'm not sure). > > The CL that added this test was > https://chromiumcodereview.appspot.com/14066022?_ga=1.10580535.283996421.1442... > so I'm adding creis@ here. > creis@ do you have any input regarding what the > DidStopLoadingDetails/DidStopLoadingDetailsWithPending tests are testing? DidStopLoadingDetailsWithPending was meant to verify that the DidStopLoading event contained the URL that just committed and not the pending URL if a new navigation had started already. It seems strange to me to fire DidFailLoad in that case, because the first navigation did successfully commit and stop loading. Why does starting a second navigation before LoadStop turn it into a fail? (Note that the second navigation might never commit and could turn out to be a download/204/etc, leaving us on the otherwise successful first page. I wouldn't consider the first one to be a failed load, but I'm probably missing some context for the decision.) At any rate, if that is the new behavior, then I agree that the test should listen for DidFailLoad instead. You can verify that the test is still meaningful by regressing the fix in WebContentsImpl::DidStopLoading (using GetActiveEntry instead of GetLastCommittedEntry on line 4118) and making sure the test fails. Note that DidStopLoadingDetails is basically a sanity check with just a single navigation. I'm surprised if that's failing as well, since I'm not sure how it would have been affected. > > Context: > The meaning of didFinishLoad / didStopLoad has changed with > https://codereview.chromium.org/1381003004/ > so that didFailLoad/didFinishLoad is dispatched when a load finishes while > didStopLoad is only dispatched when there are no navigations in progress. > > This CL is fixing a case which that CL missed: one where didFail causes a > didStopLoad even when there are navigations still in progress.
> Why does starting a second navigation before LoadStop turn it into a fail? AFAICT it is not the new navigation that fails the load but rather the call to shell->Stop() (triggered by the NavigateOnCommitObserver). Note that this test passes anyway if the render-side finishes before we call shell->Stop(), i.e. if the first load dispatches didStopLoading before the shell->Stop() call has propagated to the render side. So what happens if shell->Stop() finishes before we call didStopLoading for the first load is that we end up in WebLocalFrameImpl::stopAllLoaders() and then DocumentLoader::cancelMainResourceLoad with an error representing a cancellation of the load (and this error results in a call to didFailLoad). So in this latter case the first load did indeed commit but it never truly finished. So we should be listening to both didFinishLoad and didFailLoad to catch both these cases.. What annoys me right now is that in the latter case above (where the first navigation is cancelled) it seems we dispatch both didFinishLoading, and then because we haven't dispatched didStopLoad when the navigation is cancelled we also dispatch didFailLoad (which might not break anything but it is not the intended behaviour since https://codereview.chromium.org/1381003004/).
Now I'm replying over my own message so please see #16 before reading this... Note: The first test (DidStopLoadingDetails) is passing consistently (not sure why I saw it failing yesterday, and it has been passing on the bots for all my patch-versions). I am not entirely sure how NavigationContoller.GetActiveEntry follows the render side during a navigation but I think these browser tests (or at least the second one, i.e. DidStopLoadingDetailsWithPending) are obsolete now that we only call didStopLoading when there are no ongoing navigations. Both didFinishLoading and didFailLoading have their url passed as parameters so it doesn't make sense to check their values (well, we could check their values but that wouldn't check your fix in https://chromiumcodereview.appspot.com/14066022). I.e. now that didStopLoading is only dispatched when we are no longer loading anything the last committed entry should always be the same as the active entry (in didStopLoading). So I suggest simply removing the test DidStopLoadingDetailsWithPending :) (we could potentially test that we use the last committed url in didStopLoading through first committing a load and then failing a second load before the second load is committed to make sure that the first load is the one being used in didStopLoading).
On 2015/12/02 11:33:59, gsennton wrote: > > Why does starting a second navigation before LoadStop turn it into a fail? > > AFAICT it is not the new navigation that fails the load but rather the call to > shell->Stop() (triggered by the NavigateOnCommitObserver). Ok, I'll defer on the behavior. I could see it making sense to end the loading state for the first navigation before starting the loading state for the second one, if that's the motivation. > Note that this test passes anyway if the render-side finishes before we call > shell->Stop(), i.e. if the first load dispatches didStopLoading before the > shell->Stop() call has propagated to the render side. > So what happens if shell->Stop() finishes before we call didStopLoading for the > first load is that we end up in WebLocalFrameImpl::stopAllLoaders() and then > DocumentLoader::cancelMainResourceLoad with an error representing a cancellation > of the load (and this error results in a call to didFailLoad). > So in this latter case the first load did indeed commit but it never truly > finished. > > So we should be listening to both didFinishLoad and didFailLoad to catch both > these cases.. > > What annoys me right now is that in the latter case above (where the first > navigation is cancelled) it seems we dispatch both didFinishLoading, and then > because we haven't dispatched didStopLoad when the navigation is cancelled we > also dispatch didFailLoad (which might not break anything but it is not the > intended behaviour since https://codereview.chromium.org/1381003004/). On 2015/12/02 12:06:44, gsennton wrote: > Now I'm replying over my own message so please see #16 before reading this... > > Note: The first test (DidStopLoadingDetails) is passing consistently (not sure > why I saw it failing yesterday, and it has been passing on the bots for all my > patch-versions). Ah, good. I didn't realize/remember all these tests were marked flaky (https://crbug.com/298193), which might explain it. > I am not entirely sure how NavigationContoller.GetActiveEntry follows the render > side during a navigation but I think these browser tests (or at least the second > one, i.e. DidStopLoadingDetailsWithPending) are obsolete now that we only call > didStopLoading when there are no ongoing navigations. Both didFinishLoading and > didFailLoading have their url passed as parameters so it doesn't make sense to > check their values (well, we could check their values but that wouldn't check > your fix in https://chromiumcodereview.appspot.com/14066022). > > I.e. now that didStopLoading is only dispatched when we are no longer loading > anything the last committed entry should always be the same as the active entry > (in didStopLoading). > > So I suggest simply removing the test DidStopLoadingDetailsWithPending :) If that's the new behavior, then I agree the DidStopLoadingDetailsWithPending test isn't meaningful and can be removed. Feel free to remove DidStopLoadingDetails as well, which was just a sanity check and apparently suffers from flakiness. > (we could potentially test that we use the last committed url in didStopLoading > through first committing a load and then failing a second load before the second > load is committed to make sure that the first load is the one being used in > didStopLoading). I wouldn't worry about that.
Description was changed from ========== Don't use didStopLoading in failed navigation when other nav in progress For example if a JS redirect would fail while the original navigation is still in progress we would call didStopLoading before the original navigation calls didFinishLoading (breaking the assumption that didFinishLoading is always called before didStopLoading). BUG=557100 ========== to ========== Don't use didStopLoading in failed navigation when other nav in progress For example if a JS redirect would fail while the original navigation is still in progress we would call didStopLoading before the original navigation calls didFinishLoading (breaking the assumption that didFinishLoading is always called before didStopLoading). TBR=mnaganov@chromium.org BUG=557100 ==========
gsennton@chromium.org changed reviewers: + mkwst@chromium.org
> If that's the new behavior, then I agree the DidStopLoadingDetailsWithPending > test isn't meaningful and can be removed. Feel free to remove > DidStopLoadingDetails as well, which was just a sanity check and apparently > suffers from flakiness. Ok, I've removed both tests. Could you (creis@) review content/ ? :) The android_webview/ parts have been approved by mnaganov@ in https://codereview.chromium.org/1457343002/ so will TBR him for those. mkwst@ PTAL at third_party/Webkit :) (those are the changes discussed in https://codereview.chromium.org/1481063002/)
On 2015/12/02 22:26:27, gsennton wrote: > > If that's the new behavior, then I agree the DidStopLoadingDetailsWithPending > > test isn't meaningful and can be removed. Feel free to remove > > DidStopLoadingDetails as well, which was just a sanity check and apparently > > suffers from flakiness. > > Ok, I've removed both tests. Could you (creis@) review content/ ? :) web_contents_impl_browsertest.cc LGTM. (I'm not a good reviewer for TestCallbackHelperContainer.java; has mnaganov@ or another Android reviewer looked at that one?) Also, please list 298193 in the BUG line of the CL description, so that this CL gets mentioned there. Thanks! > > The android_webview/ parts have been approved by mnaganov@ in > https://codereview.chromium.org/1457343002/ so will TBR him for those. > > mkwst@ PTAL at third_party/Webkit :) > (those are the changes discussed in https://codereview.chromium.org/1481063002/)
Yes, I saw the android_webview/ part before and blessed it. Just one trivial comment came up since then. LGTM. https://codereview.chromium.org/1440933004/diff/80001/content/public/test/and... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java (right): https://codereview.chromium.org/1440933004/diff/80001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:77: public static class OnFailedLoadHelper extends CallbackHelper { I think it will make more sense to define this helper in TestAwContentsClient.
core/loader LGTM.
Description was changed from ========== Don't use didStopLoading in failed navigation when other nav in progress For example if a JS redirect would fail while the original navigation is still in progress we would call didStopLoading before the original navigation calls didFinishLoading (breaking the assumption that didFinishLoading is always called before didStopLoading). TBR=mnaganov@chromium.org BUG=557100 ========== to ========== Don't use didStopLoading in failed navigation when other nav in progress For example if a JS redirect would fail while the original navigation is still in progress we would call didStopLoading before the original navigation calls didFinishLoading (breaking the assumption that didFinishLoading is always called before didStopLoading). BUG=557100, 298193 ==========
The CQ bit was checked by gsennton@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, creis@chromium.org, mnaganov@chromium.org Link to the patchset: https://codereview.chromium.org/1440933004/#ps100001 (title: "Move OnFailedLoadHelper to TestAwContentsClient.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440933004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440933004/100001
Message was sent while issue was closed.
Description was changed from ========== Don't use didStopLoading in failed navigation when other nav in progress For example if a JS redirect would fail while the original navigation is still in progress we would call didStopLoading before the original navigation calls didFinishLoading (breaking the assumption that didFinishLoading is always called before didStopLoading). BUG=557100, 298193 ========== to ========== Don't use didStopLoading in failed navigation when other nav in progress For example if a JS redirect would fail while the original navigation is still in progress we would call didStopLoading before the original navigation calls didFinishLoading (breaking the assumption that didFinishLoading is always called before didStopLoading). BUG=557100, 298193 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Don't use didStopLoading in failed navigation when other nav in progress For example if a JS redirect would fail while the original navigation is still in progress we would call didStopLoading before the original navigation calls didFinishLoading (breaking the assumption that didFinishLoading is always called before didStopLoading). BUG=557100, 298193 ========== to ========== Don't use didStopLoading in failed navigation when other nav in progress For example if a JS redirect would fail while the original navigation is still in progress we would call didStopLoading before the original navigation calls didFinishLoading (breaking the assumption that didFinishLoading is always called before didStopLoading). BUG=557100, 298193 Committed: https://crrev.com/e1da704f67f34022b7aaf3afe277c86217844114 Cr-Commit-Position: refs/heads/master@{#362954} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e1da704f67f34022b7aaf3afe277c86217844114 Cr-Commit-Position: refs/heads/master@{#362954} |
