Create FrameNavigationEntries for the initial blank page in subframes.

Create FrameNavigationEntries for the initial blank page in subframes.

We did not create these before to allow us to detect whether a real
page has committed or not, but that leads to problems when navigations
commit in subframes with about:blank parent frames.

BUG=508938
TEST=See bug comment 2 for repro steps.

Committed: https://crrev.com/f0f069a2a8b0398b58b3a39ddad928093f03ae8a
Cr-Commit-Position: refs/heads/master@{#340215}

[Charlie Reis, 2015-07-23 19:52:57]

creis@chromium.org changed reviewers:
+ avi@chromium.org

[Charlie Reis, 2015-07-23 19:57:34]

Avi, can you take a look?  This is necessary for my location.replace fixes, and
it should resolve the debug crashes Nasko was seeing in
https://crbug.com/508938.

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:962: return
NAVIGATION_TYPE_NAV_IGNORE;
I think all the IGNORE cases here are when the main frame has no NavigationEntry
for the initial blank page.  That's worth revisiting as well, but in this CL I'm
only changing whether subframes have FNEs for the initial blank page.

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1234: // 3.
A real same-site navigation in the nested iframe should be AUTO.
I moved this into a nested frame to better test bug 508938, where we were
failing with a real navigation when the parent frame was still about:blank.

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1302: //
not the initial blank page.
This case made it worthwhile to move HasCommittedRealLoad from
NavigationController to FrameTreeNode.  Before we could just check whether there
was a FNE for the frame, but now we need to tell whether an about:blank FNE is
the initial one or not.  Seems easier to cache that alongside FTN::current_url_.

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:548: NOTREACHED() <<
"Shouldn't see a commit for a subframe before parent.";
This reverts https://crrev.com/334385.  (I can put the TODO to kill the renderer
back as well, since we should probably do that once we have confidence we can't
reach this case.)

[Avi (use Gerrit), 2015-07-23 20:27:58]

lgtm

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:118: bool has_committed_real_load()
const {
// Returns true iff SetCurrentURL has been called

?

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:962: return
NAVIGATION_TYPE_NAV_IGNORE;
On 2015/07/23 19:57:34, Charlie Reis wrote:
> I think all the IGNORE cases here are when the main frame has no
NavigationEntry
> for the initial blank page.

What horrible edge case is it where the there is no last-committed entry but
it's not the initial blank page?

> That's worth revisiting as well, but in this CL I'm
> only changing whether subframes have FNEs for the initial blank page.

Fair enough.

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1232:
EXPECT_FALSE(root->child_at(1)->has_committed_real_load());
Can we tell the difference between a new iframe with no url and one with
about:blank? Does the spec specify any behavior that would require us to?

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1234: // 3.
A real same-site navigation in the nested iframe should be AUTO.
On 2015/07/23 19:57:34, Charlie Reis wrote:
> I moved this into a nested frame to better test bug 508938, where we were
> failing with a real navigation when the parent frame was still about:blank.

Heh.

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1302: //
not the initial blank page.
On 2015/07/23 19:57:34, Charlie Reis wrote:
> This case made it worthwhile to move HasCommittedRealLoad from
> NavigationController to FrameTreeNode.  Before we could just check whether
there
> was a FNE for the frame, but now we need to tell whether an about:blank FNE is
> the initial one or not.  Seems easier to cache that alongside
FTN::current_url_.

Acknowledged.

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:548: NOTREACHED() <<
"Shouldn't see a commit for a subframe before parent.";
On 2015/07/23 19:57:34, Charlie Reis wrote:
> This reverts https://crrev.com/334385.  (I can put the TODO to kill the
renderer
> back as well, since we should probably do that once we have confidence we
can't
> reach this case.)

Acknowledged.

[Charlie Reis, 2015-07-23 22:02:23]

Thanks!

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:118: bool has_committed_real_load()
const {
On 2015/07/23 20:27:58, Avi wrote:
> // Returns true iff SetCurrentURL has been called
> 
> ?

...with a non-blank URL.

Done.

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:962: return
NAVIGATION_TYPE_NAV_IGNORE;
On 2015/07/23 20:27:58, Avi wrote:
> On 2015/07/23 19:57:34, Charlie Reis wrote:
> > I think all the IGNORE cases here are when the main frame has no
> NavigationEntry
> > for the initial blank page.
> 
> What horrible edge case is it where the there is no last-committed entry but
> it's not the initial blank page?

Sorry, reparse that as "when the main frame has no NavigationEntry, which means
it's on the initial blank page."  There's no case where it's on a real page but
doesn't have a last committed entry.

> 
> > That's worth revisiting as well, but in this CL I'm
> > only changing whether subframes have FNEs for the initial blank page.
> 
> Fair enough.

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1232:
EXPECT_FALSE(root->child_at(1)->has_committed_real_load());
On 2015/07/23 20:27:58, Avi wrote:
> Can we tell the difference between a new iframe with no url and one with
> about:blank? Does the spec specify any behavior that would require us to?

Not to my knowledge.  That's partly why I did both here, to show that they're
treated the same way.

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1255573005/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:548: NOTREACHED() <<
"Shouldn't see a commit for a subframe before parent.";
On 2015/07/23 20:27:58, Avi wrote:
> On 2015/07/23 19:57:34, Charlie Reis wrote:
> > This reverts https://crrev.com/334385.  (I can put the TODO to kill the
> renderer
> > back as well, since we should probably do that once we have confidence we
> can't
> > reach this case.)
> 
> Acknowledged.

Done.

[Charlie Reis, 2015-07-23 22:02:36]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2015-07-23 22:02:37]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1255573005/#ps40001
(title: "Update comments")

[commit-bot: I haz the power, 2015-07-23 22:04:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1255573005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1255573005/40001

[commit-bot: I haz the power, 2015-07-23 22:29:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-23 22:29:44]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Charlie Reis, 2015-07-23 23:12:46]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-07-23 23:12:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1255573005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1255573005/40001

[commit-bot: I haz the power, 2015-07-23 23:52:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-23 23:52:50]

Patchset 3 (id:??) landed as
https://crrev.com/f0f069a2a8b0398b58b3a39ddad928093f03ae8a
Cr-Commit-Position: refs/heads/master@{#340215}