Bug 194894 – Discard cached processes when clearing website data store

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Ryosuke Niwa]

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>

[Ryosuke Niwa]

Created attachment 362587 [details]
Fixes the bug

[Chris Dumez]

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?

[Chris Dumez]

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.

[Geoffrey Garen]

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.

[Chris Dumez]

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

[Geoffrey Garen]

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

[Ryosuke Niwa]

Created attachment 362650 [details]
Clears the suspended pages

[Chris Dumez]

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.

[Ryosuke Niwa]

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

[Chris Dumez]

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.

[Ryosuke Niwa]

Created attachment 362668 [details]
Fixes the bug

[Chris Dumez]

Comment on attachment 362668 [details]
Fixes the bug

r=me

[Ryosuke Niwa]

Committed r241928: <https://trac.webkit.org/changeset/241928>

[Geoffrey Garen]

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

[Chris Dumez]

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

[Truitt Savell]

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

[Chris Dumez]

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

[Chris Dumez]

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