Bug 201461 - Support stale-while-revalidate cache strategy
Summary: Support stale-while-revalidate cache strategy
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on: 204148 204147 204169
Blocks:
  Show dependency treegraph
 
Reported: 2019-09-04 10:12 PDT by Dima Voytenko
Modified: 2019-12-03 06:29 PST (History)
21 users (show)

See Also:


Attachments
Patch (42.52 KB, patch)
2019-09-25 07:12 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (42.29 KB, patch)
2019-09-25 08:08 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (45.03 KB, patch)
2019-09-25 11:22 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (45.06 KB, patch)
2019-10-02 06:28 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (53.28 KB, patch)
2019-10-07 00:57 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (102.39 KB, patch)
2019-10-07 01:59 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (102.64 KB, patch)
2019-10-07 07:22 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (110.75 KB, patch)
2019-10-11 03:58 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (110.80 KB, patch)
2019-10-11 04:29 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (110.91 KB, patch)
2019-10-11 05:03 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (69.00 KB, patch)
2019-10-24 07:31 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (69.84 KB, patch)
2019-10-24 07:48 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (69.49 KB, patch)
2019-10-24 09:19 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (69.91 KB, patch)
2019-10-24 11:26 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (71.94 KB, patch)
2019-11-06 11:10 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (71.64 KB, patch)
2019-11-12 08:25 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (71.54 KB, patch)
2019-11-12 13:31 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (71.52 KB, patch)
2019-11-13 00:03 PST, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dima Voytenko 2019-09-04 10:12:07 PDT
This caching strategy works well with fonts and other resources. Supported by Chrome and Firefox. 

Resources:
- https://httpwg.org/specs/rfc5861.html
- https://web.dev/stale-while-revalidate
- https://www.chromestatus.com/feature/5050913014153216
Comment 1 Rob Buis 2019-09-25 07:12:06 PDT
Created attachment 379546 [details]
Patch
Comment 2 Rob Buis 2019-09-25 08:08:33 PDT
Created attachment 379550 [details]
Patch
Comment 3 Rob Buis 2019-09-25 11:22:04 PDT
Created attachment 379563 [details]
Patch
Comment 4 Rob Buis 2019-10-02 06:28:42 PDT
Created attachment 380014 [details]
Patch
Comment 5 youenn fablet 2019-10-04 00:09:08 PDT
Comment on attachment 380014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380014&action=review

> Source/WebCore/platform/network/CacheValidation.cpp:326
> +                    result.maxStale = Seconds { staleWhileRevalidate };

This means that stale-while-revalidate is overriding max-stale, is that correct?
Do we have tests for this rule?

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:168
> +        maximumStaleness += responseMaxStaleness ? responseMaxStaleness.value() : 0_ms;

Just checking but should it be a max of response max staleness and request staleness directive instead of adding both?
Is the stale-while-revalidate/fetch spec describing this? What do other browsers do?

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:191
> +    if (responseHasExpired(response, timestamp, requestDirectives.maxStale, inResponseStaleness))

Would it make sense to update responseHasExpired to return a UseDecision as well.
That would allow removing inResponseStaleness out parameter.

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:350
> +    m_storage->retrieve(storageKey, priority, [this, request, completionHandler = WTFMove(completionHandler), info = WTFMove(info), storageKey, networkProcess = makeRef(networkProcess()), sessionID = m_sessionID, frameID](auto record, auto timings) mutable {

Is there a guarantee that 'this' is valid in the callback?
Should we protectedThis?

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:367
> +            if (m_speculativeLoadManager) {

I am wondering which rules we would like for enabling/disabling revalidation of these resources.
Do you know what policy other browsers are doing? Are they revalidating speculatively all resources?
One potential issue is that we are doing a load that will never be cancelled, even though the user actually closed the page that triggered this speculative load.
Comment 6 Rob Buis 2019-10-07 00:57:00 PDT
Created attachment 380309 [details]
Patch
Comment 7 Rob Buis 2019-10-07 01:59:53 PDT
Created attachment 380310 [details]
Patch
Comment 8 Rob Buis 2019-10-07 07:22:10 PDT
Created attachment 380325 [details]
Patch
Comment 9 Rob Buis 2019-10-07 13:17:42 PDT
(In reply to youenn fablet from comment #5)
> This means that stale-while-revalidate is overriding max-stale, is that
> correct?
> Do we have tests for this rule?

This was a bit confusing. Since max-stale should be ignored for responses I wanted to be clever and reuse the member variable. However it is still possible to provide max-stale on responses, causing it to interfere with stale-while-revalidate, so I changed it to always store stale-while-revalidate separately if encountered.

> > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:168
> > +        maximumStaleness += responseMaxStaleness ? responseMaxStaleness.value() : 0_ms;
> 
> Just checking but should it be a max of response max staleness and request
> staleness directive instead of adding both?
> Is the stale-while-revalidate/fetch spec describing this? What do other
> browsers do?

Great question, Chrome does not support max-stale AFAIK, but Firefox does:
https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttp.cpp#421
Since it ignores stale-while-revalidate if max-stale is given on the request, I changed the patch to match that behavior.

> > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:191
> > +    if (responseHasExpired(response, timestamp, requestDirectives.maxStale, inResponseStaleness))
> 
> Would it make sense to update responseHasExpired to return a UseDecision as
> well.
> That would allow removing inResponseStaleness out parameter.

Nice! Done.

> > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:350
> > +    m_storage->retrieve(storageKey, priority, [this, request, completionHandler = WTFMove(completionHandler), info = WTFMove(info), storageKey, networkProcess = makeRef(networkProcess()), sessionID = m_sessionID, frameID](auto record, auto timings) mutable {
> 
> Is there a guarantee that 'this' is valid in the callback?
> Should we protectedThis?

Done.

> > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:367
> > +            if (m_speculativeLoadManager) {
> 
> I am wondering which rules we would like for enabling/disabling revalidation of these resources.

I am not sure if we can vary a lot from the specification.

> Do you know what policy other browsers are doing? Are they revalidating speculatively all resources?

Chrome has a design doc and they follow the spec closely. I am not sure why "speculatively" is used here though, the specification specifies a regular revalidation?

> One potential issue is that we are doing a load that will never be
> cancelled, even though the user actually closed the page that triggered this
> speculative load.

The revalidation has a timer associated though. But maybe there is a better/earlier point where it can be cancelled?
Comment 10 youenn fablet 2019-10-10 05:43:09 PDT
Comment on attachment 380325 [details]
Patch

Patch goes in the right direction.
I would use a runtime flag here though, which will allow to enable the feature in WTR at first.

The thing that is probably lacking here is how we handle the pending revalidation loads.
Let's say a page while loading is triggering 100 revalidation loads.
The page is closed and 50 revalidation loads are still pending.
What should we do about them? They might take some resources for a page that is gone.

As per my reading of the fetch spec, we should just cancel these loads that are not finished when the document/context that triggered the load goes away.
This is not entirely clear to me what happens in the case a revalidation is triggered independently by two different contexts.
Initially, we could just implement a basic version where the revalidation only handles the closure of the first context.

Another thing to consider is the case of a revalidation done in two iframes of origin A that are in different top level domains B and C.
We should have tests for that case.

This patch does not need to fix all these things but we need a good model for handling these cases.

View in context: https://bugs.webkit.org/attachment.cgi?id=380325&action=review

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:340
> +class AsyncRevalidation {

Maybe it could be in its own file, like SpeculativeLoad is.

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:343
> +    typedef Function<void (bool)> AsyncRevalidationCompletionHandler;

Should probably be a CompletionHandler.
SpeculativeLoad should probably take a CompletionHandler as a pre-patch.

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:358
> +            m_completionHandler(revalidatedEntry.get());

There is a chance that m_completionHandler be called twice, here and if timer fires.
I guess we should cancel one if the other succeeds.
Comment 11 Rob Buis 2019-10-11 03:58:52 PDT
Created attachment 380743 [details]
Patch
Comment 12 Rob Buis 2019-10-11 04:29:39 PDT
Created attachment 380744 [details]
Patch
Comment 13 Rob Buis 2019-10-11 05:03:09 PDT
Created attachment 380745 [details]
Patch
Comment 14 Rob Buis 2019-10-11 07:01:12 PDT
Comment on attachment 380325 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380325&action=review

>> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:340
>> +class AsyncRevalidation {
> 
> Maybe it could be in its own file, like SpeculativeLoad is.

Done.

>> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:343
>> +    typedef Function<void (bool)> AsyncRevalidationCompletionHandler;
> 
> Should probably be a CompletionHandler.
> SpeculativeLoad should probably take a CompletionHandler as a pre-patch.

Done.

>> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:358
>> +            m_completionHandler(revalidatedEntry.get());
> 
> There is a chance that m_completionHandler be called twice, here and if timer fires.
> I guess we should cancel one if the other succeeds.

Should be done.
Comment 15 Rob Buis 2019-10-11 07:07:56 PDT
(In reply to youenn fablet from comment #10)
> Comment on attachment 380325 [details]
> Patch
> 
> Patch goes in the right direction.
> I would use a runtime flag here though, which will allow to enable the
> feature in WTR at first.

I spent a lot of time on that but it seems hard, because AFAIK the network process has to be handed the flag:
- not sure if NetworkProcess creation parameters is the proper place as the network process will get re-used during webkit test runs.
- adding it to the network load parameters makes it per network load, and needs quite some (ugly) plumbing to get it down to the network cache, not sure if that is worth it.
Possibly I am missing something, but I left it out for now.

> The thing that is probably lacking here is how we handle the pending
> revalidation loads.
> Let's say a page while loading is triggering 100 revalidation loads.
> The page is closed and 50 revalidation loads are still pending.
> What should we do about them? They might take some resources for a page that
> is gone.
>
> As per my reading of the fetch spec, we should just cancel these loads that
> are not finished when the document/context that triggered the load goes away.

This makes sense. I only had a short look so far, but I could not find a mechanism in the network process to detect the document/context went away, only individual network loads. Not sure if it should be part of this patch?

> Another thing to consider is the case of a revalidation done in two iframes
> of origin A that are in different top level domains B and C.
> We should have tests for that case.
> 
> This patch does not need to fix all these things but we need a good model
> for handling these cases.

Agreed.
Comment 16 Rob Buis 2019-10-11 10:36:36 PDT
(In reply to Rob Buis from comment #15)
> I spent a lot of time on that but it seems hard, because AFAIK the network
> process has to be handed the flag:
> - not sure if NetworkProcess creation parameters is the proper place as the
> network process will get re-used during webkit test runs.
> - adding it to the network load parameters makes it per network load, and
> needs quite some (ugly) plumbing to get it down to the network cache, not
> sure if that is worth it.
> Possibly I am missing something, but I left it out for now.

Hmm, I overlooked how AdClickAttributionDebugModeEnabled does it, will try that approach tomorrow.
Comment 17 Rob Buis 2019-10-24 07:31:03 PDT
Created attachment 381807 [details]
Patch
Comment 18 Rob Buis 2019-10-24 07:48:40 PDT
Created attachment 381809 [details]
Patch
Comment 19 Rob Buis 2019-10-24 09:19:27 PDT
Created attachment 381813 [details]
Patch
Comment 20 Rob Buis 2019-10-24 11:26:45 PDT
Created attachment 381827 [details]
Patch
Comment 21 Rob Buis 2019-10-25 01:18:19 PDT
Comment on attachment 381827 [details]
Patch

This patch uses TestRunner API to allow enabling stale-while-revalidate behavior in the NetworkProcess. I did not find a way to do a similar thing using a runtime flag, if anybody knows an existing way or a new way to do it, let me know...

It seems a build flag will remain needed since AsyncRevalidation relies on SpeculativeLoad. I went with NETWORK_CACHE_SPECULATIVE_REVALIDATION this time because it is less intrusive.
Comment 22 Rob Buis 2019-11-03 15:22:01 PST
After talking with Chris and Alex at the contributor meeting, it seems using RuntimeEnabledFeatures flag in the NetworkProcess is not the way to go. I hope the approach in the current patch is acceptable, note however that a downside is having to change the wpt tests. All the solutions I can think of have this problem though, maybe I am missing something?
Comment 23 Alex Christensen 2019-11-05 05:57:13 PST
Comment on attachment 381827 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381827&action=review

> Source/WebKit/UIProcess/API/C/WKContext.h:142
> +WK_EXPORT void WKContextSetStaleWhileRevalidateEnabled(WKContextRef, bool enabled);

We should not be increasing the call sites where WKContextRef means global in the NetworkProcess.  I think we should instead have this boolean per-WebsiteDataStore.
Also, what if it instead is an off-by-default setting that is always on in WebKitTestRunner?  That way we wouldn't have to modify the WebPlatformTests.  Since there's nothing else that uses the header, there should be no problems created by other tests.
Comment 24 Rob Buis 2019-11-06 11:10:17 PST
Created attachment 382942 [details]
Patch
Comment 25 Rob Buis 2019-11-06 14:36:46 PST
Comment on attachment 381827 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381827&action=review

>> Source/WebKit/UIProcess/API/C/WKContext.h:142
>> +WK_EXPORT void WKContextSetStaleWhileRevalidateEnabled(WKContextRef, bool enabled);
> 
> We should not be increasing the call sites where WKContextRef means global in the NetworkProcess.  I think we should instead have this boolean per-WebsiteDataStore.
> Also, what if it instead is an off-by-default setting that is always on in WebKitTestRunner?  That way we wouldn't have to modify the WebPlatformTests.  Since there's nothing else that uses the header, there should be no problems created by other tests.

How about on WKWebsiteDataStoreConfiguration, like NetworkCacheSpeculativeValidation? And yes I agree that turning it on for WebKitTestRunner only makes sense for now.
Comment 26 youenn fablet 2019-11-12 07:02:18 PST
Comment on attachment 382942 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382942&action=review

> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:43
> +    if (entry) {

I would think entry should not be null.
Can we pass an Entry& instead?

> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:44
> +        String eTag = entry->response().httpHeaderField(WebCore::HTTPHeaderName::ETag);

s/auto/String/

> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:48
> +        String lastModified = entry->response().httpHeaderField(WebCore::HTTPHeaderName::LastModified);

Ditto.

> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:62
> +        m_completionHandler = nullptr;

No need for the nullptr line, completionHandler is cleaning itself at call time.

> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:66
> +AsyncRevalidation::AsyncRevalidation(Cache& cache, const GlobalFrameID& frameID, const WebCore::ResourceRequest& request, std::unique_ptr<NetworkCache::Entry> entry, CompletionHandler<void(bool)>&& handler)

I am not very clear about the handler bool value.
It is true if the timer is triggered or if the SpeculativeLoad provides an Entry.
Maybe we should use an enumeration instead to document the intent.

> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:76
> +    m_timer.startOneShot(*responseMaxStaleness - (age - lifetime));

Would read better as *responseMaxStaleness + (lifetime - age)

> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:77
> +    m_load = makeUnique<SpeculativeLoad>(cache, frameID, revalidationRequest, WTFMove(entry), [this, key, revalidationRequest](std::unique_ptr<Entry> revalidatedEntry) {

s/std::unique_ptr<Entry>/auot&&/
s/revalidationRequest/revalidationRequest = WTFMove(revalidationRequest)/

> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:81
> +            m_completionHandler(revalidatedEntry.get());

!!revalidatedEntry.

> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:82
> +            m_completionHandler = nullptr;

No need for this line.

> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.h:33
> +#include <WebCore/ResourceRequest.h>

Can we forward declare ResourceRequest?

> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.h:45
> +    AsyncRevalidation(Cache&, const GlobalFrameID&, const WebCore::ResourceRequest&, std::unique_ptr<NetworkCache::Entry>, CompletionHandler<void(bool)>&&);

std::unique_ptr<NetworkCache::Entry>&&

> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.h:49
> +    void staleWhileRevalidateEnding();

Can we make it private?

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:269
> +        Seconds allowedStale;

allowedState might not be initialised, probably want to set it to 0_ms.

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:324
> +void Cache::startAsyncRevalidationIfNeeded(const WebCore::ResourceRequest& request, const NetworkCache::Key& key, std::unique_ptr<Entry> entry, const GlobalFrameID& frameID)

std::unique_ptr<Entry>&&

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:336
> +    m_pendingAsyncRevalidations.add(key, WTFMove(revalidator));

Seems odd that there is a case where we add key but there is already a key in the map in case canUsePendingRevalidation is returning false.
Is the goal to override the key value? Should we use m_pendingAsyncRevalidations.set?

It seems to me that we should only have one entry in the cache.
I would tend to return early even though canUsePendingRevalidation returns false.
Comment 27 Rob Buis 2019-11-12 08:25:53 PST
Created attachment 383356 [details]
Patch
Comment 28 Rob Buis 2019-11-12 11:27:40 PST
Comment on attachment 382942 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382942&action=review

>> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:43
>> +    if (entry) {
> 
> I would think entry should not be null.
> Can we pass an Entry& instead?

Done.

>> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:44
>> +        String eTag = entry->response().httpHeaderField(WebCore::HTTPHeaderName::ETag);
> 
> s/auto/String/

Done.

>> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:48
>> +        String lastModified = entry->response().httpHeaderField(WebCore::HTTPHeaderName::LastModified);
> 
> Ditto.

Done.

>> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:62
>> +        m_completionHandler = nullptr;
> 
> No need for the nullptr line, completionHandler is cleaning itself at call time.

Nice, fixed.

>> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:66
>> +AsyncRevalidation::AsyncRevalidation(Cache& cache, const GlobalFrameID& frameID, const WebCore::ResourceRequest& request, std::unique_ptr<NetworkCache::Entry> entry, CompletionHandler<void(bool)>&& handler)
> 
> I am not very clear about the handler bool value.
> It is true if the timer is triggered or if the SpeculativeLoad provides an Entry.
> Maybe we should use an enumeration instead to document the intent.

Good idea, I added an enumeration.

>> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:76
>> +    m_timer.startOneShot(*responseMaxStaleness - (age - lifetime));
> 
> Would read better as *responseMaxStaleness + (lifetime - age)

Fixed.

>> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:77
>> +    m_load = makeUnique<SpeculativeLoad>(cache, frameID, revalidationRequest, WTFMove(entry), [this, key, revalidationRequest](std::unique_ptr<Entry> revalidatedEntry) {
> 
> s/std::unique_ptr<Entry>/auot&&/
> s/revalidationRequest/revalidationRequest = WTFMove(revalidationRequest)/

Done.

>> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:81
>> +            m_completionHandler(revalidatedEntry.get());
> 
> !!revalidatedEntry.

Fixed but using the enum.

>> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:82
>> +            m_completionHandler = nullptr;
> 
> No need for this line.

Fixed.

>> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.h:33
>> +#include <WebCore/ResourceRequest.h>
> 
> Can we forward declare ResourceRequest?

Done.

>> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.h:45
>> +    AsyncRevalidation(Cache&, const GlobalFrameID&, const WebCore::ResourceRequest&, std::unique_ptr<NetworkCache::Entry>, CompletionHandler<void(bool)>&&);
> 
> std::unique_ptr<NetworkCache::Entry>&&

Done.

>> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.h:49
>> +    void staleWhileRevalidateEnding();
> 
> Can we make it private?

Done.

>> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:269
>> +        Seconds allowedStale;
> 
> allowedState might not be initialised, probably want to set it to 0_ms.

I think the default is 0, but to make things clear I did use 0_ms.

>> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:324
>> +void Cache::startAsyncRevalidationIfNeeded(const WebCore::ResourceRequest& request, const NetworkCache::Key& key, std::unique_ptr<Entry> entry, const GlobalFrameID& frameID)
> 
> std::unique_ptr<Entry>&&

Done.

>> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:336
>> +    m_pendingAsyncRevalidations.add(key, WTFMove(revalidator));
> 
> Seems odd that there is a case where we add key but there is already a key in the map in case canUsePendingRevalidation is returning false.
> Is the goal to override the key value? Should we use m_pendingAsyncRevalidations.set?
> 
> It seems to me that we should only have one entry in the cache.
> I would tend to return early even though canUsePendingRevalidation returns false.

The code was inspired by SpeculativeLoadManager code. I changed it to do an early return when there is already an entry and removed canUsePendingRevalidation.
Comment 29 youenn fablet 2019-11-12 12:59:18 PST
Comment on attachment 383356 [details]
Patch

Seems almost ready to go to me.

View in context: https://bugs.webkit.org/attachment.cgi?id=383356&action=review

> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:48
> +    String lastModified = entry.response().httpHeaderField(WebCore::HTTPHeaderName::LastModified);

auto then.

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:321
> +    if (m_pendingAsyncRevalidations.get(key))

s/get/contains/, but instead, use m_pendingAsyncRevalidations.ensure(key, ...) so that there is only one find.

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:326
> +            m_pendingAsyncRevalidations.take(key);

Should we not always remove the key, no matter whether failing or not?

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:389
> +            std::unique_ptr<Entry> entryCopy = makeUnique<Entry>(*entry);

auto
Comment 30 Rob Buis 2019-11-12 13:31:11 PST
Created attachment 383373 [details]
Patch
Comment 31 Rob Buis 2019-11-12 13:52:14 PST
Comment on attachment 383356 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383356&action=review

>> Source/WebKit/NetworkProcess/cache/AsyncRevalidation.cpp:48
>> +    String lastModified = entry.response().httpHeaderField(WebCore::HTTPHeaderName::LastModified);
> 
> auto then.

Done.

>> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:321
>> +    if (m_pendingAsyncRevalidations.get(key))
> 
> s/get/contains/, but instead, use m_pendingAsyncRevalidations.ensure(key, ...) so that there is only one find.

Nice! Done.

>> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:326
>> +            m_pendingAsyncRevalidations.take(key);
> 
> Should we not always remove the key, no matter whether failing or not?

I think you are right, fixed.

>> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:389
>> +            std::unique_ptr<Entry> entryCopy = makeUnique<Entry>(*entry);
> 
> auto

Done.
Comment 32 youenn fablet 2019-11-12 13:56:31 PST
Comment on attachment 383373 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383373&action=review

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:323
> +            m_pendingAsyncRevalidations.take(key);

s/take/remove

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:325
> +                LOG(NetworkCache, "(NetworkProcess) Async revalidation completed for '%s':", key.identifier().utf8().data());

I would tend to log all cases.
Comment 33 Rob Buis 2019-11-13 00:03:42 PST
Created attachment 383436 [details]
Patch
Comment 34 WebKit Commit Bot 2019-11-13 02:20:50 PST
The commit-queue encountered the following flaky tests while processing attachment 383436 [details]:

inspector/model/remote-object-weak-collection.html bug 202932 (authors: drousso@apple.com and joepeck@webkit.org)
The commit-queue is continuing to process your patch.
Comment 35 WebKit Commit Bot 2019-11-13 02:22:11 PST
Comment on attachment 383436 [details]
Patch

Clearing flags on attachment: 383436

Committed r252397: <https://trac.webkit.org/changeset/252397>
Comment 36 WebKit Commit Bot 2019-11-13 02:22:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 37 Radar WebKit Bug Importer 2019-11-13 02:23:18 PST
<rdar://problem/57148312>
Comment 38 Rob Buis 2019-11-13 07:40:48 PST
I had hoped this would have stayed open, since there are depending bugs. Maybe it is a downside of using cq+..
Comment 39 Simon Fraser (smfr) 2019-12-02 21:51:41 PST
I'm now hitting ASSERT(!m_networkLoad); in ~SpeculativeLoad() every time I debug. Fallout from this change?
Comment 40 Simon Fraser (smfr) 2019-12-02 21:53:22 PST
Comment on attachment 383436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383436&action=review

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:180
> +        LOG(NetworkCache, "(NetworkProcess) needsRevalidation hasExpired age=%f lifetime=%f max-staleness=%f", age, lifetime, maximumStaleness);

For stuff like this LOG_WITH_STREAM() is much nicer; no need to deal with formatting strings.
Comment 41 Rob Buis 2019-12-02 23:48:22 PST
(In reply to Simon Fraser (smfr) from comment #39)
> I'm now hitting ASSERT(!m_networkLoad); in ~SpeculativeLoad() every time I
> debug. Fallout from this change?

The idea was to only turn this on by default when running tests, though probably it can be enabled explicitly by using Developer tools. Or the code is ignoring the runtime flag somewhere. Finally it should only be triggered when the server sends Cache-control: stale-while-revalidate.
If you can give me some steps to reproduce I can have a look.
Comment 42 youenn fablet 2019-12-02 23:58:31 PST
I do not think the debug assertion failure is due to stale-while-revalidate but maybe the patch made a change impacting speculative revalidation.

The debug assertion is probably triggered in SpeculativeLoadManager::revalidateSubresource. There, we are adding a load to m_pendingPreloads and maybe m_pendingPreloads is already doing the preload so the newly created revalidator will be deleted right away.
Comment 43 Rob Buis 2019-12-03 00:14:13 PST
(In reply to youenn fablet from comment #42)
> I do not think the debug assertion failure is due to stale-while-revalidate
> but maybe the patch made a change impacting speculative revalidation.

I tried hard to not change the existing speculative revalidation code. Only change to the manager should be moving two methods to another file.

Don't want to point fingers but https://bugs.webkit.org/show_bug.cgi?id=204305 changed more code in NetworkCacheSpeculativeLoadManager.cpp.
Comment 44 Rob Buis 2019-12-03 06:29:23 PST
(In reply to Simon Fraser (smfr) from comment #40)
> Comment on attachment 383436 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383436&action=review
> 
> > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:180
> > +        LOG(NetworkCache, "(NetworkProcess) needsRevalidation hasExpired age=%f lifetime=%f max-staleness=%f", age, lifetime, maximumStaleness);
> 
> For stuff like this LOG_WITH_STREAM() is much nicer; no need to deal with
> formatting strings.

Thanks, does seem nicer indeed, I will try to use it in the future.