Avoid double download for late discovered link preload resources.

Avoid double download for late discovered link preload resources.

`<link rel=preload>` based resources are could result in a double download
if they weren't discovered by the parser by the time parsing ended. If they
we're not used by that time, they were removed from mem cache entirely.

That behavior is compatible with preloadScanner discovered resources, but
it is not compatible with rel preload, which is intended to be used on
exactly the resources that are not present in markup and therefore will
be discovered late.

This CL fixes the issue, by avoiding to remove from memcache resources
that are there due to `<link rel=preload>`.

BUG=579914
Committed: https://crrev.com/7deab7b0ed91ae7e453919ceef37e1b6074b7c1a
Cr-Commit-Position: refs/heads/master@{#371061}

[Yoav Weiss, 2016-01-21 12:44:04]

yoav@yoav.ws changed reviewers:
+ japhet@chromium.org

[Yoav Weiss, 2016-01-21 12:44:04]

Hey Nate :) We've discussed this one yesterday on irc, but things are slightly
different than what I thought. Apparently Document::finishedParsing calls
clearPreloads, as it assumes that all preloads are a result of markup discovered
by the preloadScanner. <link rel=preload> breaks that assumption, so I cancelled
the removal from cache for that scenario.

[Nate Chapin, 2016-01-21 19:07:14]

https://codereview.chromium.org/1615803004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1615803004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:843: if (!deleted &&
resource->preloadResult() == Resource::PreloadNotReferenced && (type ==
ClearAllPreloads || !resource->avoidBlockingOnLoad()))
I assume avoidBlockingOnLoad() is checked because that will only be set in the
rel=preload case? Maybe add a comment to explain that?

https://codereview.chromium.org/1615803004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right):

https://codereview.chromium.org/1615803004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.h:96: enum
ClearPreloadsType { ClearAllPreloads, ClearSpeculativeMarkupPreloads };
ClearPreloadsPolicy?

https://codereview.chromium.org/1615803004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.h:99: void
clearPreloads(ClearPreloadsType = ClearAllPreloads);
How many callsites are there for clearPreloads()? It might be good to avoid a
default argument.

[Yoav Weiss, 2016-01-21 20:34:09]

https://codereview.chromium.org/1615803004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1615803004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:843: if (!deleted &&
resource->preloadResult() == Resource::PreloadNotReferenced && (type ==
ClearAllPreloads || !resource->avoidBlockingOnLoad()))
On 2016/01/21 19:07:14, Nate Chapin wrote:
> I assume avoidBlockingOnLoad() is checked because that will only be set in the
> rel=preload case? Maybe add a comment to explain that?

added

https://codereview.chromium.org/1615803004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right):

https://codereview.chromium.org/1615803004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.h:96: enum
ClearPreloadsType { ClearAllPreloads, ClearSpeculativeMarkupPreloads };
On 2016/01/21 19:07:14, Nate Chapin wrote:
> ClearPreloadsPolicy?

yup

https://codereview.chromium.org/1615803004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.h:99: void
clearPreloads(ClearPreloadsType = ClearAllPreloads);
On 2016/01/21 19:07:14, Nate Chapin wrote:
> How many callsites are there for clearPreloads()? It might be good to avoid a
> default argument.

Yeah, but the preFinalizer is taking this as a method, and there's no current
way to pass it arguments.

[Nate Chapin, 2016-01-22 17:38:49]

lgtm

[Yoav Weiss, 2016-01-22 21:40:39]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2016-01-22 21:42:20]

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

[commit-bot: I haz the power, 2016-01-22 22:30:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-22 22:31:43]

Description was changed from

==========
Avoid double download for late discovered link preload resources.

`<link rel=preload>` based resources are could result in a double download
if they weren't discovered by the parser by the time parsing ended. If they
we're not used by that time, they were removed from mem cache entirely.

That behavior is compatible with preloadScanner discovered resources, but
it is not compatible with rel preload, which is intended to be used on
exactly the resources that are not present in markup and therefore will
be discovered late.

This CL fixes the issue, by avoiding to remove from memcache resources
that are there due to `<link rel=preload>`.

BUG=579914
==========

to

==========
Avoid double download for late discovered link preload resources.

`<link rel=preload>` based resources are could result in a double download
if they weren't discovered by the parser by the time parsing ended. If they
we're not used by that time, they were removed from mem cache entirely.

That behavior is compatible with preloadScanner discovered resources, but
it is not compatible with rel preload, which is intended to be used on
exactly the resources that are not present in markup and therefore will
be discovered late.

This CL fixes the issue, by avoiding to remove from memcache resources
that are there due to `<link rel=preload>`.

BUG=579914

Committed: https://crrev.com/7deab7b0ed91ae7e453919ceef37e1b6074b7c1a
Cr-Commit-Position: refs/heads/master@{#371061}
==========

[commit-bot: I haz the power, 2016-01-22 22:31:43]

Patchset 5 (id:??) landed as
https://crrev.com/7deab7b0ed91ae7e453919ceef37e1b6074b7c1a
Cr-Commit-Position: refs/heads/master@{#371061}