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

Issue 638088 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Regression

Blocking:
issue 614758



Sign in to add a comment

Regression: Chrome://help page seen blank after restarting browser.

Reported by rk...@etouch.net, Aug 16 2016

Issue description

Chrome Version: 54.0.2829.0 Revision aa5a827fabbe04bcabecf997edc18ffe72ced592-refs/heads/master@{#411902}
OS: Windows(7,8,10)

What steps will reproduce the problem?
(1) Launch chrome, navigate to chrome://help
(2) Restart browser, and observe the chrome://help page.

chrome://help page seen blank after restarting browser.

chrome://help page should not seen blank after restarting browser.

This is a regression issue, broken in 'M-54' will soon update the other info:

 

Comment 1 by rk...@etouch.net, Aug 16 2016

Labels: hasbisect
Owner: creis@chromium.org
Status: Assigned (was: Unconfirmed)
Good Build: 54.0.2820.0
Bad Build: 54.0.2821.0

Narrow Bisect:
https://chromium.googlesource.com/chromium/src/+log/1ec38faa31c347108fc99580ba9e3bb928e615f4..dd02e41f1e81ac54507ff79447b706447d5eb4ec?pretty=fuller&n=40

Suspecting: r410150 ?
Actual_Video.mp4
952 KB View Download
Expected_Video.mp4
431 KB View Download
Labels: ReleaseBlock-Stable
Adding release block label, please undo if not the case.
Cc: rnimmagadda@chromium.org
Still able to repro this issue on Chrome Canary Version - 54.0.2832.2 

Comment 4 by creis@chromium.org, Aug 21 2016

Components: UI>Browser>Navigation Internals>Sandbox>SiteIsolation
Status: Started (was: Assigned)
I'm looking into this one, and I can confirm that r410150 caused it.  At first it sounded like  issue 635403 , but this doesn't involve in-page navigations and only one of the subframes is blank, rather than the whole page.

As it turns out, the PageState for the chrome://help page actually looks broken after restart after we switched to the new navigation logic in r410150.  When exiting the browser (during chrome://restart), we appear to write a string to disk that includes the subframe URLs of chrome://uber-frame and chrome://chrome/help.  However, when reading the data back from disk at startup, the string no longer has the subframe URLs.  As a result, the SetPageState call to create the NavigationEntry at startup doesn't create subframe FrameNavigationEntries for those frames.  That's concerning, and definitely broken.  (Note that we have tests for this on normal pages, so there's something different about the chrome://help page.)

Before switching to the new navigation logic in r410150, we successfully wrote the subframe URLs to disk and read them back in again, so we used to restore the page correctly.  Interestingly, the old navigation logic was also robust against the bad PageState: if we tried to load a PageState for chrome://help with no subframe HistoryItems, the page loaded successfully anyway.  It appears that the page loads about:blank into the iframe first, and then script code causes the chrome://chrome/help iframe to load (similar to what happens the first time we visit the page).

Thus, there's two things we should fix here:
1) The new navigation logic should stop writing bad PageStates to disk, so that we remember the subframe URLs for chrome://help on startup.
2) The new navigation logic should be able to load chrome://help even when we don't have subframe URLs in the PageState (since this used to work before).

I'm still trying to understand why the PageState is bad, to come up with a fix for (1).  I haven't found any other pages it affects yet, but I wouldn't be surprised if there are more.

I think we can fix (2) by changing RenderFrameHostImpl::OnOpenURL to not tell the renderer to load about:blank as a fallback URL when no subframe FrameNavigationEntry is found.  The renderer is already on about:blank, and telling it to go there again just clobbers whatever the renderer is doing in that frame (similar to issue 626416).  I'm exploring a fix for that in https://codereview.chromium.org/2264633003/, but it's failing some tests (because we probably need to tell the frame that it isn't still loading).

I'll post an update when I understand more about the PageState corruption in (1).

Comment 5 by groby@chromium.org, Aug 22 2016

Note: We are moving some pages away from the uber page system, which means the PageState changes. chrome://help is one of them - it now has a different request URL, and a different PageState since the set of frames completely changed.

That means if for whatever reasons the flag for --enable-md-settings changed across runs, you have a broken pagestate. I have a fix for that in https://codereview.chromium.org/2166693002/ - but it tries to recreate a default PageState via PageState::CreateFromURL, which lacks all subframes. 

(If you can reproduce this bug without changing the settings between MD and old style, that's not the problem)

FWIW, I'm debugging my particular problem, and the execution path diverges in  RenderFrameImpl::SendUpdateState - current_history_item_.isNull() triggers for the PageState created via CreateFromURL

Comment 6 by groby@chromium.org, Aug 22 2016

Blocking: 614758

Comment 7 by creis@chromium.org, Aug 24 2016

Cc: groby@chromium.org nasko@chromium.org
Comment 5: Thanks-- it's useful to know that any field-trial-induced changes to the MD settings state can trigger another problem here.  The PageState problem between MD states that you describe should be fixed by (2) in comment 4.  (You may even be able to confirm locally by patching in https://codereview.chromium.org/2264633003/, though that has its own problems.)

Nasko and I have been debugging the separate issue in (1) in comment 4, where the new navigation path is somehow giving us an incomplete subframe-less PageState in the Current Session file on disk.  We were quite surprised to learn that the PageState returned by GetPageState (at exit) is basically identical between the two modes-- that's certainly the intention, but it's also where I expected the bug to be, given these symptoms.  (A difference there could have explained why we get different results written to disk.)

Instead, Nasko and I have a different theory to test.  We noticed that the Current Session file contains a subframe-less version of the PageState in both the new and old paths, while the old path also contains a PageState with subframes.  Perhaps the subframe-less PageState gets written at the time the main frame commits, and we don't write the version with subframes when they commit because those are AUTO_SUBFRAME navigations, which do not generate commit notifications.  The version of the PageState with subframes is not written to disk until we exit (and for some reason, we skip that step in the new navigation path).  That would explain the behavior we're seeing.

Evidence in favor: the bug does not repro if we navigate chrome://help to #foo before exiting.  That's an in-page main frame navigation which generates commit notifications and writes the PageState (with subframes) to disk.  This lends support to the theory that we aren't writing the PageState with subframes to disk at the time of commit, and somehow we must be failing to write it at exit in the new mode.

We'll need to debug to understand whether that's happening.  (It's certainly a bit unexpected that the new navigation path would affect that logic, but it's possible.)

Comment 8 by creis@chromium.org, Aug 24 2016

I think I found it.

In the old navigation path, we generate no commit notifications for AUTO_SUBFRAMEs, but there is a subsequent UpdateState message that comes in 1 second after commit.  WebContentsImpl::UpdateState then sends a NOTIFICATION_NAV_ENTRY_CHANGED, and SessionService::Observe hears it and writes it to disk.  (Side note: for some reason this Observe is called twice?  I'm not sure why.)

The interesting part is that UpdateState has a check that skips the notification if the PageState is the same as it was before.  However, in the old navigation path, it's actually *not* the same as what's stored on the NavigationEntry-- the previous PageState did not include the subframe, because we never updated the entry's PageState during RendererDidNavigateAutoSubframe!

In the new navigation path, we have a similar check for whether the PageState has changed in UpdateStateForFrame, where we're handing frame-specific PageStates rather than full-page PageStates.  In this case, we've already put the PageState on the FrameNavigationEntry during RendererDidNavigateAutoSubframe, so UpdateStateForFrame thinks there's no difference and doesn't send a notification.  Thus, SessionService never hears about it after either commit or UpdateState.

Amazingly, this implies that the GetPageState call made while exiting is not used at all for restore, in either the new or old modes!  We appear to rely on what has already been written to disk after UpdateState messages are received.

There's a lot of ugliness here that's worth cleaning up, but I've confirmed that just sending the notification from UpdateStateForFrame fixes this bug.  We can probably proceed with a simple version of that fix before branch cut.

Also, I've confirmed that this bug applies to other pages as well.  In most cases, it means we won't have subframe history items for any AUTO_SUBFRAME frames (at least until they scroll or add frame state), but that's usually not a problem.  It just means we'll load the fallback URL instead, which typically identical.  It should only matter when the frame starts out as about:blank and a later navigation is made to the frame, such that it's treated as AUTO_SUBFRAME.  In the new navigation path, the browser process won't find a history item and will fall back to about:blank.  Example:

1) Visit http://csreis.github.io/tests/cross-site-iframe-initially-blank.html
2) Click "Go cross-site (simple page)"
3) chrome://restart

I'll try to put together a fix.

Comment 9 by creis@chromium.org, Aug 24 2016

I have a fix for part (1) of comment 4 in the CQ:
https://codereview.chromium.org/2275643003/

I'll try to get a fix for part (2) soon as well.  (I'll merge it to M54 if I miss the branch cut.  It was more important to get part (1) fixed first to avoid writing incomplete PageStates to disk on beta channel.)
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/59d5a47cb75df2754b71732c5581f2bb6be54c9c

commit 59d5a47cb75df2754b71732c5581f2bb6be54c9c
Author: creis <creis@chromium.org>
Date: Wed Aug 24 23:57:19 2016

Notify about PageState changes after an AUTO_SUBFRAME commit.

The session restore logic (specifically when exiting and restarting)
depends on NOTIFICATION_NAV_ENTRY_CHANGED notifications for subframes.
In the old navigation logic, this happened for subframes during the
first UpdateState after an AUTO_SUBFRAME commit.  In the new navigation
logic, we have already put the PageState on the FrameNavigationEntry
at commit time, so the UpdateState typically looks like a no-op and
doesn't generate a notification.

To fix this (and get the notification out as early as possible), this
CL notifies at commit time for AUTO_SUBFRAMEs.

BUG= 638088 
TEST=Visit chrome://help, then chrome://restart.
NOTRY=true
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2275643003
Cr-Commit-Position: refs/heads/master@{#414207}

[modify] https://crrev.com/59d5a47cb75df2754b71732c5581f2bb6be54c9c/chrome/browser/sessions/session_restore_browsertest.cc
[add] https://crrev.com/59d5a47cb75df2754b71732c5581f2bb6be54c9c/chrome/test/data/iframe_blank.html
[modify] https://crrev.com/59d5a47cb75df2754b71732c5581f2bb6be54c9c/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/59d5a47cb75df2754b71732c5581f2bb6be54c9c/content/browser/frame_host/navigation_controller_impl_unittest.cc

Comment 11 by creis@chromium.org, Aug 25 2016

Labels: -ReleaseBlock-Stable
r414207 should fix part (1), so that we don't write incomplete PageState to disk anymore.  I'll follow up with the fix for (2), so I'll leave this open until then.

That said, the repro steps should work at this point, so I'll remove the RBS label.

Comment 12 by groby@chromium.org, Aug 25 2016

(2) is presumably blocking chrome://history and chrome://settings from shipping (they'll need to invalidate page state on load, so they have incomplete page state)

Any chance the fix for that is landing soon-ish?

Comment 13 by creis@chromium.org, Aug 25 2016

Comment 12: I'm not sure how to get around the "stop loading" problem (without introducing races) in the approach I was taking in https://codereview.chromium.org/2264633003/.  That said, there's a few other approaches I'm considering, to see if I can land something soon.

The most promising so far would be telling the renderer if there are no child history items at all for the frame, in which case we don't need to ask the browser process when a new child frame is created.  That's a step toward issue 639842, which we want to do anyway, and I think it should resolve the issue you're seeing with restoring across MD settings modes.  I just need to figure out where to store that state in the renderer and when to clear it.

(Another option is getting the renderer to ignore the fallback about:blank navigation if it has already started a load, similar to what we did for the dynamic iframe layout test.  That's a significant amount of plumbing for the fallback case, though, since it would require adding a new bit to all the OpenURL params which go out through content/public.  We should really not be using OpenURL there, but that's a much bigger change.)

I'll see if I can get the "no child history items" approach working.
Labels: TE-Verified-M54 TE-Verified-54.0.2840.6
Verified the fix on Windows 7, MAC (10.11.6) & Ubuntu Trusty (14.04) for Google Chrome Dev Version - 54.0.2840.6 

Screen-recording is attached.

TE-Verified labels is attached.

@creis: Could you please change the status accordingly.

Thank you.
638088.mp4
1.1 MB View Download

Comment 15 by creis@chromium.org, Aug 30 2016

rnimmagadda@: Thanks for verifying, but only half of this bug is fixed.  I'm leaving it open until we land the fix for part (2) of comment 4, since groby@ depends on that in the MD settings work.
Update: I have a test written for part (2) and some thoughts about the fix.  I want to try to head in the right direction for fixing issue 639842, which requires some thought about where to store the state on the renderer-side (especially when there are subtrees).  I should be able to get something working soon.
Project Member

Comment 17 by bugdroid1@chromium.org, Sep 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/45b3bba45f5c1276726541db2f6f8ca6c5e05b65

commit 45b3bba45f5c1276726541db2f6f8ca6c5e05b65
Author: creis <creis@chromium.org>
Date: Thu Sep 22 22:47:13 2016

Notify the renderer if a history navigation has no subframe items.

In this case, the renderer does not need to consult the browser
process if subframes are created during the navigation.  Since there
are no history items for it, the renderer can just load the default
URL.

BUG= 638088 , 639842
TEST=Restore chrome://settings after disabling MD settings mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2316003002
Cr-Commit-Position: refs/heads/master@{#420486}

[modify] https://crrev.com/45b3bba45f5c1276726541db2f6f8ca6c5e05b65/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/45b3bba45f5c1276726541db2f6f8ca6c5e05b65/content/browser/frame_host/navigation_entry_impl.cc
[modify] https://crrev.com/45b3bba45f5c1276726541db2f6f8ca6c5e05b65/content/browser/frame_host/navigation_entry_impl.h
[modify] https://crrev.com/45b3bba45f5c1276726541db2f6f8ca6c5e05b65/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/45b3bba45f5c1276726541db2f6f8ca6c5e05b65/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/45b3bba45f5c1276726541db2f6f8ca6c5e05b65/content/common/frame_messages.h
[modify] https://crrev.com/45b3bba45f5c1276726541db2f6f8ca6c5e05b65/content/common/navigation_params.cc
[modify] https://crrev.com/45b3bba45f5c1276726541db2f6f8ca6c5e05b65/content/common/navigation_params.h
[modify] https://crrev.com/45b3bba45f5c1276726541db2f6f8ca6c5e05b65/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/45b3bba45f5c1276726541db2f6f8ca6c5e05b65/content/renderer/render_frame_impl.h

Comment 18 by creis@chromium.org, Sep 23 2016

Status: Fixed (was: Started)
Part (2) of comment 4 should now be fixed.

Comment 19 by creis@chromium.org, Sep 30 2016

Labels: Merge-Request-54
Requesting to merge r420486 to M54, since the fix for  issue 649345  depends on it.  (This is also a useful CL to have in M54 in its own right, since it makes Chrome more robust to session restore when subframe history items are missing.)

Comment 20 by dimu@chromium.org, Sep 30 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 21 by bugdroid1@chromium.org, Sep 30 2016

Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c38c57dd850f83ba7d1a6713a0552175200efd1

commit 2c38c57dd850f83ba7d1a6713a0552175200efd1
Author: Charles Reis <creis@chromium.org>
Date: Fri Sep 30 19:54:05 2016

Notify the renderer if a history navigation has no subframe items.

In this case, the renderer does not need to consult the browser
process if subframes are created during the navigation.  Since there
are no history items for it, the renderer can just load the default
URL.

BUG= 638088 , 639842
TEST=Restore chrome://settings after disabling MD settings mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2316003002
Cr-Commit-Position: refs/heads/master@{#420486}
(cherry picked from commit 45b3bba45f5c1276726541db2f6f8ca6c5e05b65)

Review URL: https://codereview.chromium.org/2380943006 .

Cr-Commit-Position: refs/branch-heads/2840@{#604}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/browser/frame_host/navigation_entry_impl.cc
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/browser/frame_host/navigation_entry_impl.h
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/common/frame_messages.h
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/common/navigation_params.cc
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/common/navigation_params.h
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/renderer/render_frame_impl.h

Project Member

Comment 22 by bugdroid1@chromium.org, Sep 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1f7947be3253299a579a67d3fee5a9978d13d7cf

commit 1f7947be3253299a579a67d3fee5a9978d13d7cf
Author: creis <creis@chromium.org>
Date: Fri Sep 30 20:47:35 2016

Revert of Notify the renderer if a history navigation has no subframe items. (patchset #1 id:1 of https://codereview.chromium.org/2380943006/ )

Reason for revert:
Broke compile on M54.

Original issue's description:
> Notify the renderer if a history navigation has no subframe items.
>
> In this case, the renderer does not need to consult the browser
> process if subframes are created during the navigation.  Since there
> are no history items for it, the renderer can just load the default
> URL.
>
> BUG= 638088 , 639842
> TEST=Restore chrome://settings after disabling MD settings mode.
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Review-Url: https://codereview.chromium.org/2316003002
> Cr-Commit-Position: refs/heads/master@{#420486}
> (cherry picked from commit 45b3bba45f5c1276726541db2f6f8ca6c5e05b65)
>
> Committed: https://chromium.googlesource.com/chromium/src/+/2c38c57dd850f83ba7d1a6713a0552175200efd1

TBR=alexmos@chromium.org,nasko@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 638088 , 639842

Review-Url: https://codereview.chromium.org/2384833002
Cr-Commit-Position: refs/branch-heads/2840@{#606}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/browser/frame_host/navigation_entry_impl.cc
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/browser/frame_host/navigation_entry_impl.h
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/common/frame_messages.h
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/common/navigation_params.cc
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/common/navigation_params.h
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/renderer/render_frame_impl.h

Project Member

Comment 23 by bugdroid1@chromium.org, Sep 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9

commit bd0bc90fb57c339a8890c38ad1d2612009b2d5e9
Author: Charles Reis <creis@chromium.org>
Date: Fri Sep 30 22:24:51 2016

Notify the renderer if a history navigation has no subframe items.

In this case, the renderer does not need to consult the browser
process if subframes are created during the navigation.  Since there
are no history items for it, the renderer can just load the default
URL.

BUG= 638088 , 639842
TEST=Restore chrome://settings after disabling MD settings mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2316003002
Cr-Commit-Position: refs/heads/master@{#420486}
(cherry picked from commit 45b3bba45f5c1276726541db2f6f8ca6c5e05b65)

Review URL: https://codereview.chromium.org/2383303002 .

Cr-Commit-Position: refs/branch-heads/2840@{#610}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/browser/frame_host/navigation_entry_impl.cc
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/browser/frame_host/navigation_entry_impl.h
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/common/frame_messages.h
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/common/navigation_params.cc
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/common/navigation_params.h
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/renderer/render_frame_impl.h

Project Member

Comment 24 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c38c57dd850f83ba7d1a6713a0552175200efd1

commit 2c38c57dd850f83ba7d1a6713a0552175200efd1
Author: Charles Reis <creis@chromium.org>
Date: Fri Sep 30 19:54:05 2016

Notify the renderer if a history navigation has no subframe items.

In this case, the renderer does not need to consult the browser
process if subframes are created during the navigation.  Since there
are no history items for it, the renderer can just load the default
URL.

BUG= 638088 , 639842
TEST=Restore chrome://settings after disabling MD settings mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2316003002
Cr-Commit-Position: refs/heads/master@{#420486}
(cherry picked from commit 45b3bba45f5c1276726541db2f6f8ca6c5e05b65)

Review URL: https://codereview.chromium.org/2380943006 .

Cr-Commit-Position: refs/branch-heads/2840@{#604}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/browser/frame_host/navigation_entry_impl.cc
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/browser/frame_host/navigation_entry_impl.h
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/common/frame_messages.h
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/common/navigation_params.cc
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/common/navigation_params.h
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/2c38c57dd850f83ba7d1a6713a0552175200efd1/content/renderer/render_frame_impl.h

Project Member

Comment 25 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1f7947be3253299a579a67d3fee5a9978d13d7cf

commit 1f7947be3253299a579a67d3fee5a9978d13d7cf
Author: creis <creis@chromium.org>
Date: Fri Sep 30 20:47:35 2016

Revert of Notify the renderer if a history navigation has no subframe items. (patchset #1 id:1 of https://codereview.chromium.org/2380943006/ )

Reason for revert:
Broke compile on M54.

Original issue's description:
> Notify the renderer if a history navigation has no subframe items.
>
> In this case, the renderer does not need to consult the browser
> process if subframes are created during the navigation.  Since there
> are no history items for it, the renderer can just load the default
> URL.
>
> BUG= 638088 , 639842
> TEST=Restore chrome://settings after disabling MD settings mode.
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Review-Url: https://codereview.chromium.org/2316003002
> Cr-Commit-Position: refs/heads/master@{#420486}
> (cherry picked from commit 45b3bba45f5c1276726541db2f6f8ca6c5e05b65)
>
> Committed: https://chromium.googlesource.com/chromium/src/+/2c38c57dd850f83ba7d1a6713a0552175200efd1

TBR=alexmos@chromium.org,nasko@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 638088 , 639842

Review-Url: https://codereview.chromium.org/2384833002
Cr-Commit-Position: refs/branch-heads/2840@{#606}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/browser/frame_host/navigation_entry_impl.cc
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/browser/frame_host/navigation_entry_impl.h
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/common/frame_messages.h
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/common/navigation_params.cc
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/common/navigation_params.h
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/1f7947be3253299a579a67d3fee5a9978d13d7cf/content/renderer/render_frame_impl.h

Project Member

Comment 26 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9

commit bd0bc90fb57c339a8890c38ad1d2612009b2d5e9
Author: Charles Reis <creis@chromium.org>
Date: Fri Sep 30 22:24:51 2016

Notify the renderer if a history navigation has no subframe items.

In this case, the renderer does not need to consult the browser
process if subframes are created during the navigation.  Since there
are no history items for it, the renderer can just load the default
URL.

BUG= 638088 , 639842
TEST=Restore chrome://settings after disabling MD settings mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2316003002
Cr-Commit-Position: refs/heads/master@{#420486}
(cherry picked from commit 45b3bba45f5c1276726541db2f6f8ca6c5e05b65)

Review URL: https://codereview.chromium.org/2383303002 .

Cr-Commit-Position: refs/branch-heads/2840@{#610}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/browser/frame_host/navigation_entry_impl.cc
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/browser/frame_host/navigation_entry_impl.h
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/common/frame_messages.h
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/common/navigation_params.cc
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/common/navigation_params.h
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/bd0bc90fb57c339a8890c38ad1d2612009b2d5e9/content/renderer/render_frame_impl.h

Sign in to add a comment