Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2580753002:
Fix NavigationControllerBrowserTest. EnsureSamePageNavigationUpdatesFrameNavigationEntry browser nav (Closed)
DescriptionEnsure that duplicate navigations are tagged as reload in the browser.
This includes the cases where we are navigating to a URL which was
committed previously or to a URL for which we haven't received a commit.
scottmg's patch for reference is here https://codereview.chromium.org/2381503003. This was reverted due to failures on Android.
The failure was seen in cases like this. Navigation to url1->url2->url1.
If the first navigation is not committed yet, then we would treat the last
navigation to url1 as a reload. This causes inbox on Android to hang.
The proposed fix is to use the last pending navigation if it exists or
the last committed navigation to compare whether the current navigation is
a reload. This fixes the issues with Android
Changes in this patch are as below:
1. Add members in the NavigationControllerImpl class to hold the last
pending navigation entry, its index, and the last transient index.
The last transient index is used to avoid treating navigations as
reloads if we have an interstitial being displayed.
2. Add a flag in the NavigationEntryImpl class to indicate whether there
was a SSL error in the navigation. This was added to fix browser test
failures. We avoid treating navigations as reloads when the last
pending navigation has a SSL error.
3. We compare the url of the last pending or committed entry with the
current navigation entry to see if it is a reload. There are a number
of cases to consider here. Please look at the code.
4. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload
which tests various combinations of pending and committed navigations
to ensure that the navigation is tagged correctly as reload or not.
The non plznavigate case does not have the bug when the EnsureSamePageNavigationUpdatesFrameNavigationEntry test because Blink tells us the correct navigation history, i.e. whether a navigation is a
reload, etc.
R=nasko@chromium.org,clamy@chromium.org
BUG=630103, 475027, 536102, 664319, 676235
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/3bdd8aea0439f2c6a50d24c3c805bc7e579d5a2c
Cr-Commit-Position: refs/heads/master@{#440443}
Patch Set 1 #Patch Set 2 : Skip data urls #Patch Set 3 : Fix compile failures #Patch Set 4 : Fix redness #
Total comments: 2
Patch Set 5 : Check if the base URL is not empty #Patch Set 6 : Added unittest NavigationControllerTest.MultipleNavigationsAndReload #Patch Set 7 : Fix browser_tests redness. #Patch Set 8 : Fix compile failures #Patch Set 9 : Skip pending navigations with SSL errors. #Patch Set 10 : Fix compile failures #Patch Set 11 : Fix asan failures. #Patch Set 12 : Fix page transition check to properly identify reloads. #Patch Set 13 : Move the code which determines which transitions to treat as forced reloads to its own function Sho… #
Total comments: 4
Patch Set 14 : Address review comments #Patch Set 15 : Set is_loading_ to true in RenderFrameHostImpl::OnBeginNavigate(). This identifies the start of a n… #Patch Set 16 : Fix comment #Patch Set 17 : Fix redness caused by a CHECK in WebContentsObserverSanityChecker for the is_loading_ flag. We need… #Patch Set 18 : Avoid the is_loading_ check for PlzNavigate with a comment explaining why. #Patch Set 19 : Revert changes to set is_loading_ to true in navigation request start as they are unrelated to the … #Patch Set 20 : Revert changes to browser-side-navigation.linux.content_browsertests.filter. Will do that in a sepa… #
Messages
Total messages: 114 (85 generated)
Description was changed from ========== Revert "Revert of PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… (patchset #21 id:500001 of https://codereview.chromium.org/2381503003/ )" This reverts commit fe5ea3f7cb872ea6fa2e560cfefc9185393a8cb2. BUG= ========== to ========== Revert "Revert of PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… (patchset #21 id:500001 of https://codereview.chromium.org/2381503003/ )" This reverts commit fe5ea3f7cb872ea6fa2e560cfefc9185393a8cb2. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Revert "Revert of PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… (patchset #21 id:500001 of https://codereview.chromium.org/2381503003/ )" This reverts commit fe5ea3f7cb872ea6fa2e560cfefc9185393a8cb2. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102 ==========
Description was changed from ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102 ========== to ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ananta@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 ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… Relanding this with an additional check to skip data urls to see if it fixes the android gmail spinner bug http://crbug.com/664319 This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ananta@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by ananta@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
ananta@chromium.org changed reviewers: + creis@chromium.org, scottmg@chromium.org
Thanks-- seems like a good direction to mainly stick with the previous fix and look for a way to tweak it. Might help to better understand what went wrong in Android WebView. https://codereview.chromium.org/2580753002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2580753002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1797: !pending_entry_->GetURL().SchemeIs(url::kDataScheme) && I like the idea of keeping essentially the same fix as before, but we should be careful about just excluding all data URLs. I note that the test this CL is enabling (EnsureSamePageNavigationUpdatesFrameNavigationEntry) was added to prevent the document's URL and origin from getting out of sync, which is bad for security. (See https://crbug.com/630103#c5.) Would skipping this check mean PlzNavigate is vulnerable to that bug for data URLs? We could limit it further to the LoadDataWithBaseURL case, but I'd have the same question there (about making that case vulnerable to the URL/origin mismatch). I note that the lines below already tried to address Android WebView's loadDataWithBaseURL. Maybe they did so in the wrong way, and a different check would work better? Maybe it's worth talking with some Android Webview folks (e.g., sgurun or boliu) to get a sense for why the original patch caused the Inbox bug (https://crbug.com/664319).
Description was changed from ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… Relanding this with an additional check to skip data urls to see if it fixes the android gmail spinner bug http://crbug.com/664319 This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… Added a check for whether the Base URL for data URLs match as well before classifying it as a reload. This should hopefully address the gmail spinner bug http://crbug.com/664319. This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
https://codereview.chromium.org/2580753002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2580753002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1797: !pending_entry_->GetURL().SchemeIs(url::kDataScheme) && On 2016/12/15 22:28:09, Charlie Reis (OOO soon) wrote: > I like the idea of keeping essentially the same fix as before, but we should be > careful about just excluding all data URLs. > > I note that the test this CL is enabling > (EnsureSamePageNavigationUpdatesFrameNavigationEntry) was added to prevent the > document's URL and origin from getting out of sync, which is bad for security. > (See https://crbug.com/630103#c5.) Would skipping this check mean PlzNavigate > is vulnerable to that bug for data URLs? > > We could limit it further to the LoadDataWithBaseURL case, but I'd have the same > question there (about making that case vulnerable to the URL/origin mismatch). > > I note that the lines below already tried to address Android WebView's > loadDataWithBaseURL. Maybe they did so in the wrong way, and a different check > would work better? > > Maybe it's worth talking with some Android Webview folks (e.g., sgurun or boliu) > to get a sense for why the original patch caused the Inbox bug > (https://crbug.com/664319). Thanks. I checked with boliu. He mentioned that the virtual URL is what is dislayed to the user, the URL contains the whole URL. In this case the data:, etc. The base URL could contain whatever is passed by the app. I added a check for the base URL as well based on our discussion. Thanks
ananta@chromium.org changed reviewers: + boliu@chromium.org
The CQ bit was checked by ananta@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...
+boliu
creis@chromium.org changed reviewers: + sgurun@chromium.org
Seems worth a try. boliu@ or sgurun@, is there any way to try this CL out to see if it will regress the Inbox app (like https://crbug.com/664319)? That would be worth confirming before landing again, if possible. Apart from that, I suppose this LGTM.
I will give it a try. However, is it possible to change the title to indicate that it is not only a test fix. (it helps a lot when bisecting as most people tend to skip test related CLs)
On 2016/12/16 01:13:29, sgurun wrote: > I will give it a try. However, is it possible to change the title to indicate > that it is not only a test fix. (it helps a lot when bisecting as most people > tend to skip test related CLs) it seemed to work ok in inbox. However, it is a pretty big if, so hard to say :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
On 2016/12/16 01:28:02, sgurun wrote: > On 2016/12/16 01:13:29, sgurun wrote: > > I will give it a try. However, is it possible to change the title to indicate > > that it is not only a test fix. (it helps a lot when bisecting as most people > > tend to skip test related CLs) > > it seemed to work ok in inbox. However, it is a pretty big if, so hard to say :) actually our QA engineer tested it (before I event asked - heyoo) and found the issue still existing.
On 2016/12/16 19:21:31, sgurun wrote: > On 2016/12/16 01:28:02, sgurun wrote: > > On 2016/12/16 01:13:29, sgurun wrote: > > > I will give it a try. However, is it possible to change the title to > indicate > > > that it is not only a test fix. (it helps a lot when bisecting as most > people > > > tend to skip test related CLs) > > > > it seemed to work ok in inbox. However, it is a pretty big if, so hard to say > :) > > actually our QA engineer tested it (before I event asked - heyoo) and found the > issue still existing. Thanks for checking! Are there steps for Ananta to test it locally so that we can confirm we find a working fix? (Also, agreed about the CL subject/description. Would be worth cleaning them up before landing.)
On 2016/12/16 19:32:08, Charlie Reis (OOO soon) wrote: > On 2016/12/16 19:21:31, sgurun wrote: > > On 2016/12/16 01:28:02, sgurun wrote: > > > On 2016/12/16 01:13:29, sgurun wrote: > > > > I will give it a try. However, is it possible to change the title to > > indicate > > > > that it is not only a test fix. (it helps a lot when bisecting as most > > people > > > > tend to skip test related CLs) > > > > > > it seemed to work ok in inbox. However, it is a pretty big if, so hard to > say > > :) > > > > actually our QA engineer tested it (before I event asked - heyoo) and found > the > > issue still existing. > > Thanks for checking! Are there steps for Ananta to test it locally so that we > can confirm we find a working fix? > > (Also, agreed about the CL subject/description. Would be worth cleaning them up > before landing.) I can show how to repro.
Ok, I don't understand what inbox is trying to do, but here's what happens. Each
time a webview (which displays a single message in a thread) is scrolled
offscreen and then onscreen again, inbox calls:
loadUrl("about:blank")
loadUrl("file:///message/<digits>/msg-a:r<digits>")
First these are normal loads, ie similar to user typing an url in a browser.
They are not LoadDataWithBaseURL
The file url is the same for the same message, so scrolling the same message
into view again would be loading the exact same file url again
iirc, the patch that broke this was making loads of the same url a reload?
The part I don't understand is why that even matters here, because there's that
loadUrl(about:blank) in between. Although the two loads happen in rapid
succession, possibly in the same task, so maybe about:blank hasn't committed
yet?
I think I'd approach this from the other side. Clearly inbox is waiting for some
callback to fire, eg onPageFinished. So the question is then, what callback
doesn't fire in a reload, but does in a normal navigation?
On 2016/12/16 22:50:43, boliu wrote:
> Ok, I don't understand what inbox is trying to do, but here's what happens.
Each
> time a webview (which displays a single message in a thread) is scrolled
> offscreen and then onscreen again, inbox calls:
> loadUrl("about:blank")
> loadUrl("file:///message/<digits>/msg-a:r<digits>")
>
> First these are normal loads, ie similar to user typing an url in a browser.
> They are not LoadDataWithBaseURL
>
> The file url is the same for the same message, so scrolling the same message
> into view again would be loading the exact same file url again
>
> iirc, the patch that broke this was making loads of the same url a reload?
>
> The part I don't understand is why that even matters here, because there's
that
> loadUrl(about:blank) in between. Although the two loads happen in rapid
> succession, possibly in the same task, so maybe about:blank hasn't committed
> yet?
>
> I think I'd approach this from the other side. Clearly inbox is waiting for
some
> callback to fire, eg onPageFinished. So the question is then, what callback
> doesn't fire in a reload, but does in a normal navigation?
Another possibility is that inbox expects webview to make a request to the file
url again, and they are ready to intercept that request. And for a reload, that
request no longer happens? Purely guessing here..
On 2016/12/16 22:52:53, boliu wrote:
> On 2016/12/16 22:50:43, boliu wrote:
> > Ok, I don't understand what inbox is trying to do, but here's what happens.
> Each
> > time a webview (which displays a single message in a thread) is scrolled
> > offscreen and then onscreen again, inbox calls:
> > loadUrl("about:blank")
> > loadUrl("file:///message/<digits>/msg-a:r<digits>")
> >
> > First these are normal loads, ie similar to user typing an url in a browser.
> > They are not LoadDataWithBaseURL
> >
> > The file url is the same for the same message, so scrolling the same message
> > into view again would be loading the exact same file url again
> >
> > iirc, the patch that broke this was making loads of the same url a reload?
> >
> > The part I don't understand is why that even matters here, because there's
> that
> > loadUrl(about:blank) in between. Although the two loads happen in rapid
> > succession, possibly in the same task, so maybe about:blank hasn't committed
> > yet?
> >
> > I think I'd approach this from the other side. Clearly inbox is waiting for
> some
> > callback to fire, eg onPageFinished. So the question is then, what callback
> > doesn't fire in a reload, but does in a normal navigation?
>
> Another possibility is that inbox expects webview to make a request to the
file
> url again, and they are ready to intercept that request. And for a reload,
that
> request no longer happens? Purely guessing here..
it would actually be not difficult to trace webview callbacks with and without
the patch and dump here :)
On 2016/12/16 22:55:12, sgurun wrote:
> On 2016/12/16 22:52:53, boliu wrote:
> > On 2016/12/16 22:50:43, boliu wrote:
> > > Ok, I don't understand what inbox is trying to do, but here's what
happens.
> > Each
> > > time a webview (which displays a single message in a thread) is scrolled
> > > offscreen and then onscreen again, inbox calls:
> > > loadUrl("about:blank")
> > > loadUrl("file:///message/<digits>/msg-a:r<digits>")
> > >
> > > First these are normal loads, ie similar to user typing an url in a
browser.
> > > They are not LoadDataWithBaseURL
> > >
> > > The file url is the same for the same message, so scrolling the same
message
> > > into view again would be loading the exact same file url again
> > >
> > > iirc, the patch that broke this was making loads of the same url a reload?
> > >
> > > The part I don't understand is why that even matters here, because there's
> > that
> > > loadUrl(about:blank) in between. Although the two loads happen in rapid
> > > succession, possibly in the same task, so maybe about:blank hasn't
committed
> > > yet?
> > >
> > > I think I'd approach this from the other side. Clearly inbox is waiting
for
> > some
> > > callback to fire, eg onPageFinished. So the question is then, what
callback
> > > doesn't fire in a reload, but does in a normal navigation?
> >
> > Another possibility is that inbox expects webview to make a request to the
> file
> > url again, and they are ready to intercept that request. And for a reload,
> that
> > request no longer happens? Purely guessing here..
>
> it would actually be not difficult to trace webview callbacks with and without
> the patch and dump here :)
Thanks for helping debug this guys :)
On 2016/12/16 23:09:52, ananta wrote:
> On 2016/12/16 22:55:12, sgurun wrote:
> > On 2016/12/16 22:52:53, boliu wrote:
> > > On 2016/12/16 22:50:43, boliu wrote:
> > > > Ok, I don't understand what inbox is trying to do, but here's what
> happens.
> > > Each
> > > > time a webview (which displays a single message in a thread) is scrolled
> > > > offscreen and then onscreen again, inbox calls:
> > > > loadUrl("about:blank")
> > > > loadUrl("file:///message/<digits>/msg-a:r<digits>")
> > > >
> > > > First these are normal loads, ie similar to user typing an url in a
> browser.
> > > > They are not LoadDataWithBaseURL
> > > >
> > > > The file url is the same for the same message, so scrolling the same
> message
> > > > into view again would be loading the exact same file url again
> > > >
> > > > iirc, the patch that broke this was making loads of the same url a
reload?
> > > >
> > > > The part I don't understand is why that even matters here, because
there's
> > > that
> > > > loadUrl(about:blank) in between. Although the two loads happen in rapid
> > > > succession, possibly in the same task, so maybe about:blank hasn't
> committed
> > > > yet?
> > > >
> > > > I think I'd approach this from the other side. Clearly inbox is waiting
> for
> > > some
> > > > callback to fire, eg onPageFinished. So the question is then, what
> callback
> > > > doesn't fire in a reload, but does in a normal navigation?
Btw, I was expecting someone knowledgeable to just answer this ^^
Inbox has a gazillion webviews flying around, and keeping track of them in logs
is...challenging.
> > >
> > > Another possibility is that inbox expects webview to make a request to the
> > file
> > > url again, and they are ready to intercept that request. And for a reload,
> > that
> > > request no longer happens? Purely guessing here..
> >
> > it would actually be not difficult to trace webview callbacks with and
without
> > the patch and dump here :)
>
> Thanks for helping debug this guys :)
On 2016/12/16 23:18:59, boliu wrote:
> On 2016/12/16 23:09:52, ananta wrote:
> > On 2016/12/16 22:55:12, sgurun wrote:
> > > On 2016/12/16 22:52:53, boliu wrote:
> > > > On 2016/12/16 22:50:43, boliu wrote:
> > > > > Ok, I don't understand what inbox is trying to do, but here's what
> > happens.
> > > > Each
> > > > > time a webview (which displays a single message in a thread) is
scrolled
> > > > > offscreen and then onscreen again, inbox calls:
> > > > > loadUrl("about:blank")
> > > > > loadUrl("file:///message/<digits>/msg-a:r<digits>")
> > > > >
> > > > > First these are normal loads, ie similar to user typing an url in a
> > browser.
> > > > > They are not LoadDataWithBaseURL
> > > > >
> > > > > The file url is the same for the same message, so scrolling the same
> > message
> > > > > into view again would be loading the exact same file url again
> > > > >
> > > > > iirc, the patch that broke this was making loads of the same url a
> reload?
> > > > >
> > > > > The part I don't understand is why that even matters here, because
> there's
> > > > that
> > > > > loadUrl(about:blank) in between. Although the two loads happen in
rapid
> > > > > succession, possibly in the same task, so maybe about:blank hasn't
> > committed
> > > > > yet?
> > > > >
> > > > > I think I'd approach this from the other side. Clearly inbox is
waiting
> > for
> > > > some
> > > > > callback to fire, eg onPageFinished. So the question is then, what
> > callback
> > > > > doesn't fire in a reload, but does in a normal navigation?
>
> Btw, I was expecting someone knowledgeable to just answer this ^^
>
> Inbox has a gazillion webviews flying around, and keeping track of them in
logs
> is...challenging.
>
> > > >
> > > > Another possibility is that inbox expects webview to make a request to
the
> > > file
> > > > url again, and they are ready to intercept that request. And for a
reload,
> > > that
> > > > request no longer happens? Purely guessing here..
> > >
> > > it would actually be not difficult to trace webview callbacks with and
> without
> > > the patch and dump here :)
> >
> > Thanks for helping debug this guys :)
in the working case, the callbacks end up with an onpagefinished of webview:
12-16 15:40:24.267 22926 22926 D WebViewCallback:
onPageCommitVisible=about:blank
12-16 15:40:24.267 22926 22926 D WebViewCallback:
onPageCommitVisible=file:///message/17754866/msg-f:1548385701084607269
12-16 15:40:24.347 22926 22926 D WebViewCallback: onScaleChangedScaled
12-16 15:40:24.376 22926 22926 D WebViewCallback: onProgressChanged=90
12-16 15:40:24.377 22926 22926 D WebViewCallback: onProgressChanged=100
12-16 15:40:24.377 22926 22926 D WebViewCallback: onProgressChanged=100
12-16 15:40:24.377 22926 22926 D WebViewCallback:
onReceivedTitle="msg-f:1548385701084607269"
12-16 15:40:24.377 22926 22926 D WebViewCallback:
onPageFinished=file:///message/17754866/msg-f:1548385701084607269
in the non-working case (with this CL) the callbacks end up with:
12-16 15:33:47.556 22042 22042 D WebViewCallback: onProgressChanged=10
12-16 15:33:47.566 22042 22042 W cr_BindingManager: Cannot call
determinedVisibility() - never saw a connection for the pid: 22042
12-16 15:33:47.567 22042 22042 I cr_AwContents:
org.chromium.android_webview.AwContents@288f222
insertVisualStateCallbackIfNotDestroyed
12-16 15:33:47.568 22042 22042 D WebViewCallback:
doUpdateVisitedHistory=about:blank reload=false
12-16 15:33:47.578 22042 22042 D WebViewCallback: onProgressChanged=100
12-16 15:33:47.579 22042 22042 D WebViewCallback: onProgressChanged=100
12-16 15:33:47.579 22042 22042 D WebViewCallback: onPageFinished=about:blank
12-16 15:33:47.586 22042 22042 D WebViewCallback:
onPageCommitVisible=about:blank
12-16 15:33:47.586 22042 22042 D WebViewCallback:
onPageCommitVisible=about:blank
12-16 15:33:47.586 22042 22042 D WebViewCallback: onScaleChangedScaled
we never get a onpagefinished for the file url.
On 2016/12/16 23:48:36, sgurun wrote:
> On 2016/12/16 23:18:59, boliu wrote:
> > On 2016/12/16 23:09:52, ananta wrote:
> > > On 2016/12/16 22:55:12, sgurun wrote:
> > > > On 2016/12/16 22:52:53, boliu wrote:
> > > > > On 2016/12/16 22:50:43, boliu wrote:
> > > > > > Ok, I don't understand what inbox is trying to do, but here's what
> > > happens.
> > > > > Each
> > > > > > time a webview (which displays a single message in a thread) is
> scrolled
> > > > > > offscreen and then onscreen again, inbox calls:
> > > > > > loadUrl("about:blank")
> > > > > > loadUrl("file:///message/<digits>/msg-a:r<digits>")
> > > > > >
> > > > > > First these are normal loads, ie similar to user typing an url in a
> > > browser.
> > > > > > They are not LoadDataWithBaseURL
> > > > > >
> > > > > > The file url is the same for the same message, so scrolling the same
> > > message
> > > > > > into view again would be loading the exact same file url again
> > > > > >
> > > > > > iirc, the patch that broke this was making loads of the same url a
> > reload?
> > > > > >
> > > > > > The part I don't understand is why that even matters here, because
> > there's
> > > > > that
> > > > > > loadUrl(about:blank) in between. Although the two loads happen in
> rapid
> > > > > > succession, possibly in the same task, so maybe about:blank hasn't
> > > committed
> > > > > > yet?
> > > > > >
> > > > > > I think I'd approach this from the other side. Clearly inbox is
> waiting
> > > for
> > > > > some
> > > > > > callback to fire, eg onPageFinished. So the question is then, what
> > > callback
> > > > > > doesn't fire in a reload, but does in a normal navigation?
> >
> > Btw, I was expecting someone knowledgeable to just answer this ^^
> >
> > Inbox has a gazillion webviews flying around, and keeping track of them in
> logs
> > is...challenging.
> >
> > > > >
> > > > > Another possibility is that inbox expects webview to make a request to
> the
> > > > file
> > > > > url again, and they are ready to intercept that request. And for a
> reload,
> > > > that
> > > > > request no longer happens? Purely guessing here..
> > > >
> > > > it would actually be not difficult to trace webview callbacks with and
> > without
> > > > the patch and dump here :)
> > >
> > > Thanks for helping debug this guys :)
>
> in the working case, the callbacks end up with an onpagefinished of webview:
> 12-16 15:40:24.267 22926 22926 D WebViewCallback:
> onPageCommitVisible=about:blank
> 12-16 15:40:24.267 22926 22926 D WebViewCallback:
> onPageCommitVisible=file:///message/17754866/msg-f:1548385701084607269
> 12-16 15:40:24.347 22926 22926 D WebViewCallback: onScaleChangedScaled
> 12-16 15:40:24.376 22926 22926 D WebViewCallback: onProgressChanged=90
> 12-16 15:40:24.377 22926 22926 D WebViewCallback: onProgressChanged=100
> 12-16 15:40:24.377 22926 22926 D WebViewCallback: onProgressChanged=100
> 12-16 15:40:24.377 22926 22926 D WebViewCallback:
> onReceivedTitle="msg-f:1548385701084607269"
> 12-16 15:40:24.377 22926 22926 D WebViewCallback:
> onPageFinished=file:///message/17754866/msg-f:1548385701084607269
>
>
> in the non-working case (with this CL) the callbacks end up with:
> 12-16 15:33:47.556 22042 22042 D WebViewCallback: onProgressChanged=10
> 12-16 15:33:47.566 22042 22042 W cr_BindingManager: Cannot call
> determinedVisibility() - never saw a connection for the pid: 22042
> 12-16 15:33:47.567 22042 22042 I cr_AwContents:
> org.chromium.android_webview.AwContents@288f222
> insertVisualStateCallbackIfNotDestroyed
> 12-16 15:33:47.568 22042 22042 D WebViewCallback:
> doUpdateVisitedHistory=about:blank reload=false
> 12-16 15:33:47.578 22042 22042 D WebViewCallback: onProgressChanged=100
> 12-16 15:33:47.579 22042 22042 D WebViewCallback: onProgressChanged=100
> 12-16 15:33:47.579 22042 22042 D WebViewCallback: onPageFinished=about:blank
> 12-16 15:33:47.586 22042 22042 D WebViewCallback:
> onPageCommitVisible=about:blank
> 12-16 15:33:47.586 22042 22042 D WebViewCallback:
> onPageCommitVisible=about:blank
> 12-16 15:33:47.586 22042 22042 D WebViewCallback: onScaleChangedScaled
>
>
> we never get a onpagefinished for the file url.
Thanks to sgurun for patiently debugging this with me on his setup. It looks
like inbox has multiple webviews in flight
at any given time each associated with its own WebContents and navigation
controllers.
1. A navigation is initiated to about:blank followed by a file:// url on one
webcontents
2. Another navigation is initiated to about:blank followed by a file://url on
another webcontents.
3. The about:blank navigation entry for 1 (webcontents 1) is committed.
4. Another about:blank navigation is initiated for webcontents 1.
5. At this point we convert the navigation to a reload which is wrong.
We traced in blink without our change. Blink always sets the navigation type to
new navigation for these sequences which
is correct.
It looks like we cannot assume a reload in the browser until we hear back about
all pending navigations in flight. I think a better
approach might be to fix this in blink/renderer where we have full context, or
in the IPCs coming from the renderer to the browser.
Description was changed from ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… Added a check for whether the Base URL for data URLs match as well before classifying it as a reload. This should hopefully address the gmail spinner bug http://crbug.com/664319. This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. This works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319 ==========
Description was changed from ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. This works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319 ========== to ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. This works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ananta@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ananta@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by ananta@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 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_...)
Description was changed from ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. This works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. scottmg's patch for reference is here https://codereview.chromium.org/2381503003. This was reverted due to failures on Android. The failure was seen in cases like this. Navigation to url1->url2->url1. If the first navigation is not committed yet, then we would treat the last navigation to url1 as a reload. This causes inbox on Android to hang. The proposed fix is to use the last pending navigation if it exists or the last committed navigation to compare whether the current navigation is a reload. Changes in this patch are as below: 1. Add members in the NavigationControllerImpl class to hold the last pending navigation entry, its index, and the last transient index. The last transient index is used to avoid treating navigations as reloads if we have an interstitial being displayed. 2. Add a flag in the NavigationEntryImpl class to indicate whether there was a SSL error in the navigation. This was added to fix browser test failures. We avoid treating navigations as reloads when the last pending navigation has a SSL error. 3. We compare the url of the last pending or committed entry with the current navigation entry to see if it is a reload. There are a number of cases to consider here. Please look at the code. 4. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. The non plznavigate case does not have the bug when the EnsureSamePageNavigationUpdatesFrameNavigationEntry test because Blink tells us the correct navigation history, i.e. whether a navigation is a reload, etc. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ananta@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 ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. scottmg's patch for reference is here https://codereview.chromium.org/2381503003. This was reverted due to failures on Android. The failure was seen in cases like this. Navigation to url1->url2->url1. If the first navigation is not committed yet, then we would treat the last navigation to url1 as a reload. This causes inbox on Android to hang. The proposed fix is to use the last pending navigation if it exists or the last committed navigation to compare whether the current navigation is a reload. Changes in this patch are as below: 1. Add members in the NavigationControllerImpl class to hold the last pending navigation entry, its index, and the last transient index. The last transient index is used to avoid treating navigations as reloads if we have an interstitial being displayed. 2. Add a flag in the NavigationEntryImpl class to indicate whether there was a SSL error in the navigation. This was added to fix browser test failures. We avoid treating navigations as reloads when the last pending navigation has a SSL error. 3. We compare the url of the last pending or committed entry with the current navigation entry to see if it is a reload. There are a number of cases to consider here. Please look at the code. 4. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. The non plznavigate case does not have the bug when the EnsureSamePageNavigationUpdatesFrameNavigationEntry test because Blink tells us the correct navigation history, i.e. whether a navigation is a reload, etc. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. scottmg's patch for reference is here https://codereview.chromium.org/2381503003. This was reverted due to failures on Android. The failure was seen in cases like this. Navigation to url1->url2->url1. If the first navigation is not committed yet, then we would treat the last navigation to url1 as a reload. This causes inbox on Android to hang. The proposed fix is to use the last pending navigation if it exists or the last committed navigation to compare whether the current navigation is a reload. This fixes the issues with Android Changes in this patch are as below: 1. Add members in the NavigationControllerImpl class to hold the last pending navigation entry, its index, and the last transient index. The last transient index is used to avoid treating navigations as reloads if we have an interstitial being displayed. 2. Add a flag in the NavigationEntryImpl class to indicate whether there was a SSL error in the navigation. This was added to fix browser test failures. We avoid treating navigations as reloads when the last pending navigation has a SSL error. 3. We compare the url of the last pending or committed entry with the current navigation entry to see if it is a reload. There are a number of cases to consider here. Please look at the code. 4. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. The non plznavigate case does not have the bug when the EnsureSamePageNavigationUpdatesFrameNavigationEntry test because Blink tells us the correct navigation history, i.e. whether a navigation is a reload, etc. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by ananta@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #12 (id:220001) has been deleted
The CQ bit was checked by ananta@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by ananta@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
ananta@chromium.org changed reviewers: + nasko@chromium.org
+nasko
The last_navigation thing seems ok to me, but I'm not sure about the SSL bits. Why does SSL need to be so special? Nice work on that test, definitely helps to have something that can cover the webview failure case with a more direct repro. https://codereview.chromium.org/2580753002/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2580753002/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:1799: pending_entry_index_ == last_committed_entry_index_ && Indent got messed up here. https://codereview.chromium.org/2580753002/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.h (right): https://codereview.chromium.org/2580753002/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.h:377: NavigationEntryImpl* last_pending_entry_; Does this need to be a raw pointer?
The site-isolation webkit_tests http/tests/intersection-observer/iframe-cross-origin.html failure could be related too? :( 10:24:51.154 21463 worker/1 http/tests/security/mixedContent/insecure-iframe-in-main-frame.html crashed, (stderr lines): 10:24:51.154 21463 [3637:3637:1220/102450.802932:6083714579:WARNING:render_frame_host_impl.cc(2116)] OnDidStopLoading was called twice. 10:24:51.154 21463 [3637:3637:1220/102450.818444:6083730092:WARNING:web_contents_impl.cc(3515)] https://127.0.0.1:8443/ ran insecure content from http://example.test:8080/security/mixedContent/resources/boring.html 10:24:51.154 21463 [3637:3637:1220/102450.843980:6083755629:WARNING:web_contents_impl.cc(3515)] https://127.0.0.1:8443/ ran insecure content from http://example.test:8080/security/mixedContent/resources/boring.html 10:24:51.154 21463 [3637:3637:1220/102450.848620:6083760263:WARNING:render_frame_host_impl.cc(2116)] OnDidStopLoading was called twice.
The SSL change was to fix the CaptivePortalBrowserTest.InterstitialTimerNavigate* tests. I don't fully understand these tests yet. However it looks like a navigation initiated to a page which should display a SSL interstitial on commit. The test navigates to the error page first and keeps the page hanging via a timer. It then navigates to the same page and the expectation is the SSL interstitial should show. Forcing a reload in this case causes the test to fail in the non plznavigate case. https://codereview.chromium.org/2580753002/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2580753002/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:1799: pending_entry_index_ == last_committed_entry_index_ && On 2016/12/20 22:34:05, scottmg (OOO soon) wrote: > Indent got messed up here. Thanks fixed https://codereview.chromium.org/2580753002/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.h (right): https://codereview.chromium.org/2580753002/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.h:377: NavigationEntryImpl* last_pending_entry_; On 2016/12/20 22:34:05, scottmg (OOO soon) wrote: > Does this need to be a raw pointer? This is on the same lines as pending_entry_ which is deleted in the DiscardPendingEntry function.
ananta@chromium.org changed reviewers: + clamy@chromium.org
+clamy
Description was changed from ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. scottmg's patch for reference is here https://codereview.chromium.org/2381503003. This was reverted due to failures on Android. The failure was seen in cases like this. Navigation to url1->url2->url1. If the first navigation is not committed yet, then we would treat the last navigation to url1 as a reload. This causes inbox on Android to hang. The proposed fix is to use the last pending navigation if it exists or the last committed navigation to compare whether the current navigation is a reload. This fixes the issues with Android Changes in this patch are as below: 1. Add members in the NavigationControllerImpl class to hold the last pending navigation entry, its index, and the last transient index. The last transient index is used to avoid treating navigations as reloads if we have an interstitial being displayed. 2. Add a flag in the NavigationEntryImpl class to indicate whether there was a SSL error in the navigation. This was added to fix browser test failures. We avoid treating navigations as reloads when the last pending navigation has a SSL error. 3. We compare the url of the last pending or committed entry with the current navigation entry to see if it is a reload. There are a number of cases to consider here. Please look at the code. 4. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. The non plznavigate case does not have the bug when the EnsureSamePageNavigationUpdatesFrameNavigationEntry test because Blink tells us the correct navigation history, i.e. whether a navigation is a reload, etc. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. scottmg's patch for reference is here https://codereview.chromium.org/2381503003. This was reverted due to failures on Android. The failure was seen in cases like this. Navigation to url1->url2->url1. If the first navigation is not committed yet, then we would treat the last navigation to url1 as a reload. This causes inbox on Android to hang. The proposed fix is to use the last pending navigation if it exists or the last committed navigation to compare whether the current navigation is a reload. This fixes the issues with Android Changes in this patch are as below: 1. Add members in the NavigationControllerImpl class to hold the last pending navigation entry, its index, and the last transient index. The last transient index is used to avoid treating navigations as reloads if we have an interstitial being displayed. 2. Add a flag in the NavigationEntryImpl class to indicate whether there was a SSL error in the navigation. This was added to fix browser test failures. We avoid treating navigations as reloads when the last pending navigation has a SSL error. 3. We compare the url of the last pending or committed entry with the current navigation entry to see if it is a reload. There are a number of cases to consider here. Please look at the code. 4. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. 5. Set is_loading_ to true in RenderFrameHostImpl::OnBeginNavigation. This should fix the redness with blink site-per-process layout tests with plznavigate. Unrelated with this patch The non plznavigate case does not have the bug when the EnsureSamePageNavigationUpdatesFrameNavigationEntry test because Blink tells us the correct navigation history, i.e. whether a navigation is a reload, etc. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ananta@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/20 22:37:00, scottmg (OOO soon) wrote: > The site-isolation webkit_tests > http/tests/intersection-observer/iframe-cross-origin.html failure could be > related too? :( > > 10:24:51.154 21463 worker/1 > http/tests/security/mixedContent/insecure-iframe-in-main-frame.html crashed, > (stderr lines): > 10:24:51.154 21463 > [3637:3637:1220/102450.802932:6083714579:WARNING:render_frame_host_impl.cc(2116)] > OnDidStopLoading was called twice. > 10:24:51.154 21463 > [3637:3637:1220/102450.818444:6083730092:WARNING:web_contents_impl.cc(3515)] > https://127.0.0.1:8443/ ran insecure content from > http://example.test:8080/security/mixedContent/resources/boring.html > 10:24:51.154 21463 > [3637:3637:1220/102450.843980:6083755629:WARNING:web_contents_impl.cc(3515)] > https://127.0.0.1:8443/ ran insecure content from > http://example.test:8080/security/mixedContent/resources/boring.html > 10:24:51.154 21463 > [3637:3637:1220/102450.848620:6083760263:WARNING:render_frame_host_impl.cc(2116)] > OnDidStopLoading was called twice. The iframe-cross-origin.html failure is not related with this patch. I discussed with nasko and we think that setting the is_loading_ flag to true when a navigation request is received in the browser should do the trick here. Added that change in the patch and trying it on the bots.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ananta@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. scottmg's patch for reference is here https://codereview.chromium.org/2381503003. This was reverted due to failures on Android. The failure was seen in cases like this. Navigation to url1->url2->url1. If the first navigation is not committed yet, then we would treat the last navigation to url1 as a reload. This causes inbox on Android to hang. The proposed fix is to use the last pending navigation if it exists or the last committed navigation to compare whether the current navigation is a reload. This fixes the issues with Android Changes in this patch are as below: 1. Add members in the NavigationControllerImpl class to hold the last pending navigation entry, its index, and the last transient index. The last transient index is used to avoid treating navigations as reloads if we have an interstitial being displayed. 2. Add a flag in the NavigationEntryImpl class to indicate whether there was a SSL error in the navigation. This was added to fix browser test failures. We avoid treating navigations as reloads when the last pending navigation has a SSL error. 3. We compare the url of the last pending or committed entry with the current navigation entry to see if it is a reload. There are a number of cases to consider here. Please look at the code. 4. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. 5. Set is_loading_ to true in RenderFrameHostImpl::OnBeginNavigation. This should fix the redness with blink site-per-process layout tests with plznavigate. Unrelated with this patch The non plznavigate case does not have the bug when the EnsureSamePageNavigationUpdatesFrameNavigationEntry test because Blink tells us the correct navigation history, i.e. whether a navigation is a reload, etc. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. scottmg's patch for reference is here https://codereview.chromium.org/2381503003. This was reverted due to failures on Android. The failure was seen in cases like this. Navigation to url1->url2->url1. If the first navigation is not committed yet, then we would treat the last navigation to url1 as a reload. This causes inbox on Android to hang. The proposed fix is to use the last pending navigation if it exists or the last committed navigation to compare whether the current navigation is a reload. This fixes the issues with Android Changes in this patch are as below: 1. Add members in the NavigationControllerImpl class to hold the last pending navigation entry, its index, and the last transient index. The last transient index is used to avoid treating navigations as reloads if we have an interstitial being displayed. 2. Add a flag in the NavigationEntryImpl class to indicate whether there was a SSL error in the navigation. This was added to fix browser test failures. We avoid treating navigations as reloads when the last pending navigation has a SSL error. 3. We compare the url of the last pending or committed entry with the current navigation entry to see if it is a reload. There are a number of cases to consider here. Please look at the code. 4. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. 5. Set is_loading_ to true in RenderFrameHostImpl::OnBeginNavigation. This should fix the redness with blink site-per-process layout tests with plznavigate. Unrelated with this patch. The associated bug is 676235 The non plznavigate case does not have the bug when the EnsureSamePageNavigationUpdatesFrameNavigationEntry test because Blink tells us the correct navigation history, i.e. whether a navigation is a reload, etc. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319, 676235 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ananta@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/12/21 00:25:48, ananta wrote: > On 2016/12/20 22:37:00, scottmg (OOO soon) wrote: > > The site-isolation webkit_tests > > http/tests/intersection-observer/iframe-cross-origin.html failure could be > > related too? :( > > > > 10:24:51.154 21463 worker/1 > > http/tests/security/mixedContent/insecure-iframe-in-main-frame.html crashed, > > (stderr lines): > > 10:24:51.154 21463 > > > [3637:3637:1220/102450.802932:6083714579:WARNING:render_frame_host_impl.cc(2116)] > > OnDidStopLoading was called twice. > > 10:24:51.154 21463 > > [3637:3637:1220/102450.818444:6083730092:WARNING:web_contents_impl.cc(3515)] > > https://127.0.0.1:8443/ ran insecure content from > > http://example.test:8080/security/mixedContent/resources/boring.html > > 10:24:51.154 21463 > > [3637:3637:1220/102450.843980:6083755629:WARNING:web_contents_impl.cc(3515)] > > https://127.0.0.1:8443/ ran insecure content from > > http://example.test:8080/security/mixedContent/resources/boring.html > > 10:24:51.154 21463 > > > [3637:3637:1220/102450.848620:6083760263:WARNING:render_frame_host_impl.cc(2116)] > > OnDidStopLoading was called twice. > > The iframe-cross-origin.html failure is not related with this patch. I discussed > with nasko and we > think that setting the is_loading_ flag to true when a navigation request is > received in the browser > should do the trick here. Added that change in the patch and trying it on the > bots. This is weird: when we receive a navigation request in the browser we create a NavigationRequest, which will call WC::DidStartLoading (even before we get the call to DidStartNavigation). We should not have to trick the WebContentsObserverSanityChecker. I'd like if we could find why this is happening, since I'm worried it may cause issues in other parts of the code (in particular on Android which had a weird way of treating is_loading last time I checked). A TODO is fine, but we should not let this special case in the WebContentsObserverSanityChecker long term.
On 2016/12/21 13:01:55, clamy wrote: > On 2016/12/21 00:25:48, ananta wrote: > > On 2016/12/20 22:37:00, scottmg (OOO soon) wrote: > > > The site-isolation webkit_tests > > > http/tests/intersection-observer/iframe-cross-origin.html failure could be > > > related too? :( > > > > > > 10:24:51.154 21463 worker/1 > > > http/tests/security/mixedContent/insecure-iframe-in-main-frame.html crashed, > > > (stderr lines): > > > 10:24:51.154 21463 > > > > > > [3637:3637:1220/102450.802932:6083714579:WARNING:render_frame_host_impl.cc(2116)] > > > OnDidStopLoading was called twice. > > > 10:24:51.154 21463 > > > [3637:3637:1220/102450.818444:6083730092:WARNING:web_contents_impl.cc(3515)] > > > https://127.0.0.1:8443/ ran insecure content from > > > http://example.test:8080/security/mixedContent/resources/boring.html > > > 10:24:51.154 21463 > > > [3637:3637:1220/102450.843980:6083755629:WARNING:web_contents_impl.cc(3515)] > > > https://127.0.0.1:8443/ ran insecure content from > > > http://example.test:8080/security/mixedContent/resources/boring.html > > > 10:24:51.154 21463 > > > > > > [3637:3637:1220/102450.848620:6083760263:WARNING:render_frame_host_impl.cc(2116)] > > > OnDidStopLoading was called twice. > > > > The iframe-cross-origin.html failure is not related with this patch. I > discussed > > with nasko and we > > think that setting the is_loading_ flag to true when a navigation request is > > received in the browser > > should do the trick here. Added that change in the patch and trying it on the > > bots. > > This is weird: when we receive a navigation request in the browser we create a > NavigationRequest, which will call WC::DidStartLoading (even before we get the > call to DidStartNavigation). We should not have to trick the > WebContentsObserverSanityChecker. I'd like if we could find why this is > happening, since I'm worried it may cause issues in other parts of the code (in > particular on Android which had a weird way of treating is_loading last time I > checked). A TODO is fine, but we should not let this special case in the > WebContentsObserverSanityChecker long term. I reverted the is_loading_ changes. The site isolation redness are not related to this trace. Will look into that in a separate patchset.
Description was changed from ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. scottmg's patch for reference is here https://codereview.chromium.org/2381503003. This was reverted due to failures on Android. The failure was seen in cases like this. Navigation to url1->url2->url1. If the first navigation is not committed yet, then we would treat the last navigation to url1 as a reload. This causes inbox on Android to hang. The proposed fix is to use the last pending navigation if it exists or the last committed navigation to compare whether the current navigation is a reload. This fixes the issues with Android Changes in this patch are as below: 1. Add members in the NavigationControllerImpl class to hold the last pending navigation entry, its index, and the last transient index. The last transient index is used to avoid treating navigations as reloads if we have an interstitial being displayed. 2. Add a flag in the NavigationEntryImpl class to indicate whether there was a SSL error in the navigation. This was added to fix browser test failures. We avoid treating navigations as reloads when the last pending navigation has a SSL error. 3. We compare the url of the last pending or committed entry with the current navigation entry to see if it is a reload. There are a number of cases to consider here. Please look at the code. 4. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. 5. Set is_loading_ to true in RenderFrameHostImpl::OnBeginNavigation. This should fix the redness with blink site-per-process layout tests with plznavigate. Unrelated with this patch. The associated bug is 676235 The non plznavigate case does not have the bug when the EnsureSamePageNavigationUpdatesFrameNavigationEntry test because Blink tells us the correct navigation history, i.e. whether a navigation is a reload, etc. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319, 676235 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. scottmg's patch for reference is here https://codereview.chromium.org/2381503003. This was reverted due to failures on Android. The failure was seen in cases like this. Navigation to url1->url2->url1. If the first navigation is not committed yet, then we would treat the last navigation to url1 as a reload. This causes inbox on Android to hang. The proposed fix is to use the last pending navigation if it exists or the last committed navigation to compare whether the current navigation is a reload. This fixes the issues with Android Changes in this patch are as below: 1. Add members in the NavigationControllerImpl class to hold the last pending navigation entry, its index, and the last transient index. The last transient index is used to avoid treating navigations as reloads if we have an interstitial being displayed. 2. Add a flag in the NavigationEntryImpl class to indicate whether there was a SSL error in the navigation. This was added to fix browser test failures. We avoid treating navigations as reloads when the last pending navigation has a SSL error. 3. We compare the url of the last pending or committed entry with the current navigation entry to see if it is a reload. There are a number of cases to consider here. Please look at the code. 4. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. The non plznavigate case does not have the bug when the EnsureSamePageNavigationUpdatesFrameNavigationEntry test because Blink tells us the correct navigation history, i.e. whether a navigation is a reload, etc. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319, 676235 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ananta@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by ananta@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Lgtm.
The CQ bit was checked by ananta@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/2580753002/#ps400001 (title: "Revert changes to browser-side-navigation.linux.content_browsertests.filter. Will do that in a sepa…")
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": 400001, "attempt_start_ts": 1482426373478150,
"parent_rev": "340c99164a372f13f3a1a04a4a7cdbd9b21341de", "commit_rev":
"c285b99b5f81fe2ca616b0c830d16e9bee952d5a"}
Message was sent while issue was closed.
Description was changed from ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. scottmg's patch for reference is here https://codereview.chromium.org/2381503003. This was reverted due to failures on Android. The failure was seen in cases like this. Navigation to url1->url2->url1. If the first navigation is not committed yet, then we would treat the last navigation to url1 as a reload. This causes inbox on Android to hang. The proposed fix is to use the last pending navigation if it exists or the last committed navigation to compare whether the current navigation is a reload. This fixes the issues with Android Changes in this patch are as below: 1. Add members in the NavigationControllerImpl class to hold the last pending navigation entry, its index, and the last transient index. The last transient index is used to avoid treating navigations as reloads if we have an interstitial being displayed. 2. Add a flag in the NavigationEntryImpl class to indicate whether there was a SSL error in the navigation. This was added to fix browser test failures. We avoid treating navigations as reloads when the last pending navigation has a SSL error. 3. We compare the url of the last pending or committed entry with the current navigation entry to see if it is a reload. There are a number of cases to consider here. Please look at the code. 4. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. The non plznavigate case does not have the bug when the EnsureSamePageNavigationUpdatesFrameNavigationEntry test because Blink tells us the correct navigation history, i.e. whether a navigation is a reload, etc. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319, 676235 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. scottmg's patch for reference is here https://codereview.chromium.org/2381503003. This was reverted due to failures on Android. The failure was seen in cases like this. Navigation to url1->url2->url1. If the first navigation is not committed yet, then we would treat the last navigation to url1 as a reload. This causes inbox on Android to hang. The proposed fix is to use the last pending navigation if it exists or the last committed navigation to compare whether the current navigation is a reload. This fixes the issues with Android Changes in this patch are as below: 1. Add members in the NavigationControllerImpl class to hold the last pending navigation entry, its index, and the last transient index. The last transient index is used to avoid treating navigations as reloads if we have an interstitial being displayed. 2. Add a flag in the NavigationEntryImpl class to indicate whether there was a SSL error in the navigation. This was added to fix browser test failures. We avoid treating navigations as reloads when the last pending navigation has a SSL error. 3. We compare the url of the last pending or committed entry with the current navigation entry to see if it is a reload. There are a number of cases to consider here. Please look at the code. 4. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. The non plznavigate case does not have the bug when the EnsureSamePageNavigationUpdatesFrameNavigationEntry test because Blink tells us the correct navigation history, i.e. whether a navigation is a reload, etc. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319, 676235 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2580753002 ==========
Message was sent while issue was closed.
Committed patchset #20 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. scottmg's patch for reference is here https://codereview.chromium.org/2381503003. This was reverted due to failures on Android. The failure was seen in cases like this. Navigation to url1->url2->url1. If the first navigation is not committed yet, then we would treat the last navigation to url1 as a reload. This causes inbox on Android to hang. The proposed fix is to use the last pending navigation if it exists or the last committed navigation to compare whether the current navigation is a reload. This fixes the issues with Android Changes in this patch are as below: 1. Add members in the NavigationControllerImpl class to hold the last pending navigation entry, its index, and the last transient index. The last transient index is used to avoid treating navigations as reloads if we have an interstitial being displayed. 2. Add a flag in the NavigationEntryImpl class to indicate whether there was a SSL error in the navigation. This was added to fix browser test failures. We avoid treating navigations as reloads when the last pending navigation has a SSL error. 3. We compare the url of the last pending or committed entry with the current navigation entry to see if it is a reload. There are a number of cases to consider here. Please look at the code. 4. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. The non plznavigate case does not have the bug when the EnsureSamePageNavigationUpdatesFrameNavigationEntry test because Blink tells us the correct navigation history, i.e. whether a navigation is a reload, etc. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319, 676235 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2580753002 ========== to ========== Ensure that duplicate navigations are tagged as reload in the browser. This includes the cases where we are navigating to a URL which was committed previously or to a URL for which we haven't received a commit. scottmg's patch for reference is here https://codereview.chromium.org/2381503003. This was reverted due to failures on Android. The failure was seen in cases like this. Navigation to url1->url2->url1. If the first navigation is not committed yet, then we would treat the last navigation to url1 as a reload. This causes inbox on Android to hang. The proposed fix is to use the last pending navigation if it exists or the last committed navigation to compare whether the current navigation is a reload. This fixes the issues with Android Changes in this patch are as below: 1. Add members in the NavigationControllerImpl class to hold the last pending navigation entry, its index, and the last transient index. The last transient index is used to avoid treating navigations as reloads if we have an interstitial being displayed. 2. Add a flag in the NavigationEntryImpl class to indicate whether there was a SSL error in the navigation. This was added to fix browser test failures. We avoid treating navigations as reloads when the last pending navigation has a SSL error. 3. We compare the url of the last pending or committed entry with the current navigation entry to see if it is a reload. There are a number of cases to consider here. Please look at the code. 4. Added a unittest NavigationControllerTest.MultipleNavigationsAndReload which tests various combinations of pending and committed navigations to ensure that the navigation is tagged correctly as reload or not. The non plznavigate case does not have the bug when the EnsureSamePageNavigationUpdatesFrameNavigationEntry test because Blink tells us the correct navigation history, i.e. whether a navigation is a reload, etc. R=nasko@chromium.org,clamy@chromium.org BUG=630103, 475027, 536102, 664319, 676235 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/3bdd8aea0439f2c6a50d24c3c805bc7e579d5a2c Cr-Commit-Position: refs/heads/master@{#440443} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/3bdd8aea0439f2c6a50d24c3c805bc7e579d5a2c Cr-Commit-Position: refs/heads/master@{#440443} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
