Bug 204147

Created attachment 384145 [details] Patch

Created attachment 384361 [details] Patch

Created attachment 384367 [details] Patch

Created attachment 384705 [details] Patch

Created attachment 384711 [details] Patch

Comment on attachment 384711 [details] Patch This still needs a test but I think it can be reviewed to check if it is the right direction. Some remarks: - I picked stopAllLoaders to catch both page close and starting a new navigation within a page. - I do not like adding another HashMap but I think it is hard to avoid. - The cleanup only gets triggered for main frames.

Comment on attachment 384711 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384711&action=review > Source/WebCore/loader/FrameLoader.cpp:1844 > + platformStrategies()->loaderStrategy()->pageContextRemoved(*m_frame.page()); We might actually want to send the message for all contexts since we want to cancel the revalidation when a context is going away and not when its top level context is going away. We may thus need to send pageID/frameID and rename pageContextRemoved. > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:339 > + m_pendingAsyncRevalidationByPage.remove(webPageID); I do not see here where we are cancelling the async revalidation of the page. > Source/WebKit/NetworkProcess/cache/NetworkCache.h:204 > + HashMap<WebCore::PageIdentifier, std::unique_ptr<Vector<Key>>> m_pendingAsyncRevalidationByPage; Why is it a unique_ptr<Vector> and not a Vector?

Created attachment 385586 [details] Patch

Comment on attachment 384711 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384711&action=review I started on the test but maybe you can give some clues, it seems really hard to verify that the right thing is done. Even internal testing API seems difficult since we need to verify things in the network process? >> Source/WebCore/loader/FrameLoader.cpp:1844 >> + platformStrategies()->loaderStrategy()->pageContextRemoved(*m_frame.page()); > > We might actually want to send the message for all contexts since we want to cancel the revalidation when a context is going away and not when its top level context is going away. > We may thus need to send pageID/frameID and rename pageContextRemoved. Ok, I initially did it this way to reduce the IPC since I was not sure how intrusive it would be. I now renamed it to browserContextRemoved. >> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:339 >> + m_pendingAsyncRevalidationByPage.remove(webPageID); > > I do not see here where we are cancelling the async revalidation of the page. Oops you are right, should be fixed now. >> Source/WebKit/NetworkProcess/cache/NetworkCache.h:204 >> + HashMap<WebCore::PageIdentifier, std::unique_ptr<Vector<Key>>> m_pendingAsyncRevalidationByPage; > > Why is it a unique_ptr<Vector> and not a Vector? No real reason, now Use just Vector.

Comment on attachment 385586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385586&action=review > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:341 > + m_pendingAsyncRevalidations.remove(key); It seems we take all keys of the page while we should only take the keys that match the frame ID and remove the corresponding revalidations.

(In reply to Rob Buis from comment #9) > Comment on attachment 384711 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=384711&action=review > > I started on the test but maybe you can give some clues, it seems really > hard to verify that the right thing is done. Even internal testing API seems > difficult since we need to verify things in the network process? One possibility would be to write a python script that is hanging for 0.5 seconds in case this is a stale revalidation. We could first load an iframe to get the resource. We could then load a new iframe to get the same resource, it should trigger revalidation. Revalidation will be hanging. We could then remove the iframe, this should remove the ongoing revalidation. We could then load a new iframe to get the same resource, it should probably get the cached resource and trigger a revalidation instead of reusing the cancelled revalidation (that would still be hanging without the patch). It might be good to load in parallel another iframe with a different URL that triggers revalidation so that we can check that one frame revalidation is cancelled but not all.

Created attachment 386844 [details] Patch

Created attachment 387221 [details] Patch

Created attachment 387317 [details] Patch

(In reply to youenn fablet from comment #11) > (In reply to Rob Buis from comment #9) > > Comment on attachment 384711 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=384711&action=review > > > > I started on the test but maybe you can give some clues, it seems really > > hard to verify that the right thing is done. Even internal testing API seems > > difficult since we need to verify things in the network process? > > One possibility would be to write a python script that is hanging for 0.5 > seconds in case this is a stale revalidation. It seems not so easy to detect stale revalidations. One way I found that worked is responding with ETag and then the stale revalidation request will contain If-None-Match. However the memory cache can also send out If-None-Match so maybe we need to skip memory cache for s-w-r responses first: https://bugs.webkit.org/show_bug.cgi?id=205992 > We could first load an iframe to get the resource. > We could then load a new iframe to get the same resource, it should trigger > revalidation. Revalidation will be hanging. > We could then remove the iframe, this should remove the ongoing revalidation. > We could then load a new iframe to get the same resource, it should probably > get the cached resource and trigger a revalidation instead of reusing the > cancelled revalidation (that would still be hanging without the patch). I tried to add that test case to this patch. Although I think the memory cache issue maybe needs to be fixed first (https://bugs.webkit.org/show_bug.cgi?id=205992), I think this patch can be reviewed again.

Comment on attachment 387317 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387317&action=review > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:329 > + keys.removeFirst(key); keys will probably be a new vector copied from m_pendingAsyncRevalidationByPage owned vector. So removing might not work here. Can you add some ASSERTS as well so that we are sure 'keys' contains key for instance. Also, why not simplify things and have something like a Map<GlobalID, WeakHashSet<AsyncRevalidation>>. This would allow to write something like: auto loaders = m_pendingAsyncRevalidationByPage.take(globalID); for (auto& loader : loaders) loader->cancel(); > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:340 > + for (auto key : keys) { s/auto/auto&/ > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:342 > + if (revalidation) if (auto revalidation = ...) Should we use take instead of get here as well, or cancel will do it automatically?

Created attachment 388529 [details] Patch

Created attachment 388536 [details] Patch

Created attachment 388540 [details] Patch

Created attachment 388659 [details] Patch

Comment on attachment 387317 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387317&action=review >> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:329 >> + keys.removeFirst(key); > > keys will probably be a new vector copied from m_pendingAsyncRevalidationByPage owned vector. > So removing might not work here. > Can you add some ASSERTS as well so that we are sure 'keys' contains key for instance. > > Also, why not simplify things and have something like a Map<GlobalID, WeakHashSet<AsyncRevalidation>>. > This would allow to write something like: > auto loaders = m_pendingAsyncRevalidationByPage.take(globalID); > for (auto& loader : loaders) > loader->cancel(); Great idea, things get simpler when using WeakHashSet. >> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:340 >> + for (auto key : keys) { > > s/auto/auto&/ Done. >> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:342 >> + if (revalidation) > > if (auto revalidation = ...) > Should we use take instead of get here as well, or cancel will do it automatically? I changed the logic so cancel will do it automatically by calling the completion handler.

Comment on attachment 388659 [details] Patch Seems almost ready to me, I have just one question with regards to the new CachedResourceLoader check. View in context: https://bugs.webkit.org/attachment.cgi?id=388659&action=review > Source/WebCore/loader/cache/CachedResourceLoader.cpp:1173 > + if (existingResource->response().cacheControlStaleWhileRevalidate()) { The comment does not match the check, which is about checking for cache control stale while revalidate. Ditto for the message. Also, I am not sure we need this check. It seems that we should reload only if the resource is expired, otherwise we should be able to use it. Can you clarify? > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:325 > + auto revalidation = makeUnique<AsyncRevalidation>(*this, frameID, request, WTFMove(entry), [this, key](AsyncRevalidation::Result result) { s/AsyncRevalidation::Result/auto > Source/WebKit/NetworkProcess/cache/NetworkCache.h:236 > + HashMap<GlobalFrameID, WTF::WeakHashSet<AsyncRevalidation>> m_pendingAsyncRevalidationByPage; Do we need WTF::?

Created attachment 388838 [details] Patch

Created attachment 388839 [details] Patch

Comment on attachment 388659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388659&action=review >> Source/WebCore/loader/cache/CachedResourceLoader.cpp:1173 >> + if (existingResource->response().cacheControlStaleWhileRevalidate()) { > > The comment does not match the check, which is about checking for cache control stale while revalidate. > Ditto for the message. > Also, I am not sure we need this check. > It seems that we should reload only if the resource is expired, otherwise we should be able to use it. > Can you clarify? Sorry, that was a copy pasta mistake. The problem is that frame-removal.htmlt needs to be able to identify stale revalidations to check that the cancel is performed. The standard revalidate for the memory cache sends the same headers, so I propose for stale memory resources with s-w-r headers we leave it up to the network process to handle the revalidate process, and then the test will work fine, but will fail if the revalidation cancel is not done, as the test should. >> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:325 >> + auto revalidation = makeUnique<AsyncRevalidation>(*this, frameID, request, WTFMove(entry), [this, key](AsyncRevalidation::Result result) { > > s/AsyncRevalidation::Result/auto Done. >> Source/WebKit/NetworkProcess/cache/NetworkCache.h:236 >> + HashMap<GlobalFrameID, WTF::WeakHashSet<AsyncRevalidation>> m_pendingAsyncRevalidationByPage; > > Do we need WTF::? No, fixed.

Comment on attachment 388839 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388839&action=review > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:341 > + fprintf(stderr, "browsingContextRemoved cancel!!!\n"); Remove fprintf. > Source/WebKit/NetworkProcess/cache/NetworkCache.h:76 > +}; We normally do not add ; for namespaces. We could add // NetworkCache comments as well.

Created attachment 390061 [details] Patch

Comment on attachment 388839 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388839&action=review >> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:341 >> + fprintf(stderr, "browsingContextRemoved cancel!!!\n"); > > Remove fprintf. Ouch! Maybe there should be a pre upload warning hook to protect me :) Fixed. >> Source/WebKit/NetworkProcess/cache/NetworkCache.h:76 >> +}; > > We normally do not add ; for namespaces. > We could add // NetworkCache comments as well. Sure, done.

Comment on attachment 390061 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390061&action=review > Source/WebKit/NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp:72 > + if (m_networkLoad) { We could also write it if (!m_networkLoad) return;

Created attachment 391530 [details] Patch

Comment on attachment 391530 [details] Patch Clearing flags on attachment: 391530 Committed r257206: <https://trac.webkit.org/changeset/257206>

All reviewed patches have been landed. Closing bug.

<rdar://problem/59725101>

Comment on attachment 391530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391530&action=review > Source/WebCore/ChangeLog:11 > + Test: imported/w3c/web-platform-tests/fetch/stale-while-revalidate/frame-removal.html Has this test been merged into WPT? I can't see it on wpt/master. Is there any PR open for it?

(In reply to Carlos Alberto Lopez Perez from comment #34) > Comment on attachment 391530 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391530&action=review > > > Source/WebCore/ChangeLog:11 > > + Test: imported/w3c/web-platform-tests/fetch/stale-while-revalidate/frame-removal.html > > Has this test been merged into WPT? I can't see it on wpt/master. Is there > any PR open for it? Good find, I started to try to add it here: https://github.com/web-platform-tests/wpt/pull/22292