Fix incorrect creation of duplicate navigation entries for repeated page load failures.

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index d6ac418ebe47f48e932a2586a535d63b922cdaf3..527eda7d6c833688a45331a6b1d1c5ee40d9771b 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -227,7 +227,7 @@ void NavigatorImpl::DidFailProvisionalLoadWithError(
       delegate_->ShouldPreserveAbortedURLs();
   if (controller_->GetPendingEntry() != controller_->GetVisibleEntry() ||
       !should_preserve_entry) {

[Charlie Reis, 2015/04/03 20:59:46]

Hmm, the fact that this DiscardPendingEntry call is conditional bothers me. 
That means there are some ways for us to not have a failed_pending_entry_ even
though an error page may be coming.

In fact, give this a try: click a target=_blank link to a URL that will fail
with an error page.  That's an unmodified blank tab, so we leave the pending
entry around so the user can see what URL failed.  (This is helpful if there is
no error page, such as a 204 or if the user clicks Stop.)  However, that means
we won't have a failed entry in RendererDidNavigate when the error page commits.
 params.url_is_unreachable will be true, though, so we won't try to copy the
replace bit from the pending entry.

(I guess replace would be false in this particular example, but you can see how
this might be buggy.)

-    controller_->DiscardPendingEntry();
+    controller_->DiscardPendingEntry(true);

[davidben, 2015/04/03 19:44:11]

How does this work in the OOPIF world? It seems strange to me that something
about replace bit (a frame-level notion) is handled on the controller (a
tab-level notion). Or does location.replace not really work in OOPIF yet and
that's for future?

[Avi (use Gerrit), 2015/04/03 19:51:47]

I don't follow. How does location.replace figure in here? This "replace" bit
isn't location.replace, it's "we're doing a cross-process navigation, so even
though the new process is going to say that it makes a new nav item, replace the
current one instead".

[davidben, 2015/04/03 20:23:44]

I possibly stapled this to the wrong comment. NavigationController is shared
between all frames under one tab, right? So how do we make sure error pages work
if a cross-process location.replace in an iframe happens and that one fails. It
too should location.replace into the frame history entry.

Actually, as I'm typing this, it seems should_replace_entry is only on
NavigationEntry anyway. So we don't actually have browser-side frame history
entry at all yet... which I suppose is what your end goal is anyway? You can
probably disregard this comment. I'm guessing this is to be sorted out later
and/or I'm slightly confused?

[Avi (use Gerrit), 2015/04/03 20:30:36]

I'm pretty confused as well, so don't worry.

Right now, should_replace is on an entire NavEntry, so by definition it only
applies to entire pages. I don't know what its future is for oopifs.

This CL shouldn't change where we are in terms of the oopif implementation. I'm
pretty sure this flag doesn't work today at the frame level, and this CL
maintains the status quo.

[Charlie Reis, 2015/04/03 20:38:21]

David's point is valid, though I'm not sure location.replace is the right
example.

The replace bit is a frame-specific notion.  It can happen on things like
cross-process redirects or failed same-page navigations, which can happen in any
frame.

In contrast, pending entries (and the new failed pending entry) live on the
NavigationController for the whole tab.  If multiple frames are navigating at
the same time, they'll clobber each other's entries.

This is one of a few reasons I've been trying to avoid depending on pending
entries to store the state between a navigation's start and its commit.  There's
a lot of places where that happens today and it's not easy to kill them all.  I
have some hope for using PlzNavigate's NavigationRequest, but I don't think that
will be ready soon.

For this particular bug, we're trying to preserve some info (which had lived on
the pending entry) from the DidFail to the corresponding DidCommit of the error
page.  This is an awkward problem for a bunch of reasons:
1) NavigationController's classification logic is already relying on the pending
entry for the replace bit, which is unfortunate for the reasons above.
2) There's currently no link between the DidFail and the DidCommit of the error
page.  I'm not even sure how we would make such a link (e.g., does the renderer
know about the NavigationRequest even in PlzNavigate?).
3) I don't know if we can tell whether there even will be a DidCommit of an
error page at the time of DidFail.  Some DidFails (e.g., Stop, 204, downloads)
don't create an error page.  That means any state we keep around after DidFail
might stick around "forever" (or until the next commit from that frame).

This seems like a fairly fundamental flaw with how error pages are handled.  So
far, Avi's patch is dealing with #2, punting on #3 (by leaving the failed entry
around if there's no error page), and not yet addressing #1.

Ideally, we'd find a way that improves all of these.  But this is also blocking
the rest of the OOPIF session history work at the moment, so we may want to
consider it if it doesn't make things much worse.  It'd be nice to at least have
a plan for how to get rid of the failed entry notion, though.

[Avi (use Gerrit), 2015/04/06 19:38:17]

If you want me to come back to this after I kill page id, I can. But right now,
given that page id is blocking both you and Camille, I'd prefer to get this in
as a stop-gap.

 
     // Also force the UI to refresh.
     controller_->delegate()->NotifyNavigationStateChanged(INVALIDATE_TYPE_URL);