Clear all preloads when document is detached

Clear all preloads when document is detached

Currently link rel preload based preloads are never evicted from MemoryCache. This fixes that
by evicting all preloads when the document is detached.

This also fixes issues found when Link preloads were not reused after DCL due to bugs inside clearPreloads().

BUG=627026
Committed: https://crrev.com/57696ed320e6ed0263b7c447ef17bb6bef5236ca
Cr-Commit-Position: refs/heads/master@{#410008}

[Yoav Weiss, 2016-07-22 10:34:50]

Description was changed from

==========
Clear all preloads when document is torn down

Currently link rel preload based preloads are never evicted from MemoryCache.
This fixes that
by evicting all preloads when the document is detructed.

BUG=627026
==========

to

==========
Clear all preloads when document is detached

Currently link rel preload based preloads are never evicted from MemoryCache.
This fixes that
by evicting all preloads when the document is detached.

BUG=627026
==========

[Yoav Weiss, 2016-07-22 10:34:50]

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

[Yoav Weiss, 2016-07-22 10:36:48]

Hey csharrison! :)

I still need to add tests here, but can you take a look at the approach and let
me know what you think?

I know you discussed adding a clearLinkRelPreloads method to MemoryCache, but it
seems like the infrastructure for end of nav eviction is already there in
ResourceFetcher.

Cheers,
Yoav

[Charlie Harrison, 2016-07-22 12:00:34]

hmm for some reason I thought that ResourceFetcher was not holding strong
references to preloads loaded after parse. Looking at the code I think I'm wrong
though. This approach is way simpler and looks good!

[Charlie Harrison, 2016-07-22 12:00:36]

hmm for some reason I thought that ResourceFetcher was not holding strong
references to preloads loaded after parse. Looking at the code I think I'm wrong
though. This approach is way simpler and looks good!

[Yoav Weiss, 2016-08-04 05:15:54]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 05:16:14]

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

[Yoav Weiss, 2016-08-04 05:17:55]

Description was changed from

==========
Clear all preloads when document is detached

Currently link rel preload based preloads are never evicted from MemoryCache.
This fixes that
by evicting all preloads when the document is detached.

BUG=627026
==========

to

==========
Clear all preloads when document is detached

Currently link rel preload based preloads are never evicted from MemoryCache.
This fixes that
by evicting all preloads when the document is detached.

This also fixes issues found when Link preloads were not reused after DCL due to
bugs inside clearPreloads().

BUG=627026
==========

[Yoav Weiss, 2016-08-04 05:19:15]

On 2016/07/22 12:00:36, csharrison wrote:
> hmm for some reason I thought that ResourceFetcher was not holding strong
> references to preloads loaded after parse. Looking at the code I think I'm
wrong
> though. This approach is way simpler and looks good!

Finally found time (on a flight) to add tests and fix related issues that came
up while debugging the tests. PTAL?

[commit-bot: I haz the power, 2016-08-04 06:25:12]

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

[commit-bot: I haz the power, 2016-08-04 06:25:12]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-08-04 18:10:26]

Tests look great. Thanks!

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/preload/memcache_eviction_nopreload.html
(right):

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/preload/memcache_eviction_nopreload.html:6:
var t = async_test('Test that preloaded resources get evicted from MemoryCache
when preload is not used');
Update text here (no preloads).

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/preload/no_cache_reuse.html
(right):

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/preload/no_cache_reuse.html:6: var t =
async_test('Test that preloaded non-cacheable resources get reused even after
onload');
Seems like the file name should be "cache_reuse" rather than "no_cache_reuse"?

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2229:
m_fetcher->clearPreloads(ResourceFetcher::ClearAllPreloads);
tentative: can we do this in clearContext()?

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

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:897: if
(!m_preloads->size())
This is confusing. Clear the preloads only if they are already empty?

[Yoav Weiss, 2016-08-04 20:25:00]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 20:25:22]

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

[Yoav Weiss, 2016-08-04 20:26:14]

Thanks for reviewing! :)

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/preload/memcache_eviction_nopreload.html
(right):

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/preload/memcache_eviction_nopreload.html:6:
var t = async_test('Test that preloaded resources get evicted from MemoryCache
when preload is not used');
On 2016/08/04 18:10:25, csharrison wrote:
> Update text here (no preloads).

done

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/preload/no_cache_reuse.html
(right):

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/preload/no_cache_reuse.html:6: var t =
async_test('Test that preloaded non-cacheable resources get reused even after
onload');
On 2016/08/04 18:10:25, csharrison wrote:
> Seems like the file name should be "cache_reuse" rather than "no_cache_reuse"?

renamed to memcache_reuse_of_non_cacheable_preload

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2229:
m_fetcher->clearPreloads(ResourceFetcher::ClearAllPreloads);
On 2016/08/04 18:10:26, csharrison wrote:
> tentative: can we do this in clearContext()?

sure. moved

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

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:897: if
(!m_preloads->size())
On 2016/08/04 18:10:26, csharrison wrote:
> This is confusing. Clear the preloads only if they are already empty?

AFAICT the clear here clears the Member (and enables the GC of the HashSet), not
necessarily the resources inside it. Not sure it's absolutely necessary, but I
preferred not to change that, while maintaining correctness (as we don't want
m_preloads to be cleared if there are still preloads in it)

[Charlie Harrison, 2016-08-04 20:59:57]

non-owner lgtm

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

https://codereview.chromium.org/2174563003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:897: if
(!m_preloads->size())
On 2016/08/04 20:26:14, Yoav Weiss wrote:
> On 2016/08/04 18:10:26, csharrison wrote:
> > This is confusing. Clear the preloads only if they are already empty?
> 
> AFAICT the clear here clears the Member (and enables the GC of the HashSet),
not
> necessarily the resources inside it. Not sure it's absolutely necessary, but I
> preferred not to change that, while maintaining correctness (as we don't want
> m_preloads to be cleared if there are still preloads in it)

Ah, makes sense. Thanks for the clarification.

[commit-bot: I haz the power, 2016-08-04 22:26:21]

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

[commit-bot: I haz the power, 2016-08-04 22:26:22]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Yoav Weiss, 2016-08-05 06:54:17]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2016-08-05 06:54:29]

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-08-05 07:04:00]

Description was changed from

==========
Clear all preloads when document is detached

Currently link rel preload based preloads are never evicted from MemoryCache.
This fixes that
by evicting all preloads when the document is detached.

This also fixes issues found when Link preloads were not reused after DCL due to
bugs inside clearPreloads().

BUG=627026
==========

to

==========
Clear all preloads when document is detached

Currently link rel preload based preloads are never evicted from MemoryCache.
This fixes that
by evicting all preloads when the document is detached.

This also fixes issues found when Link preloads were not reused after DCL due to
bugs inside clearPreloads().

BUG=627026
==========

[commit-bot: I haz the power, 2016-08-05 07:04:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-05 07:06:15]

Description was changed from

==========
Clear all preloads when document is detached

Currently link rel preload based preloads are never evicted from MemoryCache.
This fixes that
by evicting all preloads when the document is detached.

This also fixes issues found when Link preloads were not reused after DCL due to
bugs inside clearPreloads().

BUG=627026
==========

to

==========
Clear all preloads when document is detached

Currently link rel preload based preloads are never evicted from MemoryCache.
This fixes that
by evicting all preloads when the document is detached.

This also fixes issues found when Link preloads were not reused after DCL due to
bugs inside clearPreloads().

BUG=627026

Committed: https://crrev.com/57696ed320e6ed0263b7c447ef17bb6bef5236ca
Cr-Commit-Position: refs/heads/master@{#410008}
==========

[commit-bot: I haz the power, 2016-08-05 07:06:16]

Patchset 4 (id:??) landed as
https://crrev.com/57696ed320e6ed0263b7c447ef17bb6bef5236ca
Cr-Commit-Position: refs/heads/master@{#410008}