Navigation commits to wrong NavigationEntry due to ScopedPageLoadDeferrer |
||||
Issue descriptionVersion: 54.0.2791.0 and earlier OS: All What steps will reproduce the problem? 1) Visit https://csreis.github.io/tests 2) Click "simple.html" 3) Go back. 4) In DevTools: history.forward();print(); 5) Cancel printing. What is the expected output? We should have gone forward to simple.html. The forward button should be disabled, and going back should go to the previous page. What do you see instead? The simple.html page commits over the previous page's entry. The forward button stays enabled, allowing us to go forward to the same page. It's as if we did a location.replace instead of a forward navigation. This is due to the ScopedPageLoadDeferrer, which delays the navigation until after RenderFrameImpl::pending_navigation_params_ has been cleared. The same used to happen for JavaScript dialogs, until the nested message loop was removed. (This was discovered in issue 623319 . See comment 14.)
,
Jul 13 2016
I'm exploring a fix at https://codereview.chromium.org/2140393002/, and just trying to get the test to work. (I'm using print() since alert() doesn't use a nested message loop anymore.)
,
Jul 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/275d69fc4e8af8ab4a8e7d8fe621ca5b408c134d commit 275d69fc4e8af8ab4a8e7d8fe621ca5b408c134d Author: creis <creis@chromium.org> Date: Fri Jul 22 20:10:18 2016 Don't clear pending NavigationParams until didStopLoading. Clearing them at the end of NavigateInternal causes problems when a ScopedPageLoadDeferrer is in use. Credit to thestig@ for the test framework changes. BUG=626838 TEST=See bug for repro steps. Review-Url: https://codereview.chromium.org/2140393002 Cr-Commit-Position: refs/heads/master@{#407239} [modify] https://crrev.com/275d69fc4e8af8ab4a8e7d8fe621ca5b408c134d/chrome/browser/printing/print_preview_dialog_controller_browsertest.cc [modify] https://crrev.com/275d69fc4e8af8ab4a8e7d8fe621ca5b408c134d/content/renderer/render_frame_impl.cc
,
Jul 22 2016
,
Jul 27 2016
Re-opening because we're having to revert the fix, due to issue 631617 . We'll need to find another fix that clears the pending navigation params at an appropriate time, without disrupting other navigations. (Eventually, we'd love to not have to keep these as state at all.)
,
Jul 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d356ae14da5ff102779900b23033e2afdb16cccf commit d356ae14da5ff102779900b23033e2afdb16cccf Author: creis <creis@chromium.org> Date: Thu Jul 28 00:05:07 2016 Revert of Don't clear pending NavigationParams until didStopLoading. (patchset #5 id:80001 of https://codereview.chromium.org/2140393002/ ) Reason for revert: This caused a regression ( https://crbug.com/631617 ). We'll need to look for a safer way to clear the pending_navigation_params_ and try again. Original issue's description: > Don't clear pending NavigationParams until didStopLoading. > > Clearing them at the end of NavigateInternal causes problems when a > ScopedPageLoadDeferrer is in use. > > Credit to thestig@ for the test framework changes. > > BUG=626838 > TEST=See bug for repro steps. > > Committed: https://crrev.com/275d69fc4e8af8ab4a8e7d8fe621ca5b408c134d > Cr-Commit-Position: refs/heads/master@{#407239} TBR=avi@chromium.org,thestig@chromium.org,csharrison@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=626838 Review-Url: https://codereview.chromium.org/2190463006 Cr-Commit-Position: refs/heads/master@{#408285} [modify] https://crrev.com/d356ae14da5ff102779900b23033e2afdb16cccf/chrome/browser/printing/print_preview_dialog_controller_browsertest.cc [modify] https://crrev.com/d356ae14da5ff102779900b23033e2afdb16cccf/content/renderer/render_frame_impl.cc |
||||
►
Sign in to add a comment |
||||
Comment 1 by dcheng@chromium.org
, Jul 11 2016