New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment
link

Issue 626838: Navigation commits to wrong NavigationEntry due to ScopedPageLoadDeferrer

Reported by creis@chromium.org, Jul 8 2016 Project Member

Issue description

Version: 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.)
 

Comment 1 by dcheng@chromium.org, Jul 11 2016

Cc: japhet@chromium.org

Comment 2 by creis@chromium.org, Jul 13 2016

Cc: thestig@chromium.org
Status: Started (was: Assigned)
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.)

Comment 3 by bugdroid1@chromium.org, Jul 22 2016

Project Member
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

Comment 4 by creis@chromium.org, Jul 22 2016

Status: Fixed (was: Started)

Comment 5 by creis@chromium.org, Jul 27 2016

Status: Assigned (was: Fixed)
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.)

Comment 6 by bugdroid1@chromium.org, Jul 28 2016

Project Member
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

Comment 7 Deleted

Sign in to add a comment