Remove the use of ProvisionalChangeToMainFrameUrl from prerender code.

chrome/browser/prerender/prerender_manager.cc

 // Copyright (c) 2012 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 "chrome/browser/prerender/prerender_manager.h"
 
 #include <algorithm>
 #include <functional>
 #include <string>
 #include <vector>
@@ skipping 1075 matching lines @@
   }
 
   seen_target_route_id_ = true;
   target_route_id_ = PrerenderTracker::ChildRouteIdPair(
       render_view_host->GetMainFrame()->GetProcess()->GetID(),
       render_view_host->GetMainFrame()->GetRoutingID());
   manager_->prerender_tracker()->AddPrerenderPendingSwap(
       target_route_id_, url_);
 }
 
-void PrerenderManager::PendingSwap::ProvisionalChangeToMainFrameUrl(
-        const GURL& url,
-        content::RenderFrameHost* render_frame_host) {
-  // We must only cancel the pending swap if the |url| navigated to is not
-  // the URL being attempted to be swapped in. That's because in the normal
-  // flow, a ProvisionalChangeToMainFrameUrl will happen for the URL attempted
-  // to be swapped in immediately after the pending swap has issued its merge.
-  if (url != url_)

[tburkard, 2014/09/17 14:19:42]

How about this logic here? Why was this removed completely?

[tburkard, 2014/09/17 14:21:02]

Ah I see that davidben@ mentioned that this may be removed. I'd feel more
comfortable leaving it in there in this change, and consider removing it in a
separate change, and verifying that this is indeed appropriate.

[davidben, 2014/09/17 15:08:30]

That logic is actually a pretty good indication that this check was either
broken or redundant anyway. :-) If we were actually relying on it, we'd fail to
abort the async swap if you GET a URL and, before it commits, cancel the
navigation and decide to POST to that same URL instead.

[tburkard, 2014/09/18 09:36:04]

Saying a certain code path is not needed because it would result in a bug in the
event of a very unlikely corner case does not seem like valid reasoning.

Yes, there may be a bug related to this very uncommon corner case. This does not
mean that because of this, this check is redundant. Removing it could create a
whole new class of more common bugs.

Applying your logic, you could probably prune a ton of stuff in Chrome (esp.
related to navigations) and completely break Chrome in the process.

I don't see what the big issue is of duplicating this code, especially for the
purpose of landing this change.

We can file a bug and later carefully assess whether this check is indeed
redundant an can be removed, but unless we have a better answer than your logic
of "if we rely on this there is a bug for a corner case, so it must be
redundant", I am not okay with just removing it.

[Avi (use Gerrit), 2014/09/18 21:00:21]

OK, I'm putting it back.

[davidben, 2014/09/18 22:14:21]

Heh. Well, the better answer is that DidFailProvisionalLoad will always fire. I
thought I'd mentioned this in a previous review, actually, though I can't seem
to find it. (I'd certainly been operating under the assumption it was defensive
when I tried to move the swap-in later. Though that work had since been set
aside for PlzNavigate which should let us do this properly.)

There's three cases where this'll trigger:

a) We get a redirect on the navigation request we are intercepting.

b) We start a new navigation.

c) The previous navigation raced with the one we're intercepting.

(a) cannot happen if things are working; the navigation request will get
throttled. If some other issue causes us to fail catch and throttle the request,
we're already racing the swap with the actual navigation and they're the same
anyway, so canceling the swap or not doesn't matter too much.

For (b), we will always get a DidFailProvisionalLoad before this signal which
will abort the pending swap.

For (c), this code actually makes us incorrectly clear the pending swap. But it
doesn't matter because we'll incorrectly clear it later anyway when the
DidFailProvisonalLoad or DidCommitProvisionalLoadForFrame of the previous
navigation failing to starting goes through.


Moreover, the failure mode of us failing to clear a pending swap is actually not
that bad. (If it were, some of these races would be a bigger problem than they
are.) We're swapping out entire WebContentses, so the worst that happens is that
we swap to the prerender when the navigation actually aborted and/or was
replaced by another. But everything is still internally consistent. (URLs match
what is shown, etc.)

Investigating the removal separately sounds reasonable though.

-    prerender_data_->ClearPendingSwap();
-}
-
 void PrerenderManager::PendingSwap::DidCommitProvisionalLoadForFrame(
     content::RenderFrameHost* render_frame_host,
     const GURL& validated_url,
     content::PageTransition transition_type) {
   if (render_frame_host->GetParent())
     return;
   prerender_data_->ClearPendingSwap();
 }
 
 void PrerenderManager::PendingSwap::DidFailProvisionalLoad(
@@ skipping 768 matching lines @@
     content::RenderProcessHost* host) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   prerender_process_hosts_.erase(host);
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&PrerenderTracker::RemovePrerenderCookieStoreOnIOThread,
                  base::Unretained(prerender_tracker()), host->GetID(), false));
 }
 
 }  // namespace prerender