[Prerender] Ignores resources with unsupported schemes for Offline reqs

[Prerender] Ignores resources with unsupported schemes for Offline reqs

For offlining a page with the prerender, this changes the policy when
encountering a resource with an unsupported scheme - instread of failing
the prerender (as is done with other prerender requests), it will only
cancel the unsupported resource and attempt to continue with prerendering.

This has been tested end-end on 5 articles linked from www.foxsports.com
that have a "blob:" subresource and with this change, we are able to
download and offline version of them.

BUG=686192
Review-Url: https://codereview.chromium.org/2661703002
Cr-Commit-Position: refs/heads/master@{#449337}
Committed: https://chromium.googlesource.com/chromium/src/+/f1ba435af9338792a78fc422eb9cd1a51a14aa13

[dougarnett, 2017-01-27 20:39:52]

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

[commit-bot: I haz the power, 2017-01-27 20:40:34]

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

[dougarnett, 2017-01-27 21:30:10]

dougarnett@chromium.org changed reviewers:
+ chili@chromium.org, dimich@chromium.org, pasko@chromium.org

[dougarnett, 2017-01-27 21:30:11]

pasko: code and concept review
dimich, chili: concept review and consider for new background loader

[commit-bot: I haz the power, 2017-01-27 21:48:44]

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

[commit-bot: I haz the power, 2017-01-27 21:48:45]

Dry run: This issue passed the CQ dry run.

[pasko, 2017-02-01 01:08:09]

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

[pasko, 2017-02-01 01:08:09]

Overall this looks harmless to me, so I tend to just LG, glad that it helps your
case.

I am not familiar with data urls and how they work.

Do data urls always leave blink or only in some cases, or maybe it is racy?

+japhet@ as expert in blink loading code.

One more potential point of worry: websites are likely not ready to fail loading
data urls, so they could end up in a broken state in cache/localstorage.
Theoretical.

https://codereview.chromium.org/2661703002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2661703002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_throttle.cc:187: // For
OFFLINE_ORIGIN, simply cancel this invalid request and continue.
nit: s/OFFLINE_ORIGIN/ORIGIN_OFFLINE/

[dougarnett, 2017-02-06 17:05:01]

Btw, I tried both letting the blob url load as well as suppressing them -
snapshots for both looked the same (so if the blob was used for presentation, we
perhaps snapshot the page before it took affect).

Am thinking that suppressing these blob subresource would be the safer change to
try to merge into M-57 but would like to hear if that is concerning after all
and how to proceeed.

Also thinking that loading the blob subresources might be interesting to pursue
with the new background loader instead of prerenderer.

https://codereview.chromium.org/2661703002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2661703002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_throttle.cc:187: // For
OFFLINE_ORIGIN, simply cancel this invalid request and continue.
On 2017/02/01 01:08:09, pasko wrote:
> nit: s/OFFLINE_ORIGIN/ORIGIN_OFFLINE/

Done.

[dougarnett, 2017-02-07 21:37:51]

Nate, are you able to give this CL a look?
I'm happy to provide more background if needed.

Also, ping Dmitry - can you review concept as well?

[Nate Chapin, 2017-02-07 21:47:56]

On 2017/02/01 01:08:09, pasko wrote:
> Overall this looks harmless to me, so I tend to just LG, glad that it helps
your
> case.
> 
> I am not familiar with data urls and how they work.
> 
> Do data urls always leave blink or only in some cases, or maybe it is racy?
> 
> +japhet@ as expert in blink loading code.

data: urls only leave blink for main resource and XHR-like network requests.
Though, if I understand this patch correctly, data: urls would not be failed,
becasue DoesSubresourceURLHaveValidScheme() considers data: a valid scheme?

> 
> One more potential point of worry: websites are likely not ready to fail
loading
> data urls, so they could end up in a broken state in cache/localstorage.
> Theoretical.
> 
>
https://codereview.chromium.org/2661703002/diff/1/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_resource_throttle.cc (right):
> 
>
https://codereview.chromium.org/2661703002/diff/1/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_resource_throttle.cc:187: // For
> OFFLINE_ORIGIN, simply cancel this invalid request and continue.
> nit: s/OFFLINE_ORIGIN/ORIGIN_OFFLINE/

[Nate Chapin, 2017-02-07 21:49:03]

On 2017/02/07 21:37:51, dougarnett wrote:
> Nate, are you able to give this CL a look?
> I'm happy to provide more background if needed.
> 
> Also, ping Dmitry - can you review concept as well?

LGTM, but I'm not an OWNER out here.

[pasko, 2017-02-07 21:53:06]

lgtm with tests pass

[Dmitry Titov, 2017-02-08 18:59:44]

lgtm, thanks!

[dougarnett, 2017-02-09 16:41:03]

The CQ bit was checked by dougarnett@chromium.org

[commit-bot: I haz the power, 2017-02-09 16:41:45]

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, 2017-02-09 17:28:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486658463634700,
"parent_rev": "178e459f4e385a22b73a4914ca1c290331d8fb0b", "commit_rev":
"f1ba435af9338792a78fc422eb9cd1a51a14aa13"}

[commit-bot: I haz the power, 2017-02-09 17:29:23]

Description was changed from

==========
[Prerender] Ignores resources with unsupported schemes for Offline reqs

For offlining a page with the prerender, this changes the policy when
encountering a resource with an unsupported scheme - instread of failing
the prerender (as is done with other prerender requests), it will only
cancel the unsupported resource and attempt to continue with prerendering.

This has been tested end-end on 5 articles linked from www.foxsports.com
that have a "blob:" subresource and with this change, we are able to
download and offline version of them.

BUG=686192
==========

to

==========
[Prerender] Ignores resources with unsupported schemes for Offline reqs

For offlining a page with the prerender, this changes the policy when
encountering a resource with an unsupported scheme - instread of failing
the prerender (as is done with other prerender requests), it will only
cancel the unsupported resource and attempt to continue with prerendering.

This has been tested end-end on 5 articles linked from www.foxsports.com
that have a "blob:" subresource and with this change, we are able to
download and offline version of them.

BUG=686192

Review-Url: https://codereview.chromium.org/2661703002
Cr-Commit-Position: refs/heads/master@{#449337}
Committed:
https://chromium.googlesource.com/chromium/src/+/f1ba435af9338792a78fc422eb9c...
==========

[commit-bot: I haz the power, 2017-02-09 17:29:26]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f1ba435af9338792a78fc422eb9c...