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

Add back the code to PrerenderManager::PendingSwap to handle RenderViewCreated being called for a cross-site navigation.

The two tests are from David's https://codereview.chromium.org/140073003.

BUG=330735
R=davidben@chromium.org, tburkard@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245386

[jam, 2014-01-16 02:11:11]

Note: PrerenderBrowserTest.PrerenderPageNewTabCrossProcess makes me see that
RenderViewCreated is hit twice. However, the test still passes if on the second
call I do nothing. I tried playing with the timing of DOMStorageNamespace::Merge
but that didn't seem to change things (i.e. test passed either way).

I'm readding the vector because now at least I see that that method is called
twice.

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

LGTM

Clarification: by RenderViewCreated "is hit twice", you mean once when it's
called in the constructor, and then once observing, correct? (I'd be surprised
if the observing call happened twice).

The reason why the test passes is most likely because of timing: By the time the
renderer receives the navigation, requests the resource, and that request makes
it to the IO thread, the merge may have already finished.

One way this could be fixed in the test is to artificially delay the merge by a
wide margin (eg a few seconds) to ensure the browser will get a chance to
receive the fetch request.

https://codereview.chromium.org/138583003/diff/140001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.h (right):

https://codereview.chromium.org/138583003/diff/140001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.h:88:
std::vector<base::WeakPtr<PrerenderPendingSwapThrottle> > throttles;
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]

On 2014/01/16 05:46:43, tburkard wrote:
> LGTM
> 
> Clarification: by RenderViewCreated "is hit twice", you mean once when it's
> called in the constructor, and then once observing, correct? (I'd be surprised
> if the observing call happened twice).

twice is total (i.e. once by constructor and another through the observer
callback)

> 
> The reason why the test passes is most likely because of timing: By the time
the
> renderer receives the navigation, requests the resource, and that request
makes
> it to the IO thread, the merge may have already finished.
> 
> One way this could be fixed in the test is to artificially delay the merge by
a
> wide margin (eg a few seconds) to ensure the browser will get a chance to
> receive the fetch request.

btw i had tried pausing the dom storage thread when it gets to the merge call,
and it still succeeded..

https://codereview.chromium.org/138583003/diff/140001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.h (right):

https://codereview.chromium.org/138583003/diff/140001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.h:88:
std::vector<base::WeakPtr<PrerenderPendingSwapThrottle> > throttles;
On 2014/01/16 05:46:43, tburkard wrote:
> 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!

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]

https://codereview.chromium.org/138583003/diff/140001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.h (right):

https://codereview.chromium.org/138583003/diff/140001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.h:88:
std::vector<base::WeakPtr<PrerenderPendingSwapThrottle> > throttles;
On 2014/01/16 06:39:48, jam wrote:
> On 2014/01/16 05:46:43, tburkard wrote:
> > 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!
> 
> 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.

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.

https://codereview.chromium.org/138583003/diff/740002/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/138583003/diff/740002/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1183: content::RenderViewHost*
render_view_host) {
Hrm. So, was the goal of this work to get rid of RenderViewHost altogether or
just remove the IO-thread level state and convert to RenderFrameHost later?
Because this is still using RenderViewHost. (I ask because I'm thinking of doing
a follow-up that switches it to AboutToNavigateRenderView and avoids the
main_rfh_ids_ vector. But I didn't see a RenderFrame version.)

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

m

https://codereview.chromium.org/138583003/diff/140001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.h (right):

https://codereview.chromium.org/138583003/diff/140001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.h:88:
std::vector<base::WeakPtr<PrerenderPendingSwapThrottle> > throttles;
On 2014/01/16 18:06:00, David Benjamin wrote:
> On 2014/01/16 06:39:48, jam wrote:
> > On 2014/01/16 05:46:43, tburkard wrote:
> > > 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!
> > 
> > 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.
> 
> 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.

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

https://codereview.chromium.org/138583003/diff/740002/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/138583003/diff/740002/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1183: content::RenderViewHost*
render_view_host) {
On 2014/01/16 18:06:00, David Benjamin wrote:
> Hrm. So, was the goal of this work to get rid of RenderViewHost altogether or
> just remove the IO-thread level state and convert to RenderFrameHost later?

The latter. Eventually RenderViewCreated will be gone, but I have to work in
many small steps :) The first is removing IO thread usage of RV IDs.

> Because this is still using RenderViewHost. (I ask because I'm thinking of
doing
> a follow-up that switches it to AboutToNavigateRenderView and avoids the
> main_rfh_ids_ vector. But I didn't see a RenderFrame version.)

[davidben, 2014-01-16 19:41:01]

Sorry, this is kind of a long comment. I don't know how to repro this race, but
I believe this situation is possible with our ordering constraints. Depends on
how a PostTask from IPC message 1's handler can interleave with a later IPC
message 2's handler.

https://codereview.chromium.org/138583003/diff/140001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.h (right):

https://codereview.chromium.org/138583003/diff/140001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.h:88:
std::vector<base::WeakPtr<PrerenderPendingSwapThrottle> > throttles;
On 2014/01/16 18:12:26, jam wrote:
> On 2014/01/16 18:06:00, David Benjamin wrote:
> > On 2014/01/16 06:39:48, jam wrote:
> > > On 2014/01/16 05:46:43, tburkard wrote:
> > > > 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!
> > > 
> > > 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.
> > 
> > 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.
> 
> 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

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.

https://codereview.chromium.org/138583003/diff/740002/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.cc (right):

https://codereview.chromium.org/138583003/diff/740002/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.cc:121: CHECK(!it->second.throttle);
(If you do get rid of the CHECK, probably good to Resume the old throttle so it
doesn't get stuck indefinitely. Though I think it's always doomed to be
cancelled anyway.)

[davidben, 2014-01-16 19:44:09]

To be clear, if I'm even right about this race, I think it still shouldn't need
a std::vector. Just removing the CHECK and maybe Resume'ing the old throttle if
it's not yet invalidated is probably fine. That or maybe, if the CHECK would
have failed, don't override the old throttle and just immediately Resume the new
throttle you were just given?

[jam, 2014-01-16 20:28:12]

On 2014/01/16 19:44:09, David Benjamin wrote:
> To be clear, if I'm even right about this race, I think it still shouldn't
need
> a std::vector. Just removing the CHECK and maybe Resume'ing the old throttle
if
> it's not yet invalidated is probably fine. That or maybe, if the CHECK would
> have failed, don't override the old throttle and just immediately Resume the
new
> throttle you were just given?

Thanks for the long explanation. It's hard for me to evaluate if this can happen
or not.

We've had the CHECK in for 2 days. Do you mind if we leave as is, and if we get
crash reports, then I can handle this case? I'm generally averse to adding code
to deal with conditions unless I know for sure they can be reached.

[davidben, 2014-01-16 20:45:38]

Oh, has this already landed? If not, LGTM. Yeah, if it's fine that a malicious
renderer can easily hit the CHECK, I don't especially mind. I don't really like
races and this one can exist, but realistically I would be somewhat surprised if
it ever actually happened. Maybe I'll spend a few minutes later to try to
actually trigger it and follow-up if I succeed.

https://codereview.chromium.org/138583003/diff/740002/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/138583003/diff/740002/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1183: content::RenderViewHost*
render_view_host) {
On 2014/01/16 18:12:26, jam wrote:
> On 2014/01/16 18:06:00, David Benjamin wrote:
> > Hrm. So, was the goal of this work to get rid of RenderViewHost altogether
or
> > just remove the IO-thread level state and convert to RenderFrameHost later?
> 
> The latter. Eventually RenderViewCreated will be gone, but I have to work in
> many small steps :) The first is removing IO thread usage of RV IDs.
> 
> > Because this is still using RenderViewHost. (I ask because I'm thinking of
> doing
> > a follow-up that switches it to AboutToNavigateRenderView and avoids the
> > main_rfh_ids_ vector. But I didn't see a RenderFrame version.)
> 

Ah, okay. Would it be a problem and/or useful for you then if I switched it to
AboutToNavigateRenderView to avoid the vector?

[davidben, 2014-01-16 20:46:23]

> and this one can exist

and *I think* this one can exist. :-)

[jam, 2014-01-16 20:50:34]

https://codereview.chromium.org/138583003/diff/740002/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/138583003/diff/740002/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1183: content::RenderViewHost*
render_view_host) {
On 2014/01/16 20:45:38, David Benjamin wrote:
> On 2014/01/16 18:12:26, jam wrote:
> > On 2014/01/16 18:06:00, David Benjamin wrote:
> > > Hrm. So, was the goal of this work to get rid of RenderViewHost altogether
> or
> > > just remove the IO-thread level state and convert to RenderFrameHost
later?
> > 
> > The latter. Eventually RenderViewCreated will be gone, but I have to work in
> > many small steps :) The first is removing IO thread usage of RV IDs.
> > 
> > > Because this is still using RenderViewHost. (I ask because I'm thinking of
> > doing
> > > a follow-up that switches it to AboutToNavigateRenderView and avoids the
> > > main_rfh_ids_ vector. But I didn't see a RenderFrame version.)
> > 
> 
> Ah, okay. Would it be a problem and/or useful for you then if I switched it to
> AboutToNavigateRenderView to avoid the vector?

np to me, I was just trying to leave things in the same state before my earlier
change after your test showed me that RVCreated can be called twice.

[jam, 2014-01-17 00:10:11]

On 2014/01/16 20:50:34, jam wrote:
>
https://codereview.chromium.org/138583003/diff/740002/chrome/browser/prerende...
> File chrome/browser/prerender/prerender_manager.cc (right):
> 
>
https://codereview.chromium.org/138583003/diff/740002/chrome/browser/prerende...
> chrome/browser/prerender/prerender_manager.cc:1183: content::RenderViewHost*
> render_view_host) {
> On 2014/01/16 20:45:38, David Benjamin wrote:
> > On 2014/01/16 18:12:26, jam wrote:
> > > On 2014/01/16 18:06:00, David Benjamin wrote:
> > > > Hrm. So, was the goal of this work to get rid of RenderViewHost
altogether
> > or
> > > > just remove the IO-thread level state and convert to RenderFrameHost
> later?
> > > 
> > > The latter. Eventually RenderViewCreated will be gone, but I have to work
in
> > > many small steps :) The first is removing IO thread usage of RV IDs.
> > > 
> > > > Because this is still using RenderViewHost. (I ask because I'm thinking
of
> > > doing
> > > > a follow-up that switches it to AboutToNavigateRenderView and avoids the
> > > > main_rfh_ids_ vector. But I didn't see a RenderFrame version.)
> > > 
> > 
> > Ah, okay. Would it be a problem and/or useful for you then if I switched it
to
> > AboutToNavigateRenderView to avoid the vector?
> 
> np to me, I was just trying to leave things in the same state before my
earlier
> change after your test showed me that RVCreated can be called twice.

my bad, the check wasn't there before. Let's see if it fires now.