OOPIF: Fix willSendRequest in A-B-A nested case.

OOPIF: willSendRequest in A-B-A nested case.

Before, we weren't using the transferred request IDs from the correct
frame, so the transfer failed and we were waiting for the network
stack to time out and issue the request again.  As a result, the inner
A request was very slow.

This CL also expands test coverage for auto subframe navigations.

BUG=236848
TEST=A-B-A nested OOPIFs are no longer slow.

Committed: https://crrev.com/6d58b4f33a0bd7d92b044ab16784aae69d517594
Cr-Commit-Position: refs/heads/master@{#340003}

[Charlie Reis, 2015-07-07 23:32:24]

creis@chromium.org changed reviewers:
+ avi@chromium.org, japhet@chromium.org

[Charlie Reis, 2015-07-07 23:32:24]

Nate, can you take a look at the render_frame_impl.cc change?  You did something
similar in https://codereview.chromium.org/1002953004/, but Nick reverted that
CL in https://codereview.chromium.org/1108863002.  I'm trying to determine if
there was a reason it operated on the top frame.  The try jobs seem happy with
using the current frame instead.

Avi, can you take a look at the test?  The render_frame_impl.cc change makes it
so the A-B-A case works, since we were losing the
transferred_request_{child,request}_id by using the wrong frame.

[Nate Chapin, 2015-07-07 23:36:33]

On 2015/07/07 23:32:24, Charlie Reis wrote:
> Nate, can you take a look at the render_frame_impl.cc change?  You did
something
> similar in https://codereview.chromium.org/1002953004/, but Nick reverted that
> CL in https://codereview.chromium.org/1108863002.  I'm trying to determine if
> there was a reason it operated on the top frame.  The try jobs seem happy with
> using the current frame instead.
> 
> Avi, can you take a look at the test?  The render_frame_impl.cc change makes
it
> so the A-B-A case works, since we were losing the
> transferred_request_{child,request}_id by using the wrong frame.

Seems reasonable to me, but I don't know this code well enough to be very
certain.

[Avi (use Gerrit), 2015-07-08 18:49:23]

Same here. This looks plausible, but I'm not sure enough to say "this is
definitely the right way to go".

I'll stamp it if you want, tho.

https://codereview.chromium.org/1225593003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1225593003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1398:
LoadCommittedCapturer capturer(shell()->web_contents());
Why a LoadCommittedCapturer? A LoadCommittedCapturer is strictly weaker than a
FrameNavigateParamsCapturer, as it only captures the params, not the details,
but a LoadCommittedCapturer is useful when we only have a load commit but not a
navigation.

I can understand its use above for iframe creation, but for a load like this, we
should capture the full navigation and test for the classification as well as
the transition type.

[Charlie Reis, 2015-07-08 20:27:03]

Hmm.  I'm not sure who knows it better.  I'll try to take a closer look to
justify the change, and we can go from there.

https://codereview.chromium.org/1225593003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1225593003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1398:
LoadCommittedCapturer capturer(shell()->web_contents());
On 2015/07/08 18:49:23, Avi wrote:
> Why a LoadCommittedCapturer? A LoadCommittedCapturer is strictly weaker than a
> FrameNavigateParamsCapturer, as it only captures the params, not the details,
> but a LoadCommittedCapturer is useful when we only have a load commit but not
a
> navigation.
> 
> I can understand its use above for iframe creation, but for a load like this,
we
> should capture the full navigation and test for the classification as well as
> the transition type.

I don't understand how this one is different than those above.  These are all
cases where we create a new frame and wait for AUTO_SUBFRAME, which I thought we
couldn't use FrameNavigateParamsCapturer for.

[Avi (use Gerrit), 2015-07-08 20:36:15]

https://codereview.chromium.org/1225593003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1225593003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1398:
LoadCommittedCapturer capturer(shell()->web_contents());
On 2015/07/08 20:27:03, Charlie Reis wrote:
> On 2015/07/08 18:49:23, Avi wrote:
> > Why a LoadCommittedCapturer? A LoadCommittedCapturer is strictly weaker than
a
> > FrameNavigateParamsCapturer, as it only captures the params, not the
details,
> > but a LoadCommittedCapturer is useful when we only have a load commit but
not
> a
> > navigation.
> > 
> > I can understand its use above for iframe creation, but for a load like
this,
> we
> > should capture the full navigation and test for the classification as well
as
> > the transition type.
> 
> I don't understand how this one is different than those above.  These are all
> cases where we create a new frame and wait for AUTO_SUBFRAME, which I thought
we
> couldn't use FrameNavigateParamsCapturer for.

OK. That's right.

[Charlie Reis, 2015-07-09 17:40:12]

Good news: I looked more closely at the willSendRequest code affected by this,
and I found additional bugs that it fixes.  It's almost certainly the right
change to make.

It affected cross-process location.replace in subframes, since we weren't
remembering to replace the current entry.  It turns out there was a second bug
preventing cross-process location.replace (for both main frames and subframes):
https://crbug.com/317872.  I've fixed that here as well.

Only downside is that we need a small Blink patch to land first, since there was
an assert preventing this:
https://codereview.chromium.org/1223373002/

https://codereview.chromium.org/1225593003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1225593003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:737:
EXPECT_EQ(NAVIGATION_TYPE_EXISTING_PAGE, capturer.details().type);
We were treating this as NEW_PAGE before, with |replace_entry| set to true. 
That did replace the entry but incorrectly cleared the forward history
(https://crbug.com/317872) and was inconsistent with the same-process case
above.

https://codereview.chromium.org/1225593003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:969:
EXPECT_EQ(ui::PAGE_TRANSITION_AUTO_SUBFRAME, capturer.transition_type());
We were treating this as NEW_SUBFRAME, which didn't replace the entry.

https://codereview.chromium.org/1225593003/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/1225593003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3120: WebDataSource* frame_ds =
frame->provisionalDataSource();
Noticed this was redundant now.

https://codereview.chromium.org/1225593003/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1225593003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3181: should_replace_current_entry =
data_source->replacesCurrentHistoryItem();
This adds to my confidence in this change, because it was (unsurprisingly) wrong
when we got the navigation_state from the top frame.

I had to make one more change to NavigateInternal to get cross-process
location.replace in a subframe to work, but now it's behaving.

https://codereview.chromium.org/1225593003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3235:
navigation_state->start_params().transferred_request_child_id);
(This was what motivated this CL in the first place, fixing the A-B-A case.)

https://codereview.chromium.org/1225593003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:4423:
blink::WebFrameLoadType::RedirectWithLockedBackForwardList :
This is the fix for https://crbug.com/317872, and it works for both main frames
and subframes.

[Avi (use Gerrit), 2015-07-09 19:05:53]

This looks reasonable, and since it causes all the cross-process tests to yield
the same results as the non-cross-process tests, I will approve.

LGTM

https://codereview.chromium.org/1225593003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1225593003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:737:
EXPECT_EQ(NAVIGATION_TYPE_EXISTING_PAGE, capturer.details().type);
On 2015/07/09 17:40:11, Charlie Reis wrote:
> We were treating this as NEW_PAGE before, with |replace_entry| set to true. 
> That did replace the entry but incorrectly cleared the forward history
> (https://crbug.com/317872) and was inconsistent with the same-process case
> above.

Acknowledged. It's nice that it classifies just like the existing
location.replace.

https://codereview.chromium.org/1225593003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:969:
EXPECT_EQ(ui::PAGE_TRANSITION_AUTO_SUBFRAME, capturer.transition_type());
On 2015/07/09 17:40:11, Charlie Reis wrote:
> We were treating this as NEW_SUBFRAME, which didn't replace the entry.

Same as above; it's nice to see that this classifies the same as non-cross-site.

[Charlie Reis, 2015-07-22 21:23:54]

Avi, would you mind taking another look?  I rebased and effectively rolled this
back to patch 2, leaving out the fix for location.replace for now.

The fix for location.replace is getting much more involved, and it just happens
to rely on these initial changes.  I'd like to land these first to make the
location.replace fix easier to review.

[Avi (use Gerrit), 2015-07-22 21:36:01]

On 2015/07/22 21:23:54, Charlie Reis wrote:
> Avi, would you mind taking another look?  I rebased and effectively rolled
this
> back to patch 2, leaving out the fix for location.replace for now.
> 
> The fix for location.replace is getting much more involved, and it just
happens
> to rely on these initial changes.  I'd like to land these first to make the
> location.replace fix easier to review.

Still lgtm

[Charlie Reis, 2015-07-22 22:46:24]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-07-22 22:47:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225593003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1225593003/100001

[commit-bot: I haz the power, 2015-07-23 00:22:54]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-23 00:24:25]

Patchset 6 (id:??) landed as
https://crrev.com/6d58b4f33a0bd7d92b044ab16784aae69d517594
Cr-Commit-Position: refs/heads/master@{#340003}