Bug 114233 – Don't create another plugin process for restarted plugins

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Dean Jackson]

Snapshotted plugins run in their own process. This causes a bit of confusion, because there is also a normal plugin process that handles autostarted plugins. But, it's still correct.

However, restarted plugins get a new process type, and thus start *another* process. They should just use the existing normal process.

And to reduce potential confusion, make the snapshotting process kill itself a lot sooner if it doesn't have anything to do. It doesn't need to run the plugins for long, and the other process will be around so hopefully it is paged in.

<rdar://problem/13503755>

[Dean Jackson]

Created attachment 196984 [details]
Patch

[Geoffrey Garen]

Comment on attachment 196984 [details]
Patch

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

r=me

This is a clear improvement.

But I wonder why we have two processes at all: Why can't snapshotting take place in the singleton plug-in process?

> Source/WebKit2/WebProcess/Plugins/PluginProxy.h:62
> -    static PassRefPtr<PluginProxy> create(const String& pluginPath, PluginProcess::Type);
> +    static PassRefPtr<PluginProxy> create(const String& pluginPath, PluginProcess::Type, bool);
>      ~PluginProxy();
>  
>      uint64_t pluginInstanceID() const { return m_pluginInstanceID; }

This function should name its bool parameter since the meaning is not obvious.

> Source/WebKit2/WebProcess/Plugins/PluginProxy.h:71
> +    explicit PluginProxy(const String& pluginPath, PluginProcess::Type, bool);

Ditto.

> Source/WebKit2/WebProcess/Plugins/PluginProxy.h:221
> +    bool m_restartedProcess;

I prefer the "is" prefix for bools, so they read more clearly. "Restarted process" might sound like "I restarted a process", which is not what we mean. Also, it's confusing to name this bool about a process, since we have a separate variable for process type. How about: "bool m_isRestartedPlugin"?

[Jon Lee]

(In reply to comment #2)
> (From update of attachment 196984 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196984&action=review
> 
> r=me
> 
> This is a clear improvement.
> 
> But I wonder why we have two processes at all: Why can't snapshotting take place in the singleton plug-in process?

Audio is sandboxed in one of them.

[Dean Jackson]

Comment on attachment 196984 [details]
Patch

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

>> Source/WebKit2/WebProcess/Plugins/PluginProxy.h:62
>>      uint64_t pluginInstanceID() const { return m_pluginInstanceID; }
> 
> This function should name its bool parameter since the meaning is not obvious.

Yeah, I don't know why I forgot this, nor why I didn't prefix with "is". Thanks.

[Dean Jackson]

Committed r148038: <http://trac.webkit.org/changeset/148038>