Hmm, this is odd. The preload should be owned by the ResourceFetcher, so the evict() doesn't really do anything except make the resource not available to other parts of the code. What is its purpose now that WeakMemoryCache exists?

+hiroshige

I'm assuming (but might be wrong) that the purpose here is not to reuse resources from MemoryCache between navigations when "disable cache" is on. However, if that's indeed the case, we'd need to call this evictResources() sooner, before the header-based preloads are triggered.

If that's the only use case (it might not be...) then wouldn't it make sense to just not evict unused preloads? They will be cleared on document detach anyways.

That might work, but would require to get the preloads map from ResourceFetcher and pipe it into MemoryCache so it can take it into account.

Wouldn't it be easier to call the evictResources() logic sooner?

Yeah I'm fine with that but it is a bit more fragile of a solution imo. Hopefully MemoryCache can be refactored to be more preload-aware in the future (as I think it will eventually own preload resources).

Could we just use:
if (resource->isUnusedPreload())
  return

in the resource eviction function?

I guess we could. I'm still not 100% convinced that we should. What would be more fragile about triggering eviction before any resource might have been preloaded?

Generally clearing / evicting resources at set times just seem a bit more dangerous to me. Code evolves and callsites might change slightly, leading to ordering bugs. 

That being said I think in this case your solution is fine, as long as there's documentation for why we changed the ordering (I'm guessing we're just going to move loadLinksFromHeaders after InspectorInstrumentation::didCommitLoad).

https://codereview.chromium.org/2385313002/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f78da02239eef5039ce480e8620d30a136918a74

commit f78da02239eef5039ce480e8620d30a136918a74
Author: yoav <yoav@yoav.ws>
Date: Wed Oct 12 23:43:42 2016

Avoid MemoryCache eviction of preloaded resources with devtools open

Currently, InspectorNetworkAgent::didCommitLoad is evicting all
resources from MemoryCache, including ones that were preloaded
using headers just before that, which results in double downloads
when devtools are open.

This CL fixes that, by making sure that this eviction spares unused
preloads.

BUG= 652187 

Review-Url: https://codereview.chromium.org/2385313002
Cr-Commit-Position: refs/heads/master@{#424907}

[add] https://crrev.com/f78da02239eef5039ce480e8620d30a136918a74/third_party/WebKit/LayoutTests/http/tests/inspector/network/network-disable-cache-preloads-expected.txt
[add] https://crrev.com/f78da02239eef5039ce480e8620d30a136918a74/third_party/WebKit/LayoutTests/http/tests/inspector/network/network-disable-cache-preloads.php
[modify] https://crrev.com/f78da02239eef5039ce480e8620d30a136918a74/third_party/WebKit/Source/core/fetch/MemoryCache.cpp
[modify] https://crrev.com/f78da02239eef5039ce480e8620d30a136918a74/third_party/WebKit/Source/core/fetch/MemoryCache.h
[modify] https://crrev.com/f78da02239eef5039ce480e8620d30a136918a74/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp

Sheriff here. Reopening, as the added test doesn't pass on Win7 (dbg). Given that it's a new test and that it passes everywhere else, I won't revert for now - please try to fix.

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%29/builds/7710

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/50c7b3ce18404e7b0002b0a7a7fc325fb3e0854b

commit 50c7b3ce18404e7b0002b0a7a7fc325fb3e0854b
Author: msramek <msramek@chromium.org>
Date: Thu Oct 13 09:55:33 2016

Disable network-disable-cache-preloads.php on Win7 (dbg)

NOTRY=true
TBR=yoav@yoav.ws
BUG= 652187 

Review-Url: https://codereview.chromium.org/2410863006
Cr-Commit-Position: refs/heads/master@{#424992}

[modify] https://crrev.com/50c7b3ce18404e7b0002b0a7a7fc325fb3e0854b/third_party/WebKit/LayoutTests/TestExpectations

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78b9ba0dd94faec1f861a6e2e2c061fc909fecae

commit 78b9ba0dd94faec1f861a6e2e2c061fc909fecae
Author: yhirano <yhirano@chromium.org>
Date: Fri Dec 09 04:56:01 2016

Mark http/tests/inspector/network/network-disable-cache-preloads.php as flaky

It has been marked as flaky on Win7 Debug but it is flaky on all debug bots.

BUG= 652187 
NOTRY=true
TBR=asargent@chromium.org, grt@chromium.org, vasilii@chromium.org

Review-Url: https://codereview.chromium.org/2565653002
Cr-Commit-Position: refs/heads/master@{#437471}

[modify] https://crrev.com/78b9ba0dd94faec1f861a6e2e2c061fc909fecae/third_party/WebKit/LayoutTests/TestExpectations

Since issue is fixed, but the bug kept open due to flaky test, downgrading priority

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5a1b7255488614f897206baaee7f537b0b99e41d

commit 5a1b7255488614f897206baaee7f537b0b99e41d
Author: yoav <yoav@yoav.ws>
Date: Tue Jan 03 15:08:00 2017

Deflake network-disable-cache-preloads

The test was flaky due to its use of an expected.txt file combined with unused preloads warnings. Moving this test to an HTML expected file should make the flakiness go away.

BUG= 652187 

Review-Url: https://codereview.chromium.org/2608163002
Cr-Commit-Position: refs/heads/master@{#441124}

[modify] https://crrev.com/5a1b7255488614f897206baaee7f537b0b99e41d/third_party/WebKit/LayoutTests/TestExpectations
[add] https://crrev.com/5a1b7255488614f897206baaee7f537b0b99e41d/third_party/WebKit/LayoutTests/http/tests/inspector/network/network-disable-cache-preloads-expected.html
[delete] https://crrev.com/f176e05cf23ff77ceffda2e22caa0a71814bb403/third_party/WebKit/LayoutTests/http/tests/inspector/network/network-disable-cache-preloads-expected.txt
[modify] https://crrev.com/5a1b7255488614f897206baaee7f537b0b99e41d/third_party/WebKit/LayoutTests/http/tests/inspector/network/network-disable-cache-preloads.php