When clearing website data store, we should clear the process caches. Otherwise, the privacy sensitive information such as which site the user had visited may linger around. <rdar://problem/48243972>
Created attachment 362587 [details] Fixes the bug
Comment on attachment 362587 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=362587&action=review > Source/WebKit/ChangeLog:8 > + Clear the process cache when clearing the website data store so that Is there any reason we clear the process cache but keep suspended pages/processes?
Comment on attachment 362587 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=362587&action=review > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:2843 > + auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]); It would be better to use the convenience function in this file that returns a process pool configuration. You can always disable prewarming after calling it.
I didn't think about suspended pages because they're tied to the back forward list, and technically the client didn't ask to clear the back forward list. Still, might as well clear suspended pages too.
(In reply to Geoffrey Garen from comment #4) > I didn't think about suspended pages because they're tied to the back > forward list, and technically the client didn't ask to clear the back > forward list. > > Still, might as well clear suspended pages too. If the concern is leaking information via activity monitor, suspended pages actually more obviously leak domains than cached processes.
> If the concern is leaking information via activity monitor, suspended pages > actually more obviously leak domains than cached processes. Let's clear suspended pages too. Marking r- to reflect that request (and patch doesn't apply cleanly?) Clearing suspended pages seems like a reasonable interpretation of a request to clear the memory cache, and it's very unlikely to be a performance problem in practice, since the act of clearing the memory cache is rare. That said, just to explain the situation a bit more: I believe it's technically not an information leak to display a domain in Activity Monitor if the domain is already displayed in a window's back-forward list. What's unique about cached pages is that they reflect domains that are completely gone from any other user-visible presentation.
Created attachment 362650 [details] Clears the suspended pages
Comment on attachment 362650 [details] Clears the suspended pages View in context: https://bugs.webkit.org/attachment.cgi?id=362650&action=review > Source/WebKit/UIProcess/WebProcessCache.cpp:54 > + if (!capacity() || m_disabled) Is this patch based on ToT? Seems like this check should also be in addProcessIfNecessary(), not just addProcess(). > Source/WebKit/UIProcess/WebProcessCache.h:52 > + bool disabled() const { return m_disabled; } isDisabled() and m_isDisabled as per coding style. > Source/WebKit/UIProcess/WebProcessCache.h:53 > + void setDisabled(bool disabled) { m_disabled = disabled; } setIsDisabled(). > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:783 > + ASSERT(!processPool->webProcessCache().disabled()); Wouldn't it be pretty easy to trigger this assertion by clearing website data twice in a row, without returning to the run loop? > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:787 > + WTF::RunLoop::main().dispatch([pool = makeRef(*processPool)] { WTF:: should not be needed. Also, it seems like this should capture the callbackAggregator since we do not want to claim we're done until we've cleared the WebProcess cache.
(In reply to Chris Dumez from comment #8) > Comment on attachment 362650 [details] > Clears the suspended pages > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362650&action=review > > > Source/WebKit/UIProcess/WebProcessCache.cpp:54 > > + if (!capacity() || m_disabled) > > Is this patch based on ToT? Seems like this check should also be in > addProcessIfNecessary(), not just addProcess(). Oh, I guess my checkout was old. Will fix. > > Source/WebKit/UIProcess/WebProcessCache.h:52 > > + bool disabled() const { return m_disabled; } > > isDisabled() and m_isDisabled as per coding style. Good catch. Will fix. > > Source/WebKit/UIProcess/WebProcessCache.h:53 > > + void setDisabled(bool disabled) { m_disabled = disabled; } > > setIsDisabled(). Ditto. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:783 > > + ASSERT(!processPool->webProcessCache().disabled()); > > Wouldn't it be pretty easy to trigger this assertion by clearing website > data twice in a row, without returning to the run loop? > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:787 > > + WTF::RunLoop::main().dispatch([pool = makeRef(*processPool)] { > > WTF:: should not be needed. > Also, it seems like this should capture the callbackAggregator since we do > not want to claim we're done until we've cleared the WebProcess cache. Hm... good point. I guess I'm gonna just restore the original value.
Comment on attachment 362650 [details] Clears the suspended pages View in context: https://bugs.webkit.org/attachment.cgi?id=362650&action=review >>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:783 >>> + ASSERT(!processPool->webProcessCache().disabled()); >> >> Wouldn't it be pretty easy to trigger this assertion by clearing website data twice in a row, without returning to the run loop? > > Hm... good point. I guess I'm gonna just restore the original value. Alternatively, it could be a "isClearingWebsiteData" flag on the WebProcessPool that the WebProcessCache would check.
Created attachment 362668 [details] Fixes the bug
Comment on attachment 362668 [details] Fixes the bug r=me
Committed r241928: <https://trac.webkit.org/changeset/241928>
Comment on attachment 362668 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=362668&action=review > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:785 > + // FIXME: We need to delay the clearing of the process cache because ~SuspendedPageProxy() calls maybeShutDown asynchronously. This is pretty weird. :(
(In reply to Geoffrey Garen from comment #14) > Comment on attachment 362668 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362668&action=review > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:785 > > + // FIXME: We need to delay the clearing of the process cache because ~SuspendedPageProxy() calls maybeShutDown asynchronously. > > This is pretty weird. :( Yes, I will clean this up on trunk in a follow up.
The new API test ProcessSwap.NumberOfCachedProcesses added in https://trac.webkit.org/changeset/241928/webkit is failing on iOS. Build: https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/2751
(In reply to Truitt Savell from comment #16) > The new API test ProcessSwap.NumberOfCachedProcesses > > added in https://trac.webkit.org/changeset/241928/webkit > > is failing on iOS. > > Build: > https://build.webkit.org/builders/ > Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/2751 Process cache is disabled on devices with less than 3GB of RAM, which likely includes the iOS simulator. We should probably disable the new test on iOS.
(In reply to Chris Dumez from comment #17) > (In reply to Truitt Savell from comment #16) > > The new API test ProcessSwap.NumberOfCachedProcesses > > > > added in https://trac.webkit.org/changeset/241928/webkit > > > > is failing on iOS. > > > > Build: > > https://build.webkit.org/builders/ > > Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/2751 > > Process cache is disabled on devices with less than 3GB of RAM, which likely > includes the iOS simulator. We should probably disable the new test on iOS. Done in <https://trac.webkit.org/changeset/241948>.