Bug 92919 – Out-of-process plug-ins should support asynchronous initialization.

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Out-of-process plug-ins should support asynchronous initialization.

Currently the WebProcess synchronously waits for plug-ins to initialize.  It shouldn't have to.

To help reviewers I'll be trying to break this into a few different patches, the first of which will be dead code until they all land.

In radar as <rdar://problem/10598594>

[Brady Eidson]

Created attachment 155921 [details]
Part 1 - Add two related preferences to the API

[Brady Eidson]

Part 1 landed in http://trac.webkit.org/changeset/124393

I will be prepping the remainder into either 1 or 2 patches tomorrow.

[Brady Eidson]

Created attachment 156446 [details]
Part 2 - More preference API and groundwork for regression testing

[Brady Eidson]

http://trac.webkit.org/changeset/124649

Final patch will (unfortunately) be much larger.

[Build Bot]

Comment on attachment 156446 [details]
Part 2 - More preference API and groundwork for regression testing

Attachment 156446 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13437016

[Brady Eidson]

(In reply to comment #5)
> (From update of attachment 156446 [details])
> Attachment 156446 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/13437016

This failure is because the Windows builder didn't pick up the WebPreferences changes and regenerate everything.  I don't know what to do about that.

[Brady Eidson]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 156446 [details] [details])
> > Attachment 156446 [details] [details] did not pass win-ews (win):
> > Output: http://queues.webkit.org/results/13437016
> 
> This failure is because the Windows builder didn't pick up the WebPreferences changes and regenerate everything.  I don't know what to do about that.

Actually, wait, I might have failed on a much more epic level - Might not have gotten the new pref in my patch...  =/

Dealing with git right now to try and fix this.

[Brady Eidson]

http://trac.webkit.org/changeset/124652

Sorry about that!

[Gustavo Noronha (kov)]

Comment on attachment 156446 [details]
Part 2 - More preference API and groundwork for regression testing

Attachment 156446 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13425769

[Brady Eidson]

(In reply to comment #9)
> (From update of attachment 156446 [details])
> Attachment 156446 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/13425769

Will be fixed by http://trac.webkit.org/changeset/124652

[Brady Eidson]

Created attachment 156485 [details]
Part 3 - Add the feature (with some tests)

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebKit2/WebProcess/Plugins/PluginProcessConnection.h:64:  The parameter name "pluginProcessConnectionManager" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 38 files


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

[Gustavo Noronha (kov)]

Comment on attachment 156485 [details]
Part 3 - Add the feature (with some tests)

Attachment 156485 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13424873

[Brady Eidson]

(In reply to comment #12)
> Attachment 156485 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
> Source/WebKit2/WebProcess/Plugins/PluginProcessConnection.h:64:  The parameter name "pluginProcessConnectionManager" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Total errors found: 1 in 38 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Fixed locally, won't bother uploading a new patch for this.

[Brady Eidson]

(In reply to comment #13)
> (From update of attachment 156485 [details])
> Attachment 156485 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/13424873

Last 500 characters of output:
xy::DidCreateWebProcessConnection&)
./DerivedSources/WebKit2/PluginProcessProxyMessages.h:55:8: note:   no known conversion for argument 1 from 'CoreIPC::Attachment' to 'const Messages::PluginProcessProxy::DidCreateWebProcessConnection&'

This appears to be a compatibility problem with the compiler GTK is using.

[Brady Eidson]

(In reply to comment #15)
> (In reply to comment #13)
> > (From update of attachment 156485 [details] [details])
> > Attachment 156485 [details] [details] did not pass gtk-ews (gtk):
> > Output: http://queues.webkit.org/results/13424873
> 
> Last 500 characters of output:
> xy::DidCreateWebProcessConnection&)
> ./DerivedSources/WebKit2/PluginProcessProxyMessages.h:55:8: note:   no known conversion for argument 1 from 'CoreIPC::Attachment' to 'const Messages::PluginProcessProxy::DidCreateWebProcessConnection&'
> 
> This appears to be a compatibility problem with the compiler GTK is using.

This is a generated file.

And looking at the PluginProcessProxyMessages.h generated locally I'm not sure exactly why it's broken on GTK - Maybe their generated PluginProcessProxyMessages.h is different and broken?

[Early Warning System Bot]

Comment on attachment 156485 [details]
Part 3 - Add the feature (with some tests)

Attachment 156485 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13421996

[Build Bot]

Comment on attachment 156485 [details]
Part 3 - Add the feature (with some tests)

Attachment 156485 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13425852

[Brady Eidson]

(In reply to comment #17)
> (From update of attachment 156485 [details])
> Attachment 156485 [details] did not pass qt-wk2-ews (qt):
> Output: http://queues.webkit.org/results/13421996

(In reply to comment #18)
> (From update of attachment 156485 [details])
> Attachment 156485 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/13425852

Okay, clearly something quite real is happening.

I did build successfully on Mac before uploading.  Maybe I'll need to try a clean build to sort it out...

Reviewing the patch is still an admirable task in the meantime.  :)

[Brady Eidson]

(In reply to comment #19)
> (In reply to comment #17)
> > (From update of attachment 156485 [details] [details])
> > Attachment 156485 [details] [details] did not pass qt-wk2-ews (qt):
> > Output: http://queues.webkit.org/results/13421996
> 
> (In reply to comment #18)
> > (From update of attachment 156485 [details] [details])
> > Attachment 156485 [details] [details] did not pass mac-ews (mac):
> > Output: http://queues.webkit.org/results/13425852
> 
> Okay, clearly something quite real is happening.
> 
> I did build successfully on Mac before uploading.  Maybe I'll need to try a clean build to sort it out...
> 
> Reviewing the patch is still an admirable task in the meantime.  :)

I did a 100% clean build on Mac with the patch as is...  *sigh*  Digging in to the specific failures.

[Brady Eidson]

I see, probably compiler differences with OwnPtr = 0.  Something my compiler supports but older ones don't, I guess.  Will fix.

[Brady Eidson]

Created attachment 156721 [details]
Part 3 - v2 - Add the feature (with some tests)

[Early Warning System Bot]

Comment on attachment 156721 [details]
Part 3 - v2 - Add the feature (with some tests)

Attachment 156721 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13445358

[Brady Eidson]

(In reply to comment #23)
> (From update of attachment 156721 [details])
> Attachment 156721 [details] did not pass qt-wk2-ews (qt):
> Output: http://queues.webkit.org/results/13445358

AHA, realized what the deal is.  Didn't update the non-Mac block here.  New patch forthcoming.

[Brady Eidson]

Created attachment 156734 [details]
Part 3 - v3 - Add the feature (with some tests) (and non-Mac build fixes)

[Brady Eidson]

Created attachment 156781 [details]
Part 3 - v4 - Add the feature (with some tests) (and Qt build fixes) (and possibly GTK build fixes)

[Brady Eidson]

GTK doesn't have sleep() declared... trying a direct include of <unistd.h> as a fix

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebKit2/PluginProcess/WebProcessConnection.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 38 files


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

[Anders Carlsson]

Comment on attachment 156781 [details]
Part 3 - v4 - Add the feature (with some tests) (and Qt build fixes) (and possibly GTK build fixes)

> Source/WebKit2/PluginProcess/WebProcessConnection.cpp:93
> +static HashSet<uint64_t>& asynchronousInstanceIDsToIgnore()
> +{
> +    DEFINE_STATIC_LOCAL(HashSet<uint64_t>, instances, ());
> +    return instances;
> +}

This should be a member of WebProcessConnection. pluginInstanceIDs are not unique across multiple WebProcesses.

> Source/WebKit2/PluginProcess/WebProcessConnection.cpp:311
> +    if (creationParameters.artificialPluginInitializationDelayEnabled) {
> +        static unsigned int artificialPluginInitializationDelay = 5;
> +        sleep(artificialPluginInitializationDelay);
> +    }

I don't like that this test code is here, but I guess it's OK. The constant should not be static, and you can just use unsigned instead of unsigned int.

> Source/WebKit2/WebProcess/Plugins/Plugin.h:88
> +    virtual bool isInitializingAsynchronously() const = 0;

How about isBeingAsynchronouslyInitialized() instead? Also, add a comment indicating what this function does.

> Source/WebKit2/WebProcess/Plugins/PluginController.h:164
> +    virtual void didInitializePlugin() = 0;
> +    virtual void didFailToInitializePlugin() = 0;

Please add comments indicating what these functions are supposed to do.

[Brady Eidson]

Created attachment 156785 [details]
Part 3 - v5 - Add the feature, address review comments, and maybe have 100% working builds

[Brady Eidson]

http://trac.webkit.org/changeset/124815