Bug 201461

Created attachment 379546 [details] Patch

Created attachment 379550 [details] Patch

Created attachment 379563 [details] Patch

Created attachment 380014 [details] Patch

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.

Created attachment 380309 [details] Patch

Created attachment 380310 [details] Patch

Created attachment 380325 [details] Patch

(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 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.

Created attachment 380743 [details] Patch

Created attachment 380744 [details] Patch

Created attachment 380745 [details] Patch

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.

(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.

(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.

Created attachment 381807 [details] Patch

Created attachment 381809 [details] Patch

Created attachment 381813 [details] Patch

Created attachment 381827 [details] Patch

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.

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 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.

Created attachment 382942 [details] Patch

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 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.

Created attachment 383356 [details] Patch

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 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

Created attachment 383373 [details] Patch

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 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.

Created attachment 383436 [details] Patch

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 on attachment 383436 [details] Patch Clearing flags on attachment: 383436 Committed r252397: <https://trac.webkit.org/changeset/252397>

All reviewed patches have been landed. Closing bug.

<rdar://problem/57148312>

I had hoped this would have stayed open, since there are depending bugs. Maybe it is a downside of using cq+..

I'm now hitting ASSERT(!m_networkLoad); in ~SpeculativeLoad() every time I debug. Fallout from this change?

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.

(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.

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.

(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.

(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.