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

content/browser/frame_host/navigator_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/navigator_impl.h"
 
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/frame_tree.h"
@@ skipping 208 matching lines @@
   // to say when it's safe to leave aborted URLs in the omnibox, to let the user
   // edit the URL and try again.  This may be useful in cases that the committed
   // page cannot be attacker-controlled.  In these cases, we still allow the
   // view to clear the pending entry and typed URL if the user requests
   // (e.g., hitting Escape with focus in the address bar).
   //
   // Note: don't touch the transient entry, since an interstitial may exist.
   bool should_preserve_entry = controller_->IsUnmodifiedBlankTab() ||
       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);
   }
 
   if (delegate_)
     delegate_->DidFailProvisionalLoadWithError(render_frame_host, params);
 }
 
 void NavigatorImpl::DidFailLoadWithError(
@@ skipping 650 matching lines @@
         "Navigation.TimeToCommit_ExistingRenderer_BeforeUnloadDiscounted",
         time_to_commit);
     UMA_HISTOGRAM_TIMES(
         "Navigation.TimeToURLJobStart_ExistingRenderer_BeforeUnloadDiscounted",
         time_to_network);
   }
   navigation_data_.reset();
 }
 
 }  // namespace content