Bug 180819 – Avoid waking plugin process up unnecessarily

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

WebKit purges data from origins marked as prevalent on an hourly interval. This includes waking up plugins and removing relevant data stored in those plugins. This causes multiple plugin processes to be spawned, even though the user is not interacting with any plugins.

Instead, we should delay removing data from plugins until they are loaded due to the user interacting with a website using a plugin.

Make the following changes:

1. When looking for plugin data related to prevalent sites, only examine plugin data if the relevant plugin is already running.
2. When the state of the active plugins changes, trigger a data removal check.

[Brent Fulgham]

<rdar://problem/36051548>

[Brent Fulgham]

Created attachment 329367 [details]
Patch

[EWS Watchlist]

Attachment 329367 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/Plugins/PluginProcessManager.cpp:99:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/Plugins/PluginProcessManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 329377 [details]
Patch

[EWS Watchlist]

Attachment 329377 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/Plugins/PluginProcessManager.cpp:99:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/Plugins/PluginProcessManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

I think this approach would allow us to do the same for the Database Process.

[Geoffrey Garen]

Comment on attachment 329377 [details]
Patch

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

r- because I think the count thing is wrong.

The rest of my comments are aesthetic preferences, but they probably don't make the code wrong.

> Source/WebKit/Shared/WebsiteData/WebsiteDataFetchOption.h:32
> +    DoNotCreateProcesses = 1 << 1,

I think this would make more sense if it were an affirmative value, "LaunchPluginProcesses". Launching plugin processes and computing sizes are both optional additions to the standard fetch operation, with additional cost, which we would like to avoid if we can. Generally speaking, flags make more sense when they indicate the presence of something. Otherwise, you end up with double negatives like "if (!(x & DoNotCreateProcesses))", which reads "if I do not have the do not create processes option set", which hurts my brain.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:248
> +    OptionSet<WebKit::WebsiteDataFetchOption> fetchOptions = WebKit::WebsiteDataFetchOption::DoNotCreateProcesses;

I'm curious -- does anybody ever specify DO create processes?

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:638
> +    auto activePluginCount = WebKit::PluginProcessManager::singleton().pluginProcesses().size();
> +    if (activePluginCount != m_activePluginCount)
> +        return true;

What happens if a new plugin launches and an old plugin exits? It seems like you would decide that the count is the same, and do nothing. I don't think that's right. Or do plugins never exit, and does the active count only go up? If so, you should assert that.

> Source/WebKit/UIProcess/Plugins/PluginProcessManager.cpp:122
> +PluginProcessProxy* PluginProcessManager::getOrCreatePluginProcess(uint64_t pluginProcessToken, ProcessAccessType processesAccessType)

I think it would be clearer if you broke this into two functions: get, and getOrCreate, with getOrCreate calling get. It's too weird for getOrCreate to take a parameter that specifies "just kidding, don't create".

The caller can call get or getOrCreate depending on its needs.

> Source/WebKit/UIProcess/Plugins/PluginProcessManager.h:63
> +    enum class ProcessAccessType { OnlyIfLaunched, Launch };
> +    void fetchWebsiteData(const PluginModuleInfo&, ProcessAccessType createProcessesBehavior, WTF::Function<void (Vector<String>)>&& completionHandler);

I think you can use the WebsiteDataFetchOption enum here. This function is about fetching website data.

[Brent Fulgham]

Comment on attachment 329377 [details]
Patch

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

>> Source/WebKit/Shared/WebsiteData/WebsiteDataFetchOption.h:32
>> +    DoNotCreateProcesses = 1 << 1,
> 
> I think this would make more sense if it were an affirmative value, "LaunchPluginProcesses". Launching plugin processes and computing sizes are both optional additions to the standard fetch operation, with additional cost, which we would like to avoid if we can. Generally speaking, flags make more sense when they indicate the presence of something. Otherwise, you end up with double negatives like "if (!(x & DoNotCreateProcesses))", which reads "if I do not have the do not create processes option set", which hurts my brain.

I can see your point, but this new option is an edge case. In all places where WebsiteDataFetch operations are happening, we expect processes to be launched so that their plugin data (or database data) can be inspected or deleted. The case I'm addressing in this patch is a high-frequency probe of website data, where we do not want to spawn new processes if they don't already exist. Using a positive flag name here would make users of the API have to set a flag for the 'normal' case, and not setting the flag would give you the 'unusual' case.

I think making the default behavior the "unusual" use case would lead to mistakes by people unfamiliar with the code.

>> Source/WebKit/UIProcess/WebProcessProxy.cpp:248
>> +    OptionSet<WebKit::WebsiteDataFetchOption> fetchOptions = WebKit::WebsiteDataFetchOption::DoNotCreateProcesses;
> 
> I'm curious -- does anybody ever specify DO create processes?

All uses of the fetch and delete API to date expect processes to be automatically spawned to do the work. So the ResourceLoadStatistics case is an unusual edge case, where we want to opportunistically inspect running processes, but don't want to pay the cost to launch them.

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:638
>> +        return true;
> 
> What happens if a new plugin launches and an old plugin exits? It seems like you would decide that the count is the same, and do nothing. I don't think that's right. Or do plugins never exit, and does the active count only go up? If so, you should assert that.

Yeah -- I thought about this, too, but I was worried that would make this overly complicated. However, it looks like the plugin process proxies all carry around a custom token we can use to distinguish them. I'll use that to decide whether the trigger the data fetch.n

>> Source/WebKit/UIProcess/Plugins/PluginProcessManager.cpp:122
>> +PluginProcessProxy* PluginProcessManager::getOrCreatePluginProcess(uint64_t pluginProcessToken, ProcessAccessType processesAccessType)
> 
> I think it would be clearer if you broke this into two functions: get, and getOrCreate, with getOrCreate calling get. It's too weird for getOrCreate to take a parameter that specifies "just kidding, don't create".
> 
> The caller can call get or getOrCreate depending on its needs.

Lol. Sounds good! I'll change it.

>> Source/WebKit/UIProcess/Plugins/PluginProcessManager.h:63
>> +    void fetchWebsiteData(const PluginModuleInfo&, ProcessAccessType createProcessesBehavior, WTF::Function<void (Vector<String>)>&& completionHandler);
> 
> I think you can use the WebsiteDataFetchOption enum here. This function is about fetching website data.

Sounds good.

[Brent Fulgham]

Created attachment 329490 [details]
Patch

[EWS Watchlist]

Attachment 329490 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/Plugins/PluginProcessManager.cpp:100:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/Plugins/PluginProcessManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 329490 [details]
Patch

r=me

[Brent Fulgham]

Committed r225984: <https://trac.webkit.org/changeset/225984>