Add back the code to PrerenderManager::PendingSwap to handle RenderViewCreated being called for a...

chrome/browser/prerender/prerender_tracker.h

Index: chrome/browser/prerender/prerender_tracker.h
===================================================================
--- chrome/browser/prerender/prerender_tracker.h	(revision 244941)
+++ chrome/browser/prerender/prerender_tracker.h	(working copy)
@@ -85,7 +85,7 @@
     explicit PendingSwapThrottleData(const GURL& swap_url);
     ~PendingSwapThrottleData();
     GURL url;
-    base::WeakPtr<PrerenderPendingSwapThrottle> throttle;
+    std::vector<base::WeakPtr<PrerenderPendingSwapThrottle> > throttles;

[tburkard, 2014/01/16 05:46:43]

While the other vector is necessary, I think this vector may technically be ok
to do away with, if you add a check, as discussed.

This array is orthogonal to the other vector (since we will have one of this
vector for each entry in the other vector).

However, as discussed, I feel more comfortable keeping this vector, because
then, even if there are multiple requests for the resource for whatever reason,
we will do the right thing.

So this looks good!

[jam, 2014/01/16 06:39:48]

ah, I missed that, you're right this is orthogonal.

personally, I prefer if we know it can't happen that we don't add extra code to
deal with it. it just adds extra complexity, and as people refactor code they
won't know that this can't happen and will spend time handling this case. I've
added a CHECK to catch this. if it's triggered, which I don't see how that can
happen, then we can add code to deal with it.

[davidben, 2014/01/16 18:06:00]

Hrm. Actually, I think it might be possible in some races? (Certainly it's
possible with a misbehaving renderer; I don't know if we'd consider it okay for
a misbehaving renderer to hit a CHECK() in the browser.)

For well-behaved renderers, this is only tracked for a single process, so that
means we get an ordering on messages. Therefore, before a new MAIN_FRAME request
begins, I believe we should see DidFailProvisionalLoad from the renderer before
the new request. PendingSwaps should get canceled on DidFailProvisionalLoad
which would uninstall the PendingSwapThrottleData so we won't have two
throttles...

EXCEPT there are multiple threads in the browser process servicing these
requests. If I understand things correctly, this can happen: IPC flows from
renderer to IO thread to UI thread. Hops in that direction will retain their
ordering, but PendingSwap gets canceled on the UI thread and posted BACK to the
IO thread to uninstall the PendingSwapThrottleData. That UI -> IO thread edge
has no ordering guarantees w.r.t. the IPC stream and so it is possible that the
IO thread has already serviced the renderer's new MAIN_FRAME request and hit the
CHECK.

[jam, 2014/01/16 18:12:26]

I don't follow. Can you tell me exactly which method calls will lead to this
CHECK being hit? The check is to ensure that there aren't multiple pending swap
throttles for a main render frame's MAIN_FRAME request. I don't see how there
could be multiple throttles added in that case

[davidben, 2014/01/16 19:41:02]

Hrm. So, I started out writing out this very long step-by-step list of what
happens, and then I realized something and thought it was fine. And then I
looked at how request cancellation actually works and think it isn't again.
Soooo...

The situation is if the navigation gets canceled by starting another one. Then
there will be a second MAIN_FRAME request in flight and I believe the events and
PostTasks and thread-hops here are such that there can be a slight overlap. But
it's a pretty tight and unlikely race.


There might be multiple MAIN_FRAME requests for a main render frame if there are
two navigations in a row. The PendingSwap is supposed to get canceled when the
first navigation aborts, but there are thread-hops involved, so it's a little
asynchronous. This race is pretty tight and probably involves the UI thread a
little slow at the right point. The scenario I'm thinking of is:

1. Open two tabs, A and B.
2. A triggers a prerender P to example.com/1
3. B navigates to and completely loads example.com/2

=== Stage 1: attempt to use prerender P ===

4. B navigates to example.com/1. Although this is renderer-initiated, because
there is an prerender at that URL, the navigation goes through ShouldFork in the
renderer and goes up to the browser process.
5. WebContentsDelegate::OpenURLInTab is called which goes up to chrome::Navigate
where we call MaybeUsePrerenderedPage.
6. MaybeUsePrerenderedPage notices P matches in URL, but not sessionStorage
namespace (because P began in tab A instead of B). It creates a PendingSwap for
P which calls RenderViewCreated and registers the current top-level RenderFrame
(for example.com) with the PrerenderTracker.
7. Because this is the PendingSwap codepath, chrome::Navigate still calls
LoadURLInContents to go through with a normal navigation.
8. LoadURLInContents eventually sends a message to the renderer which responds
with IPCs for DidStartProvisionalLoad and begins a MAIN_FRAME network request
R1.
9. R1 matches the entry in the PrerenderTracker created in step 6 (because IPCs
go through the IO thread, the ordering constraints work out) and gets a
PrerenderPendingSwapResourceThrottle attached. ResourceLoader::StartRequest is
called and asks the resource throttles.
10. The throttle defers start and calls AddPendingSwapThrottleOnIOThread.

=== Stage 2: cancel the navigation ===

11. In all this time, P has not finished its merge yet and the normal navigation
has not committed (it can't; it's throttled), so example.com/2 is still live in
B and can run JS. B aborts that navigation by starting another one. Since
there's a URL check, say the new navigation is again to example.com/2 (it's
totally legitimate to navigate to URL again). To avoid complicating matters with
P being active for that URL, say it's now a form POST to that URL instead of a
GET. (Although, because P has an active PendingSwap, I think just a normal GET
works too.)
12. From Blink this sends the following events (there are probably several in
between I'm missing, but I believe this is the order they happen in).
    (a) cancel R1
    (b) didFailProvisionalLoad
    (c) didStartProvisionalLoad
    (d) start R2, a new MAIN_FRAME request to example.com/1
13. (a) reaches the IO thread. This cancels R1 in the browser BUT canceling a
request is asynchronous (delayed by a PostTask), so, as I understand, it does
not have an ordering guarantee w.r.t. events (b-d). The WeakPtr to the resource
throttle is NOT yet invalidated.
14. (b) reaches the IO thread and then is posted over the UI thread. When it
reaches the UI thread, the PendingSwap will be destroyed and its dtor will call
RemovePendingPrerenderSwap on the PrerenderTracker and remove the whole assembly
on the IO thread. BUT that task is also not ordered w.r.t event (d).
15. (c) reaches the IO thread and is posted over to the UI thread.
16. (d) reaches the IO thread and R2 is created in the browser. As in steps
9-10, it eventually calls AddPendingSwapThrottleOnIOThread. R1's WeakPtr has not
yet been invalidated (step 13) and the PendingSwapThrottleData is not yet gone
(step 14), so we hit the CHECK.
----
17. Without the check, step 14's asynchronous task would eventually reach the IO
thread, call RemovePendingPrerenderSwapOnIOThread, and release R2's throttle
because it shouldn't have been throttled in the first place.

   };
   // Set of throttles for pending swaps. The key is the routing ID pair
   // of a RenderFrame.