Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1048963002:
Fix incorrect creation of duplicate navigation entries for repeated page load failures. (Closed)
|
Created:
5 years, 8 months ago by Avi (use Gerrit) Modified:
5 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@committype3 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix incorrect creation of duplicate navigation entries for repeated page load failures.
BUG=471292
TEST=the new test, NavigationControllerBrowserTest.ErrorPageReplacement
Committed: https://crrev.com/45a7253bdc2863a5e8193e9cfb20a28e2dfb816b
Cr-Commit-Position: refs/heads/master@{#324116}
Patch Set 1 #
Total comments: 4
Patch Set 2 : more test #
Total comments: 2
Patch Set 3 : works #
Total comments: 17
Patch Set 4 : rev #Patch Set 5 : oopsie #Patch Set 6 : bettar #
Total comments: 2
Patch Set 7 : with more comment #
Messages
Total messages: 30 (4 generated)
avi@chromium.org changed reviewers: + creis@chromium.org
This requires the previous CLs that route commit_type into didFail. To explain why I chose EXISTING_PAGE rather than SAME_PAGE: 1. They are indistinguishable inside ClassifyNavigation. The only difference between a reload and a tap on the enter key that is turned into a reload is the pending entry. Because the pending entry is cleared on a load failure, there isn't anything to distinguish the cases. 2. The difference between NavigationControllerImpl::RendererDidNavigateToExistingPage and NavigationControllerImpl::RendererDidNavigateToSamePage is that RendererDidNavigateToSamePage cleans up the pending entry. Since that's exactly what we _don't_ have, classifying SAME_PAGE would cause issues. In this case, we need to treat this as an existing page navigation.
avi@chromium.org changed reviewers: + davidben@chromium.org
Also, David, if you can take a look. The previous series of three commits: https://codereview.chromium.org/1033233003 https://codereview.chromium.org/1046953003 https://codereview.chromium.org/1044923003 plumbed the commit type into didFail, so now we can rely on what Blink _would_ have done rather than try to infer it from the page transition type (which isn't always populated) and from the page id (which is going away).
Looks good, pending the other CLs landing and one more test. https://codereview.chromium.org/1048963002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1048963002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:407: // page, the EXISTING_PAGE navigation type, and no addition to the history Let's put a bit of your explanation in this comment. Something like: We do not use SAME_PAGE here; that case only differs in that it clears the pending entry, and there is no pending entry after a load failure. https://codereview.chromium.org/1048963002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:418: } Can you add a test for location.replace to error_url (after a navigation to a real URL)? That would cover the TODO(davidben) comment we're removing.
https://codereview.chromium.org/1048963002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1048963002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:407: // page, the EXISTING_PAGE navigation type, and no addition to the history On 2015/03/30 21:36:54, Charlie Reis wrote: > Let's put a bit of your explanation in this comment. Something like: > We do not use SAME_PAGE here; that case only differs in that it clears the > pending entry, and there is no pending entry after a load failure. Done. https://codereview.chromium.org/1048963002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:418: } On 2015/03/30 21:36:54, Charlie Reis wrote: > Can you add a test for location.replace to error_url (after a navigation to a > real URL)? That would cover the TODO(davidben) comment we're removing. Done.
Thanks! LGTM.
[already chatted about this out-of-band but adding here for posterity] https://codereview.chromium.org/1048963002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1048963002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:434: } There's a third case that's probably worth testing: if the NavigateToURLAndReplace which failed was a cross-process navigation, I recall things get really weird. Blink and the browser process disagree on the replace bit in that case.
https://codereview.chromium.org/1048963002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1048963002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:434: } On 2015/03/31 17:26:08, David Benjamin wrote: > There's a third case that's probably worth testing: if the > NavigateToURLAndReplace which failed was a cross-process navigation, I recall > things get really weird. Blink and the browser process disagree on the replace > bit in that case. It looks like that's what NavigateToURLAndReplace essentially does, by setting params.should_replace_current_entry. (Right?) I agree that actually doing a location.replace in the renderer process would behave differently, so maybe it's worth testing both.
On 2015/03/31 17:29:48, Charlie Reis wrote: > https://codereview.chromium.org/1048963002/diff/20001/content/browser/frame_h... > File content/browser/frame_host/navigation_controller_impl_browsertest.cc > (right): > > https://codereview.chromium.org/1048963002/diff/20001/content/browser/frame_h... > content/browser/frame_host/navigation_controller_impl_browsertest.cc:434: } > On 2015/03/31 17:26:08, David Benjamin wrote: > > There's a third case that's probably worth testing: if the > > NavigateToURLAndReplace which failed was a cross-process navigation, I recall > > things get really weird. Blink and the browser process disagree on the replace > > bit in that case. > > It looks like that's what NavigateToURLAndReplace essentially does, by setting > params.should_replace_current_entry. (Right?) I agree that actually doing a > location.replace in the renderer process would behave differently, so maybe it's > worth testing both. If I do a NavigateToURLAndReplace atop a WebUI page (to force a process swap), things time out. I am looking at that failure now.
On 2015/03/31 17:33:14, Avi wrote: > On 2015/03/31 17:29:48, Charlie Reis wrote: > > > https://codereview.chromium.org/1048963002/diff/20001/content/browser/frame_h... > > File content/browser/frame_host/navigation_controller_impl_browsertest.cc > > (right): > > > > > https://codereview.chromium.org/1048963002/diff/20001/content/browser/frame_h... > > content/browser/frame_host/navigation_controller_impl_browsertest.cc:434: } > > On 2015/03/31 17:26:08, David Benjamin wrote: > > > There's a third case that's probably worth testing: if the > > > NavigateToURLAndReplace which failed was a cross-process navigation, I > recall > > > things get really weird. Blink and the browser process disagree on the > replace > > > bit in that case. > > > > It looks like that's what NavigateToURLAndReplace essentially does, by setting > > > params.should_replace_current_entry. (Right?) I agree that actually doing a > > location.replace in the renderer process would behave differently, so maybe > it's > > worth testing both. > > If I do a NavigateToURLAndReplace atop a WebUI page (to force a process swap), > things time out. I am looking at that failure now. Specifically the distinction is that example.com -> about:blank -> example.com doesn't seem to switch processes (checked this in my running Chrome). But if you do a browser-initiated load from, say, example.com -replace-> davidben.net and then davidben.net gets an error page, it's a little weird. davidben.net's renderer thinks this is the first load in a WebFrame. I think that's the case when you get the commit type and should_replace_current_entry disagreeing? It's been a while and I could be misremembering. (Plus I'm sure some of it's changed.)
Charlie, David, please take another look.
https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:230: controller_->DiscardPendingEntry(true); How does this work in the OOPIF world? It seems strange to me that something about replace bit (a frame-level notion) is handled on the controller (a tab-level notion). Or does location.replace not really work in OOPIF yet and that's for future?
https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:230: controller_->DiscardPendingEntry(true); On 2015/04/03 19:44:11, David Benjamin wrote: > How does this work in the OOPIF world? It seems strange to me that something > about replace bit (a frame-level notion) is handled on the controller (a > tab-level notion). Or does location.replace not really work in OOPIF yet and > that's for future? I don't follow. How does location.replace figure in here? This "replace" bit isn't location.replace, it's "we're doing a cross-process navigation, so even though the new process is going to say that it makes a new nav item, replace the current one instead".
https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:230: controller_->DiscardPendingEntry(true); On 2015/04/03 19:51:47, Avi (OOO Friday 3 Apr) wrote: > On 2015/04/03 19:44:11, David Benjamin wrote: > > How does this work in the OOPIF world? It seems strange to me that something > > about replace bit (a frame-level notion) is handled on the controller (a > > tab-level notion). Or does location.replace not really work in OOPIF yet and > > that's for future? > > I don't follow. How does location.replace figure in here? This "replace" bit > isn't location.replace, it's "we're doing a cross-process navigation, so even > though the new process is going to say that it makes a new nav item, replace the > current one instead". I possibly stapled this to the wrong comment. NavigationController is shared between all frames under one tab, right? So how do we make sure error pages work if a cross-process location.replace in an iframe happens and that one fails. It too should location.replace into the frame history entry. Actually, as I'm typing this, it seems should_replace_entry is only on NavigationEntry anyway. So we don't actually have browser-side frame history entry at all yet... which I suppose is what your end goal is anyway? You can probably disregard this comment. I'm guessing this is to be sorted out later and/or I'm slightly confused?
https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:230: controller_->DiscardPendingEntry(true); On 2015/04/03 20:23:44, David Benjamin wrote: > Actually, as I'm typing this, it seems should_replace_entry is only on > NavigationEntry anyway. So we don't actually have browser-side frame history > entry at all yet... which I suppose is what your end goal is anyway? You can > probably disregard this comment. I'm guessing this is to be sorted out later > and/or I'm slightly confused? I'm pretty confused as well, so don't worry. Right now, should_replace is on an entire NavEntry, so by definition it only applies to entire pages. I don't know what its future is for oopifs. This CL shouldn't change where we are in terms of the oopif implementation. I'm pretty sure this flag doesn't work today at the frame level, and this CL maintains the status quo.
lgtm (Of course, you'll want to have Charlie look at the new version too.)
https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:230: controller_->DiscardPendingEntry(true); On 2015/04/03 19:51:47, Avi (OOO Friday 3 Apr) wrote: > On 2015/04/03 19:44:11, David Benjamin wrote: > > How does this work in the OOPIF world? It seems strange to me that something > > about replace bit (a frame-level notion) is handled on the controller (a > > tab-level notion). Or does location.replace not really work in OOPIF yet and > > that's for future? > > I don't follow. How does location.replace figure in here? This "replace" bit > isn't location.replace, it's "we're doing a cross-process navigation, so even > though the new process is going to say that it makes a new nav item, replace the > current one instead". David's point is valid, though I'm not sure location.replace is the right example. The replace bit is a frame-specific notion. It can happen on things like cross-process redirects or failed same-page navigations, which can happen in any frame. In contrast, pending entries (and the new failed pending entry) live on the NavigationController for the whole tab. If multiple frames are navigating at the same time, they'll clobber each other's entries. This is one of a few reasons I've been trying to avoid depending on pending entries to store the state between a navigation's start and its commit. There's a lot of places where that happens today and it's not easy to kill them all. I have some hope for using PlzNavigate's NavigationRequest, but I don't think that will be ready soon. For this particular bug, we're trying to preserve some info (which had lived on the pending entry) from the DidFail to the corresponding DidCommit of the error page. This is an awkward problem for a bunch of reasons: 1) NavigationController's classification logic is already relying on the pending entry for the replace bit, which is unfortunate for the reasons above. 2) There's currently no link between the DidFail and the DidCommit of the error page. I'm not even sure how we would make such a link (e.g., does the renderer know about the NavigationRequest even in PlzNavigate?). 3) I don't know if we can tell whether there even will be a DidCommit of an error page at the time of DidFail. Some DidFails (e.g., Stop, 204, downloads) don't create an error page. That means any state we keep around after DidFail might stick around "forever" (or until the next commit from that frame). This seems like a fairly fundamental flaw with how error pages are handled. So far, Avi's patch is dealing with #2, punting on #3 (by leaving the failed entry around if there's no error page), and not yet addressing #1. Ideally, we'd find a way that improves all of these. But this is also blocking the rest of the OOPIF session history work at the moment, so we may want to consider it if it doesn't make things much worse. It'd be nice to at least have a plan for how to get rid of the failed entry notion, though.
On 2015/04/03 20:38:21, Charlie Reis wrote: > So far, Avi's patch is dealing with #2, punting on #3 (by leaving the failed entry > around if there's no error page), and not yet addressing #1. Indeed. This is only a slight step forward from where we are now, but it unlocks progress on page id and doesn't make things worse. Charlie, you said you were still pondering holding onto the pending entry like this vs holding onto two bits of info. Let me know if you're happy with this version or if you want me to fiddle with it more.
On 2015/04/03 20:45:15, Avi (OOO Friday 3 Apr) wrote: > On 2015/04/03 20:38:21, Charlie Reis wrote: > > So far, Avi's patch is dealing with #2, punting on #3 (by leaving the failed > entry > > around if there's no error page), and not yet addressing #1. > > Indeed. This is only a slight step forward from where we are now, but it unlocks > progress on page id and doesn't make things worse. > > Charlie, you said you were still pondering holding onto the pending entry like > this vs holding onto two bits of info. Let me know if you're happy with this > version or if you want me to fiddle with it more. Yep, thoughts on that below. Thanks! https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:801: if (params.url_is_unreachable) { Should this be params.url_is_unreachable && failed_pending_entry_.get()? See my comment in navigator_impl.cc for why there might be a bug here. https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1786: failed_pending_entry_.reset(); You've confirmed this gets cleared after the error page commits, right? (I think we should get here via the DidCommit of the error page.) https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.h (right): https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.h:350: // page is shown. This needs to mention that it will stick around until the next commit if there is no error page (e.g., Stop, 204, downloads). We should also have a TODO with bug # to remove this and find a different way to match up DidFail to the corresponding error page's DidCommit, because this approach can't handle navigations in multiple frames. https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.h:351: scoped_ptr<NavigationEntryImpl> failed_pending_entry_; How bad would it be to store just the things we need from the failed entry? The entry is pretty big and (as noted above) it may stick around too long in a lot of cases. I'm concerned about things that the entry might be keeping alive, like site_instance_, source_site_instance_, and browser_initiated_post_data_. (This may also make it easier to see what needs to stick around on a per-frame basis before DidFail and a potential error page DidCommit.) https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:229: !should_preserve_entry) { Hmm, the fact that this DiscardPendingEntry call is conditional bothers me. That means there are some ways for us to not have a failed_pending_entry_ even though an error page may be coming. In fact, give this a try: click a target=_blank link to a URL that will fail with an error page. That's an unmodified blank tab, so we leave the pending entry around so the user can see what URL failed. (This is helpful if there is no error page, such as a 204 or if the user clicks Stop.) However, that means we won't have a failed entry in RendererDidNavigate when the error page commits. params.url_is_unreachable will be true, though, so we won't try to copy the replace bit from the pending entry. (I guess replace would be false in this particular example, but you can see how this might be buggy.)
https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:801: if (params.url_is_unreachable) { On 2015/04/03 20:59:46, Charlie Reis wrote: > Should this be params.url_is_unreachable && failed_pending_entry_.get()? See my > comment in navigator_impl.cc for why there might be a bug here. I'm trying to come up with a scenario in which there's a difference. Your comment in NavigatorImpl clearly explains that the whole setup here is bad, but doesn't show a specific scenario. I'm imagining two frames loading as part of the same navigation. If frame 1 errors, then it sets the failure variables, and doesn't load an error page. Frame 2 commits, and it isn't an error load so it hits the else here. There won't be a pending entry, so we get false overall. Maybe that second frame commit should get the should_replace value from the (now dead) pending entry, but the if() expression would need to be an || not &&. However, we can't do that because of the possibility, like below, of the error values floating, in which case the || would cause intermixing with completely unrelated future loads. If frame 1 succeeds but frame 2 fails, then it doesn't matter as there won't be a subsequent error page load. If both frames fail, then both are url_is_unreachable, and it works as-is. Can you clarify the scenario in which this code makes things worse? https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1786: failed_pending_entry_.reset(); On 2015/04/03 20:59:46, Charlie Reis wrote: > You've confirmed this gets cleared after the error page commits, right? (I > think we should get here via the DidCommit of the error page.) Yes, this gets cleared by the DiscardNonCommittedEntriesInternal in DidNavigate for the error page commit. OTOH, you are right, we have no guarantees that there will be an error page. If you look at Camille's patch, she is deliberately loading an error page from the browser process on failure, rather than relying on the renderer process unilaterally doing that without telling the browser process. I see a variation of that approach as being the sanest way forward here in a subsequent CL. https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.h (right): https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.h:350: // page is shown. On 2015/04/03 20:59:46, Charlie Reis wrote: > This needs to mention that it will stick around until the next commit if there > is no error page (e.g., Stop, 204, downloads). > > We should also have a TODO with bug # to remove this and find a different way to > match up DidFail to the corresponding error page's DidCommit, because this > approach can't handle navigations in multiple frames. Done. https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.h:351: scoped_ptr<NavigationEntryImpl> failed_pending_entry_; On 2015/04/03 20:59:46, Charlie Reis wrote: > How bad would it be to store just the things we need from the failed entry? The > entry is pretty big and (as noted above) it may stick around too long in a lot > of cases. I'm concerned about things that the entry might be keeping alive, > like site_instance_, source_site_instance_, and browser_initiated_post_data_. > > (This may also make it easier to see what needs to stick around on a per-frame > basis before DidFail and a potential error page DidCommit.) Done. https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:230: controller_->DiscardPendingEntry(true); On 2015/04/03 20:38:21, Charlie Reis wrote: > But this is also blocking > the rest of the OOPIF session history work at the moment, so we may want to > consider it if it doesn't make things much worse. It'd be nice to at least have > a plan for how to get rid of the failed entry notion, though. If you want me to come back to this after I kill page id, I can. But right now, given that page id is blocking both you and Camille, I'd prefer to get this in as a stop-gap.
Thanks-- that's looking better. We should still fix NavigationControllerImpl::RendererDidNavigate, and hopefully I have a test case for it below. https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:801: if (params.url_is_unreachable) { On 2015/04/06 19:38:16, Avi wrote: > On 2015/04/03 20:59:46, Charlie Reis wrote: > > Should this be params.url_is_unreachable && failed_pending_entry_.get()? See > my > > comment in navigator_impl.cc for why there might be a bug here. > > I'm trying to come up with a scenario in which there's a difference. Your > comment in NavigatorImpl clearly explains that the whole setup here is bad, but > doesn't show a specific scenario. > > I'm imagining two frames loading as part of the same navigation. > > If frame 1 errors, then it sets the failure variables, and doesn't load an error > page. Frame 2 commits, and it isn't an error load so it hits the else here. > There won't be a pending entry, so we get false overall. Maybe that second frame > commit should get the should_replace value from the (now dead) pending entry, > but the if() expression would need to be an || not &&. However, we can't do that > because of the possibility, like below, of the error values floating, in which > case the || would cause intermixing with completely unrelated future loads. > > If frame 1 succeeds but frame 2 fails, then it doesn't matter as there won't be > a subsequent error page load. > > If both frames fail, then both are url_is_unreachable, and it works as-is. > > Can you clarify the scenario in which this code makes things worse? The multiple frames thing is a different issue, and one that we can't resolve in this if statement. For example: 1) Frame 1 fails and sets failed_pending_entry_should_replace_ to false. 2) Frame 2 fails and sets failed_pending_entry_should_replace_ to true. 3) Frame 1 commits its error page. We get here and set did_replace_entry to true (incorrectly). The bug I was talking about is that the pending entry might not have been discarded when failing. We need a case where should_preserve_entry is false but should_replace_entry() is true for this bug to become visible. I think that's possible like this: 1) Start on the NTP. 2) Navigate to a test server page, then another page. 3) Go back to the NTP. 4) Clear your cache and shut down the test server. 5) Go forward. The forward operation will fail and have should_replace_entry() == true, right? But should_preserve_entry will be false because Browser::ShouldPreserveAbortedURLs sees the last committed page is the NTP. Thus, when the commit comes in, we'll still have the pending entry and no failed_pending_entry_*, meaning that we won't replace the desired entry with the error message. That means we'll probably clear the forward history when we shouldn't. (Note: it's just luck that I found what might be a test case here. If this doesn't work or there isn't a test case, we should still fix this bug.)
https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1048963002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:801: if (params.url_is_unreachable) { On 2015/04/06 20:48:10, Charlie Reis wrote: > On 2015/04/06 19:38:16, Avi wrote: > > On 2015/04/03 20:59:46, Charlie Reis wrote: > > > Should this be params.url_is_unreachable && failed_pending_entry_.get()? > See > > my > > > comment in navigator_impl.cc for why there might be a bug here. > > > > I'm trying to come up with a scenario in which there's a difference. Your > > comment in NavigatorImpl clearly explains that the whole setup here is bad, > but > > doesn't show a specific scenario. > > > > I'm imagining two frames loading as part of the same navigation. > > > > If frame 1 errors, then it sets the failure variables, and doesn't load an > error > > page. Frame 2 commits, and it isn't an error load so it hits the else here. > > There won't be a pending entry, so we get false overall. Maybe that second > frame > > commit should get the should_replace value from the (now dead) pending entry, > > but the if() expression would need to be an || not &&. However, we can't do > that > > because of the possibility, like below, of the error values floating, in which > > case the || would cause intermixing with completely unrelated future loads. > > > > If frame 1 succeeds but frame 2 fails, then it doesn't matter as there won't > be > > a subsequent error page load. > > > > If both frames fail, then both are url_is_unreachable, and it works as-is. > > > > Can you clarify the scenario in which this code makes things worse? > > The multiple frames thing is a different issue, and one that we can't resolve in > this if statement. For example: > 1) Frame 1 fails and sets failed_pending_entry_should_replace_ to false. > 2) Frame 2 fails and sets failed_pending_entry_should_replace_ to true. > 3) Frame 1 commits its error page. We get here and set did_replace_entry to > true (incorrectly). > > The bug I was talking about is that the pending entry might not have been > discarded when failing. We need a case where should_preserve_entry is false but > should_replace_entry() is true for this bug to become visible. I think that's > possible like this: > 1) Start on the NTP. > 2) Navigate to a test server page, then another page. > 3) Go back to the NTP. > 4) Clear your cache and shut down the test server. > 5) Go forward. > > The forward operation will fail and have should_replace_entry() == true, right? > But should_preserve_entry will be false because > Browser::ShouldPreserveAbortedURLs sees the last committed page is the NTP. > > Thus, when the commit comes in, we'll still have the pending entry and no > failed_pending_entry_*, meaning that we won't replace the desired entry with the > error message. That means we'll probably clear the forward history when we > shouldn't. > > (Note: it's just luck that I found what might be a test case here. If this > doesn't work or there isn't a test case, we should still fix this bug.) Yeah, I got it. The pending entry isn't guaranteed to be cleared on error, so if there is an error, we still might need to check the pending entry.
Thanks, LGTM. https://codereview.chromium.org/1048963002/diff/90007/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1048963002/diff/90007/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:802: // doing a cross-site redirect navigation, we will do a similar thing. Might also be worth mentioning the error page case, and that we need to remember whether the error page should replace the current entry from DidFail to DidCommit.
The CQ bit was checked by avi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1048963002/#ps110001 (title: "with more comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1048963002/110001
https://codereview.chromium.org/1048963002/diff/90007/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1048963002/diff/90007/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:802: // doing a cross-site redirect navigation, we will do a similar thing. On 2015/04/07 18:00:47, Charlie Reis wrote: > Might also be worth mentioning the error page case, and that we need to remember > whether the error page should replace the current entry from DidFail to > DidCommit. Done.
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/45a7253bdc2863a5e8193e9cfb20a28e2dfb816b Cr-Commit-Position: refs/heads/master@{#324116} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
