[PlzNavigate] Use WebContentsGetter in PrerenderResourceThrottle

[PlzNavigate] Use WebContentsGetter in PrerenderResourceThrottle

BUG=668714, 673771
Committed: https://crrev.com/78a01d19d3321e75787d5d77a0e4e7006d1269c9
Cr-Commit-Position: refs/heads/master@{#438854}

[droger, 2016-12-13 11:20:49]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 11:21:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[droger, 2016-12-13 11:56:02]

droger@chromium.org changed reviewers:
+ clamy@chromium.org, mattcary@chromium.org

[commit-bot: I haz the power, 2016-12-13 12:06:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 12:06:15]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-12-13 12:12:12]

Thanks! Lgtm.

[droger, 2016-12-13 12:24:31]

droger@chromium.org changed reviewers:
+ pasko@chromium.org

[droger, 2016-12-13 12:24:32]

+pasko as OWNER

[droger, 2016-12-13 16:13:16]

pasko: hold on the review. This makes some tests fail (see crbug.com/673771),
not sure why the trybots did not catch it.

[droger, 2016-12-13 16:13:16]

pasko: hold on the review. This makes some tests fail (see crbug.com/673771),
not sure why the trybots did not catch it.

[droger, 2016-12-14 16:30:42]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 16:30:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[droger, 2016-12-14 16:35:40]

Description was changed from

==========
[PlzNavigate] Use WebContentsGetter in PrerenderResourceThrottle

BUG=668714
==========

to

==========
[PlzNavigate] Use WebContentsGetter in PrerenderResourceThrottle

BUG=668714, 673771
==========

[droger, 2016-12-14 16:55:37]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 16:56:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 18:08:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 18:08:57]

Dry run: This issue passed the CQ dry run.

[droger, 2016-12-15 09:46:21]

pasko: this is now ready for review.

https://codereview.chromium.org/2576443002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2576443002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_resource_throttle.cc:174: resource_type !=
content::RESOURCE_TYPE_MAIN_FRAME) {
This is the one line fix for the failing tests.

The test here is not intended for the main resource (see the line above, using a
subresource-related check).

The check for the main resource is done elsewhere (in PrerenderContents and in
WillRedirectRequestOnUI below for redirects).

The reason why it was failing the test is a bit complex. Here are some
explanations for posterity:

This was making the test fail because it would cause
FINAL_STATUS_UNSUPPORTED_SCHEME when requesting chrome://crash instead of
letting it go through and crashing the rendrerer.

This was only happening with plz navigate because of an ordering issue: with plz
navigate this code is called before the renderer is invoked, whereas with the
old flow the renderer is invoked first (and crashes before running this code).

Note that chrome://crash is able to go through in the test, because the check
for unsupported URLs in PrerenderContents is specifically disabled by
TestPrerenderContents.
So in real life, prerendering chrome://crash would not crash the prerenderer, it
only can do so in the test.

[droger, 2016-12-15 09:46:46]

Patchset #2 (id:20001) has been deleted

[droger, 2016-12-15 09:46:53]

Patchset #2 (id:40001) has been deleted

[pasko, 2016-12-15 15:25:45]

lgtm with a comment added referencing the bug

https://codereview.chromium.org/2576443002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2576443002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_resource_throttle.cc:174: resource_type !=
content::RESOURCE_TYPE_MAIN_FRAME) {
On 2016/12/15 09:46:21, droger wrote:
> This is the one line fix for the failing tests.
> 
> The test here is not intended for the main resource (see the line above, using
a
> subresource-related check).
> 
> The check for the main resource is done elsewhere (in PrerenderContents and in
> WillRedirectRequestOnUI below for redirects).
> 
> The reason why it was failing the test is a bit complex. Here are some
> explanations for posterity:
> 
> This was making the test fail because it would cause
> FINAL_STATUS_UNSUPPORTED_SCHEME when requesting chrome://crash instead of
> letting it go through and crashing the rendrerer.
> 
> This was only happening with plz navigate because of an ordering issue: with
plz
> navigate this code is called before the renderer is invoked, whereas with the
> old flow the renderer is invoked first (and crashes before running this code).
> 
> Note that chrome://crash is able to go through in the test, because the check
> for unsupported URLs in PrerenderContents is specifically disabled by
> TestPrerenderContents.
> So in real life, prerendering chrome://crash would not crash the prerenderer,
it
> only can do so in the test.

Thanks. That's very informative! Can we put a comment in the body of this branch
referencing a the bug with this description?

Like:
// Destroying the prerender for unsupported scheme only for non-main resource to
// allow chrome://crash to actually crash in the *RendererCrash tests instead of
// being intercepted here. The unsupported scheme for the main resource is
// checked in X. See http://crbug.com/673771.

[droger, 2016-12-15 16:25:02]

Thanks for the reviews.

https://codereview.chromium.org/2576443002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2576443002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_resource_throttle.cc:174: resource_type !=
content::RESOURCE_TYPE_MAIN_FRAME) {
On 2016/12/15 15:25:45, pasko wrote:
> On 2016/12/15 09:46:21, droger wrote:
> > This is the one line fix for the failing tests.
> > 
> > The test here is not intended for the main resource (see the line above,
using
> a
> > subresource-related check).
> > 
> > The check for the main resource is done elsewhere (in PrerenderContents and
in
> > WillRedirectRequestOnUI below for redirects).
> > 
> > The reason why it was failing the test is a bit complex. Here are some
> > explanations for posterity:
> > 
> > This was making the test fail because it would cause
> > FINAL_STATUS_UNSUPPORTED_SCHEME when requesting chrome://crash instead of
> > letting it go through and crashing the rendrerer.
> > 
> > This was only happening with plz navigate because of an ordering issue: with
> plz
> > navigate this code is called before the renderer is invoked, whereas with
the
> > old flow the renderer is invoked first (and crashes before running this
code).
> > 
> > Note that chrome://crash is able to go through in the test, because the
check
> > for unsupported URLs in PrerenderContents is specifically disabled by
> > TestPrerenderContents.
> > So in real life, prerendering chrome://crash would not crash the
prerenderer,
> it
> > only can do so in the test.
> 
> Thanks. That's very informative! Can we put a comment in the body of this
branch
> referencing a the bug with this description?
> 
> Like:
> // Destroying the prerender for unsupported scheme only for non-main resource
to
> // allow chrome://crash to actually crash in the *RendererCrash tests instead
of
> // being intercepted here. The unsupported scheme for the main resource is
> // checked in X. See http://crbug.com/673771.

Done.

Note also that DoesSubresourceURLHaveValidScheme() is only intended to be called
on sub resources.

If we wanted to check main resources, we would call DoesURLHaveValidScheme(). We
don't do it though, for the reasons explained above.

[droger, 2016-12-15 16:25:38]

The CQ bit was checked by droger@chromium.org

[droger, 2016-12-15 16:25:38]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
pasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2576443002/#ps80001
(title: "add comment")

[commit-bot: I haz the power, 2016-12-15 16:25:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 17:12:35]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481819138551460,
"parent_rev": "8569949e29fdd54fd6452095068b39b4a9d9f989", "commit_rev":
"213ec51d612cb9ba5267bed88b8adb4b92052c88"}

[commit-bot: I haz the power, 2016-12-15 17:13:03]

Description was changed from

==========
[PlzNavigate] Use WebContentsGetter in PrerenderResourceThrottle

BUG=668714, 673771
==========

to

==========
[PlzNavigate] Use WebContentsGetter in PrerenderResourceThrottle

BUG=668714, 673771

Review-Url: https://codereview.chromium.org/2576443002
==========

[commit-bot: I haz the power, 2016-12-15 17:13:04]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-12-15 17:14:44]

Description was changed from

==========
[PlzNavigate] Use WebContentsGetter in PrerenderResourceThrottle

BUG=668714, 673771

Review-Url: https://codereview.chromium.org/2576443002
==========

to

==========
[PlzNavigate] Use WebContentsGetter in PrerenderResourceThrottle

BUG=668714, 673771

Committed: https://crrev.com/78a01d19d3321e75787d5d77a0e4e7006d1269c9
Cr-Commit-Position: refs/heads/master@{#438854}
==========

[commit-bot: I haz the power, 2016-12-15 17:14:45]

Patchset 3 (id:??) landed as
https://crrev.com/78a01d19d3321e75787d5d77a0e4e7006d1269c9
Cr-Commit-Position: refs/heads/master@{#438854}