Bug 114233 - Don't create another plugin process for restarted plugins
Summary: Don't create another plugin process for restarted plugins
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-04-08 20:00 PDT by Dean Jackson
Modified: 2013-04-09 12:47 PDT (History)
2 users (show)

See Also:


Attachments
Patch (9.48 KB, patch)
2013-04-08 20:11 PDT, Dean Jackson
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2013-04-08 20:00:02 PDT
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>
Comment 1 Dean Jackson 2013-04-08 20:11:27 PDT
Created attachment 196984 [details]
Patch
Comment 2 Geoffrey Garen 2013-04-08 21:09:40 PDT
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"?
Comment 3 Jon Lee 2013-04-08 22:37:48 PDT
(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.
Comment 4 Dean Jackson 2013-04-09 12:27:08 PDT
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.
Comment 5 Dean Jackson 2013-04-09 12:47:24 PDT
Committed r148038: <http://trac.webkit.org/changeset/148038>