Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1825523002:
Do not reset navigation state when BeforeUnload is cancelled by a commit (Closed)
|
Created:
4 years, 9 months ago by clamy Modified:
4 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, Peter Beverloo, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-shell_chromium.org, jochen+watch_chromium.org, carlosk Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not reset navigation state when BeforeUnload is cancelled by a commit
It is possible for a cross-site navigation to commit while a BeforeUnload
confirmation dialog is showing. This will destroy dialog and call
WebContentsImpl::OnDialogClosed. This function should not reset the navigatin
state if the RenderFrameHost that was showing the dialog is no longer the
current RenderFrameHost.
BUG=589365
Committed: https://crrev.com/bc6f4150a3eef02b926611404e7cdd44c9d67635
Cr-Commit-Position: refs/heads/master@{#384674}
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : Rebase #Patch Set 4 : Addressed comments #
Total comments: 32
Patch Set 5 : Rebase #Patch Set 6 : Addressed comments #
Total comments: 7
Patch Set 7 : Rebase + addressed nit #Messages
Total messages: 36 (14 generated)
Description was changed from ========== Do not reset navigation state when BeforeUnload is cancelled by a commit It is possible for a cross-site navigation to commit while a BeforeUnload confirmation dialog is showing. This will destroy dialog and call WebContentsImpl::OnDialogClosed. This function should not reset the navigatin state if the RenderFrameHost that was showing the dialog is no longer the current RenderFrameHost. BUG=589365 ========== to ========== Do not reset navigation state when BeforeUnload is cancelled by a commit It is possible for a cross-site navigation to commit while a BeforeUnload confirmation dialog is showing. This will destroy dialog and call WebContentsImpl::OnDialogClosed. This function should not reset the navigatin state if the RenderFrameHost that was showing the dialog is no longer the current RenderFrameHost. BUG=589365 ==========
clamy@chromium.org changed reviewers: + nasko@chromium.org
@nasko: PTAL. This is the fix for the BeforeUnload dialog issue that is causing the UAF bug in NavigationHandle. The browser test does confirm that we show the dialog twice if we do a cross-site navigation followed by a renderer-initiated navigation. https://codereview.chromium.org/1825523002/diff/20001/content/test/content_br... File content/test/content_browser_test_utils_internal.h (right): https://codereview.chromium.org/1825523002/diff/20001/content/test/content_br... content/test/content_browser_test_utils_internal.h:116: class NavigationDelayer : public WebContentsObserver { Since I need to pause a navigation and then have it commit, I had to introduce this class based on the NavigationHandle. The NavigationStallDelegate would not work there. https://codereview.chromium.org/1825523002/diff/20001/content/test/data/hang_... File content/test/data/hang_before_unload.html (right): https://codereview.chromium.org/1825523002/diff/20001/content/test/data/hang_... content/test/data/hang_before_unload.html:15: window.setTimeout(clickLink, 100); The browser will hang in BeforeUnload, so I can't have the navigation start in the function that's executed via ExecuteScript. This is why I'm putting the click on a timer.
nasko@chromium.org changed reviewers: + creis@chromium.org
Hey Charlie, Could you help review this one? I won't have the bandwidth to review it in the depth it deserves in the next couple of days. Thanks! Nasko
Haven't looked closely at the test yet, but I have a concern about the fix. https://codereview.chromium.org/1825523002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1825523002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4695: if (is_showing_before_unload_dialog_ && !success && rfh == GetMainFrame()) { This won't work for subframes. (I know we have some bugs about showing dialogs from subframes, but this will need to work when we fix them.) Can we compare the NavigationHandle's nav_entry_id to the pending entry ID instead?
https://codereview.chromium.org/1825523002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1825523002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4695: if (is_showing_before_unload_dialog_ && !success && rfh == GetMainFrame()) { On 2016/03/24 21:50:41, Charlie Reis wrote: > This won't work for subframes. (I know we have some bugs about showing dialogs > from subframes, but this will need to work when we fix them.) > > Can we compare the NavigationHandle's nav_entry_id to the pending entry ID > instead? I don't quite see which NavigationHandle I should be comparing to the NavigationEntry and what I would get from that. In the problematic case (ie the commit of the pending rfh), rfh would have no NavigationHandle but the pending RFH (which is now the current one) would have it. Even if I check that the IDs match, this gives me no information on whether I need to cancel the navigation or not. In other cases, the user could have canceled the dialog and the IDs would still match. For example, this exact same case but where the user closed the dialog instead of letting the pending navigation commit. I cannot check that the state of the RFH is the default one, since the crash I'm fixing happens in the middle of the swap out, the RFH has changed but its state has not been modified yet. I agree that this is problematic for subframes, but it seems to me that this entire code does not handle the subframe case. It can probably be changed to address them later.
Thanks. Another suggestion below. https://codereview.chromium.org/1825523002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1825523002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4695: if (is_showing_before_unload_dialog_ && !success && rfh == GetMainFrame()) { On 2016/03/25 13:43:48, clamy wrote: > On 2016/03/24 21:50:41, Charlie Reis wrote: > > This won't work for subframes. (I know we have some bugs about showing > dialogs > > from subframes, but this will need to work when we fix them.) > > > > Can we compare the NavigationHandle's nav_entry_id to the pending entry ID > > instead? > I don't quite see which NavigationHandle I should be comparing to the > NavigationEntry and what I would get from that. In the problematic case (ie the > commit of the pending rfh), rfh would have no NavigationHandle but the pending > RFH (which is now the current one) would have it. Even if I check that the IDs > match, this gives me no information on whether I need to cancel the navigation > or not. In other cases, the user could have canceled the dialog and the IDs > would still match. For example, this exact same case but where the user closed > the dialog instead of letting the pending navigation commit. > > I cannot check that the state of the RFH is the default one, since the crash I'm > fixing happens in the middle of the swap out, the RFH has changed but its state > has not been modified yet. Ok. I hadn't looked closely enough to see which RFHs would have the NavigationHandle in this case, and I was looking for a way to tell if the frame was associated with the navigation or not. Perhaps that's not the way. > I agree that this is problematic for subframes, but it seems to me that this > entire code does not handle the subframe case. It can probably be changed to > address them later. It should be a simple fix to make it work for subframes, and I'd rather not dig a deeper hole there. Couldn't we check if rfh == rfh->frame_tree_node()->current_frame_host(), rather than assuming it's the main frame? https://codereview.chromium.org/1825523002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4701: BeforeUnloadDialogCancelled()); Is it safe to skip this? Looks like it might confuse CoreTabHelper.
Thanks! https://codereview.chromium.org/1825523002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1825523002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4695: if (is_showing_before_unload_dialog_ && !success && rfh == GetMainFrame()) { On 2016/03/25 23:59:43, Charlie Reis wrote: > On 2016/03/25 13:43:48, clamy wrote: > > On 2016/03/24 21:50:41, Charlie Reis wrote: > > > This won't work for subframes. (I know we have some bugs about showing > > dialogs > > > from subframes, but this will need to work when we fix them.) > > > > > > Can we compare the NavigationHandle's nav_entry_id to the pending entry ID > > > instead? > > I don't quite see which NavigationHandle I should be comparing to the > > NavigationEntry and what I would get from that. In the problematic case (ie > the > > commit of the pending rfh), rfh would have no NavigationHandle but the pending > > RFH (which is now the current one) would have it. Even if I check that the IDs > > match, this gives me no information on whether I need to cancel the navigation > > or not. In other cases, the user could have canceled the dialog and the IDs > > would still match. For example, this exact same case but where the user closed > > the dialog instead of letting the pending navigation commit. > > > > I cannot check that the state of the RFH is the default one, since the crash > I'm > > fixing happens in the middle of the swap out, the RFH has changed but its > state > > has not been modified yet. > > Ok. I hadn't looked closely enough to see which RFHs would have the > NavigationHandle in this case, and I was looking for a way to tell if the frame > was associated with the navigation or not. Perhaps that's not the way. > > > I agree that this is problematic for subframes, but it seems to me that this > > entire code does not handle the subframe case. It can probably be changed to > > address them later. > > It should be a simple fix to make it work for subframes, and I'd rather not dig > a deeper hole there. Couldn't we check if rfh == > rfh->frame_tree_node()->current_frame_host(), rather than assuming it's the main > frame? Done. https://codereview.chromium.org/1825523002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4701: BeforeUnloadDialogCancelled()); On 2016/03/25 23:59:43, Charlie Reis wrote: > Is it safe to skip this? Looks like it might confuse CoreTabHelper. Since this could be problematic, I now only skip the reset of teh loading state and the pending entry. https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:819: #if defined(OS_WIN) || defined(OS_MACOSX) Apparently the test has some issues on Windows and MacOS. Since this a fix for a crash I'd like to land it ASAP, and fix the test for other platforms in a second patch.
Nasko, can you review the new test class? I'd like to get your input on it, but I think you'll like it. :) Mostly nits apart from that. I'll say LGTM, but I'm a bit worried about the test. Please only proceed if you're confident it's a platform-specific test issue and not a possible bug. https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:819: #if defined(OS_WIN) || defined(OS_MACOSX) On 2016/03/29 13:01:50, clamy wrote: > Apparently the test has some issues on Windows and MacOS. Since this a fix for a > crash I'd like to land it ASAP, and fix the test for other platforms in a second > patch. Hmm, this concerns me, because the crash affects those platforms and it's not clear to me why this would be platform specific. Is the test going to be flaky on Linux? If you're confident it's due to a test-only issue on Windows and Mac, then please elaborate in the comment a bit and I'll be ok with fixing it in a followup. https://codereview.chromium.org/1825523002/diff/60001/content/shell/browser/s... File content/shell/browser/shell_javascript_dialog_manager.h (right): https://codereview.chromium.org/1825523002/diff/60001/content/shell/browser/s... content/shell/browser/shell_javascript_dialog_manager.h:62: bool proceed_beforeunload_default_; nit: should_proceed_on_beforeunload_? (The current name is a bit hard to parse.) A comment here would also help. https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... File content/test/content_browser_test_utils_internal.h (right): https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... content/test/content_browser_test_utils_internal.h:116: class NavigationDelayer : public WebContentsObserver { Nasko, can you review this part? I know you've been wanting this for a while. (You have a TODO for it just above, on line 96.) https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... content/test/content_browser_test_utils_internal.h:125: // WaitForNavigationPaused. Do you also need to call ResumeNavigation before calling this? That's what your test is doing, and it's not clear here whether that's required. https://codereview.chromium.org/1825523002/diff/60001/content/test/data/hang_... File content/test/data/hang_before_unload.html (right): https://codereview.chromium.org/1825523002/diff/60001/content/test/data/hang_... content/test/data/hang_before_unload.html:3: <head><title>Possible BeforeUnload</title> Why "Possible"? https://codereview.chromium.org/1825523002/diff/60001/content/test/data/hang_... content/test/data/hang_before_unload.html:14: function clickLinkSoon() { nit: Use consistent indent. https://codereview.chromium.org/1825523002/diff/60001/content/test/data/hang_... content/test/data/hang_before_unload.html:19: window.addEventListener("beforeunload", function(e){ nit: Space before { https://codereview.chromium.org/1825523002/diff/60001/content/test/data/hang_... content/test/data/hang_before_unload.html:23: }); nit: 2 fewer spaces.
Super excited about utilizing the throttle in test code! Bunch more nits from me. https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4680: // It is possible for the RenderFrameHost to have been swapped out in the nit: Let's start avoiding using "swapped out" as terminology. Suggestion: "It is possible for the current RenderFrameHost to have changed in the meantime." https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:826: // This CL tests that if a BeforeUnload dialog is destroyed due to the commit nit: drop "CL". This is a test case, it just happens to be part of a CL, which once committed won't exist as a concept. https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:844: // Click on a link in the page. This will show the BeforeUnload dialog. Have nit: "Have it hang" doesn't convey why or what hangs? Something along the lines of "Ensure that dialog is not dismissed, which will cause ... to wait ...". https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... File content/test/content_browser_test_utils_internal.cc (right): https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... content/test/content_browser_test_utils_internal.cc:370: loop_runner_ = new MessageLoopRunner(); Why not create it in the constructor? https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... File content/test/content_browser_test_utils_internal.h (right): https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... content/test/content_browser_test_utils_internal.h:116: class NavigationDelayer : public WebContentsObserver { I'd call this something more generic, as it ideally will be able to block/drop/delay navigations. I had TestNavigationScheduler in my strawman CL a year ago, but needn't be that. https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... content/test/content_browser_test_utils_internal.h:118: NavigationDelayer(WebContents* web_contents, const GURL& url); Is the goal for this class to monitor any frame in WebContents? If so, let's put a comment clarifying this on this constructor. I imagine in the future we will want to monitor only a specific frame, especially since cross_site_iframe_factory.html generates URLs that differ only in the query string when they are for the same site. Alternatively, this might be a useful class to expose outside of content as it is. I haven't looked at what tests could use it though. https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... content/test/content_browser_test_utils_internal.h:122: void WaitForNavigationPaused(); At what point will navigation be paused? Let's use the naming of the method to make this clear. For example: WaitForCommit(), WaitForRedirect(), etc. The default behavior should probably be to defer the navigation at the point of interest based on the method that was called and have the testcase itself resume it once the intended checks have been completed. https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... content/test/content_browser_test_utils_internal.h:136: // Called when the NavigationThrottlePauses the navigation. nit: s/NavigationThrottlePauses/NavigationThrottle pauses/?
Thanks! https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4680: // It is possible for the RenderFrameHost to have been swapped out in the On 2016/03/29 20:19:06, nasko (slow) wrote: > nit: Let's start avoiding using "swapped out" as terminology. Suggestion: "It is > possible for the current RenderFrameHost to have changed in the meantime." Done. https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:819: #if defined(OS_WIN) || defined(OS_MACOSX) On 2016/03/29 18:33:21, Charlie Reis wrote: > On 2016/03/29 13:01:50, clamy wrote: > > Apparently the test has some issues on Windows and MacOS. Since this a fix for > a > > crash I'd like to land it ASAP, and fix the test for other platforms in a > second > > patch. > > Hmm, this concerns me, because the crash affects those platforms and it's not > clear to me why this would be platform specific. Is the test going to be flaky > on Linux? > > If you're confident it's due to a test-only issue on Windows and Mac, then > please elaborate in the comment a bit and I'll be ok with fixing it in a > followup. From what I see on the logs, there is an issue with the navigation not being caught. I think it may be due to the way the ShellJavascriptDialogManager is implemented on Windows and Mac, which is different than on Linux. In particular, I'm not sure it says proceed by default on those platforms, unlike on Linux. https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:826: // This CL tests that if a BeforeUnload dialog is destroyed due to the commit On 2016/03/29 20:19:06, nasko (slow) wrote: > nit: drop "CL". This is a test case, it just happens to be part of a CL, which > once committed won't exist as a concept. Done. I did meant to write tests don't know what happened there. https://codereview.chromium.org/1825523002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:844: // Click on a link in the page. This will show the BeforeUnload dialog. Have On 2016/03/29 20:19:06, nasko (slow) wrote: > nit: "Have it hang" doesn't convey why or what hangs? Something along the lines > of "Ensure that dialog is not dismissed, which will cause ... to wait ...". Done. https://codereview.chromium.org/1825523002/diff/60001/content/shell/browser/s... File content/shell/browser/shell_javascript_dialog_manager.h (right): https://codereview.chromium.org/1825523002/diff/60001/content/shell/browser/s... content/shell/browser/shell_javascript_dialog_manager.h:62: bool proceed_beforeunload_default_; On 2016/03/29 18:33:21, Charlie Reis wrote: > nit: should_proceed_on_beforeunload_? (The current name is a bit hard to > parse.) > > A comment here would also help. Done. https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... File content/test/content_browser_test_utils_internal.cc (right): https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... content/test/content_browser_test_utils_internal.cc:370: loop_runner_ = new MessageLoopRunner(); On 2016/03/29 20:19:06, nasko (slow) wrote: > Why not create it in the constructor? Because we will use it twice, and you can only use a MessageLoopRunner once AFAIU. https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... File content/test/content_browser_test_utils_internal.h (right): https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... content/test/content_browser_test_utils_internal.h:116: class NavigationDelayer : public WebContentsObserver { On 2016/03/29 20:19:06, nasko (slow) wrote: > I'd call this something more generic, as it ideally will be able to > block/drop/delay navigations. I had TestNavigationScheduler in my strawman CL a > year ago, but needn't be that. I went with TestNavigationManager. I can switch to Scheduler if you prefer. https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... content/test/content_browser_test_utils_internal.h:118: NavigationDelayer(WebContents* web_contents, const GURL& url); On 2016/03/29 20:19:06, nasko (slow) wrote: > Is the goal for this class to monitor any frame in WebContents? If so, let's put > a comment clarifying this on this constructor. I imagine in the future we will > want to monitor only a specific frame, especially since > cross_site_iframe_factory.html generates URLs that differ only in the query > string when they are for the same site. > > Alternatively, this might be a useful class to expose outside of content as it > is. I haven't looked at what tests could use it though. Precised it in a comment, + adde a TODO for a frame version. https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... content/test/content_browser_test_utils_internal.h:122: void WaitForNavigationPaused(); On 2016/03/29 20:19:06, nasko (slow) wrote: > At what point will navigation be paused? Let's use the naming of the method to > make this clear. For example: WaitForCommit(), WaitForRedirect(), etc. The > default behavior should probably be to defer the navigation at the point of > interest based on the method that was called and have the testcase itself resume > it once the intended checks have been completed. I renamed the method WaitForWillStartRequest since this is what it is waiting for. I think WaitForNavigationFinished is okay since we're waiting for DidFinishNavigation. https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... content/test/content_browser_test_utils_internal.h:125: // WaitForNavigationPaused. On 2016/03/29 18:33:21, Charlie Reis wrote: > Do you also need to call ResumeNavigation before calling this? That's what your > test is doing, and it's not clear here whether that's required. Yes. I precised it in the comment. Added a TODO to not need it in the future. https://codereview.chromium.org/1825523002/diff/60001/content/test/content_br... content/test/content_browser_test_utils_internal.h:136: // Called when the NavigationThrottlePauses the navigation. On 2016/03/29 20:19:06, nasko (slow) wrote: > nit: s/NavigationThrottlePauses/NavigationThrottle pauses/? Done. https://codereview.chromium.org/1825523002/diff/60001/content/test/data/hang_... File content/test/data/hang_before_unload.html (right): https://codereview.chromium.org/1825523002/diff/60001/content/test/data/hang_... content/test/data/hang_before_unload.html:3: <head><title>Possible BeforeUnload</title> On 2016/03/29 18:33:21, Charlie Reis wrote: > Why "Possible"? Because the first version did not have the dialog all the time. Changed it to BeforeUnload dialog https://codereview.chromium.org/1825523002/diff/60001/content/test/data/hang_... content/test/data/hang_before_unload.html:14: function clickLinkSoon() { On 2016/03/29 18:33:21, Charlie Reis wrote: > nit: Use consistent indent. Done. https://codereview.chromium.org/1825523002/diff/60001/content/test/data/hang_... content/test/data/hang_before_unload.html:19: window.addEventListener("beforeunload", function(e){ On 2016/03/29 18:33:21, Charlie Reis wrote: > nit: Space before { Done. https://codereview.chromium.org/1825523002/diff/60001/content/test/data/hang_... content/test/data/hang_before_unload.html:23: }); On 2016/03/29 18:33:21, Charlie Reis wrote: > nit: 2 fewer spaces. Done.
I have one comment about the API for TestNavigationManager, but I'm ok with addressing its API in a separate CL. LGTM. https://codereview.chromium.org/1825523002/diff/100001/content/test/content_b... File content/test/content_browser_test_utils_internal.h (right): https://codereview.chromium.org/1825523002/diff/100001/content/test/content_b... content/test/content_browser_test_utils_internal.h:129: // first use WaitForWillStartRequest, then call ResumeNavigation, and only Why this limitation? Simple to implement now or intended API behavior for test writers?
Thanks, LGTM. https://codereview.chromium.org/1825523002/diff/100001/content/test/content_b... File content/test/content_browser_test_utils_internal.h (right): https://codereview.chromium.org/1825523002/diff/100001/content/test/content_b... content/test/content_browser_test_utils_internal.h:135: void ResumeNavigation(); nit: Let's list this between WaitForWillStartRequest and WaitForNavigationFinished, so that they're in the order they're expected to be called.
carlosk@chromium.org changed reviewers: + carlosk@chromium.org
Thanks. https://codereview.chromium.org/1825523002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1825523002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4695: controller_.DiscardNonCommittedEntries(); This used to be called even if BeforeUnloadCanceled was not. Just to make sure: is this an intentional change? https://codereview.chromium.org/1825523002/diff/100001/content/test/content_b... File content/test/content_browser_test_utils_internal.h (right): https://codereview.chromium.org/1825523002/diff/100001/content/test/content_b... content/test/content_browser_test_utils_internal.h:129: // first use WaitForWillStartRequest, then call ResumeNavigation, and only On 2016/03/30 15:47:52, nasko (slow) wrote: > Why this limitation? Simple to implement now or intended API behavior for test > writers? I'm guessing the idea is to expand this helper class to cover more intermediary navigation steps. If that's the intention the I have an API suggestion. When TNM is created it pauses at the earliest step possible. Then when a Wait-call is made it allows navigations to cross all previous pause moments until the specific one for the call is reached. Future Wait-calls work the same way. ResumeNavigation would not be needed so much. Good to have: If that moment is not reachable anymore in for the current navigation then fail the test. Now looking at your TODO below it might be what you were already planning to do...
Thanks! https://codereview.chromium.org/1825523002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1825523002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4695: controller_.DiscardNonCommittedEntries(); On 2016/03/31 09:48:58, carlosk wrote: > This used to be called even if BeforeUnloadCanceled was not. Just to make sure: > is this an intentional change? Yes. This avoids deleting the entry when we're in the middle of committing it. https://codereview.chromium.org/1825523002/diff/100001/content/test/content_b... File content/test/content_browser_test_utils_internal.h (right): https://codereview.chromium.org/1825523002/diff/100001/content/test/content_b... content/test/content_browser_test_utils_internal.h:129: // first use WaitForWillStartRequest, then call ResumeNavigation, and only On 2016/03/31 09:48:58, carlosk wrote: > On 2016/03/30 15:47:52, nasko (slow) wrote: > > Why this limitation? Simple to implement now or intended API behavior for test > > writers? > > I'm guessing the idea is to expand this helper class to cover more intermediary > navigation steps. If that's the intention the I have an > API suggestion. > > When TNM is created it pauses at the earliest step possible. Then when a > Wait-call is made it allows navigations to cross all previous pause moments > until the specific one for the call is reached. Future Wait-calls work the same > way. ResumeNavigation would not be needed so much. > > Good to have: If that moment is not reachable anymore in for the current > navigation then fail the test. > > Now looking at your TODO below it might be what you were already planning to > do... Yes. Just it was easier to write the first version that way. https://codereview.chromium.org/1825523002/diff/100001/content/test/content_b... content/test/content_browser_test_utils_internal.h:135: void ResumeNavigation(); On 2016/03/30 19:04:58, Charlie Reis wrote: > nit: Let's list this between WaitForWillStartRequest and > WaitForNavigationFinished, so that they're in the order they're expected to be > called. Done.
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1825523002/#ps120001 (title: "Rebase + addressed nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1825523002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1825523002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1825523002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1825523002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1825523002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1825523002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1825523002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1825523002/120001
Message was sent while issue was closed.
Description was changed from ========== Do not reset navigation state when BeforeUnload is cancelled by a commit It is possible for a cross-site navigation to commit while a BeforeUnload confirmation dialog is showing. This will destroy dialog and call WebContentsImpl::OnDialogClosed. This function should not reset the navigatin state if the RenderFrameHost that was showing the dialog is no longer the current RenderFrameHost. BUG=589365 ========== to ========== Do not reset navigation state when BeforeUnload is cancelled by a commit It is possible for a cross-site navigation to commit while a BeforeUnload confirmation dialog is showing. This will destroy dialog and call WebContentsImpl::OnDialogClosed. This function should not reset the navigatin state if the RenderFrameHost that was showing the dialog is no longer the current RenderFrameHost. BUG=589365 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Do not reset navigation state when BeforeUnload is cancelled by a commit It is possible for a cross-site navigation to commit while a BeforeUnload confirmation dialog is showing. This will destroy dialog and call WebContentsImpl::OnDialogClosed. This function should not reset the navigatin state if the RenderFrameHost that was showing the dialog is no longer the current RenderFrameHost. BUG=589365 ========== to ========== Do not reset navigation state when BeforeUnload is cancelled by a commit It is possible for a cross-site navigation to commit while a BeforeUnload confirmation dialog is showing. This will destroy dialog and call WebContentsImpl::OnDialogClosed. This function should not reset the navigatin state if the RenderFrameHost that was showing the dialog is no longer the current RenderFrameHost. BUG=589365 Committed: https://crrev.com/bc6f4150a3eef02b926611404e7cdd44c9d67635 Cr-Commit-Position: refs/heads/master@{#384674} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/bc6f4150a3eef02b926611404e7cdd44c9d67635 Cr-Commit-Position: refs/heads/master@{#384674} |
