Bug 92919 - Out-of-process plug-ins should support asynchronous initialization.
Summary: Out-of-process plug-ins should support asynchronous initialization.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-08-01 16:00 PDT by Brady Eidson
Modified: 2012-08-06 16:44 PDT (History)
6 users (show)

See Also:


Attachments
Part 1 - Add two related preferences to the API (6.83 KB, patch)
2012-08-01 16:43 PDT, Brady Eidson
andersca: review+
Details | Formatted Diff | Diff
Part 2 - More preference API and groundwork for regression testing (10.08 KB, patch)
2012-08-03 13:31 PDT, Brady Eidson
andersca: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Part 3 - Add the feature (with some tests) (66.69 KB, patch)
2012-08-03 17:12 PDT, Brady Eidson
gns: commit-queue-
Details | Formatted Diff | Diff
Part 3 - v2 - Add the feature (with some tests) (66.55 KB, patch)
2012-08-06 10:28 PDT, Brady Eidson
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Part 3 - v3 - Add the feature (with some tests) (and non-Mac build fixes) (67.00 KB, patch)
2012-08-06 11:44 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Part 3 - v4 - Add the feature (with some tests) (and Qt build fixes) (and possibly GTK build fixes) (67.03 KB, patch)
2012-08-06 16:06 PDT, Brady Eidson
andersca: review-
Details | Formatted Diff | Diff
Part 3 - v5 - Add the feature, address review comments, and maybe have 100% working builds (67.17 KB, patch)
2012-08-06 16:23 PDT, Brady Eidson
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2012-08-01 16:00:18 PDT
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>
Comment 1 Brady Eidson 2012-08-01 16:43:56 PDT
Created attachment 155921 [details]
Part 1 - Add two related preferences to the API
Comment 2 Brady Eidson 2012-08-01 17:27:44 PDT
Part 1 landed in http://trac.webkit.org/changeset/124393

I will be prepping the remainder into either 1 or 2 patches tomorrow.
Comment 3 Brady Eidson 2012-08-03 13:31:39 PDT
Created attachment 156446 [details]
Part 2 - More preference API and groundwork for regression testing
Comment 4 Brady Eidson 2012-08-03 13:50:26 PDT
http://trac.webkit.org/changeset/124649

Final patch will (unfortunately) be much larger.
Comment 5 Build Bot 2012-08-03 13:56:47 PDT
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
Comment 6 Brady Eidson 2012-08-03 14:07:51 PDT
(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.
Comment 7 Brady Eidson 2012-08-03 14:09:03 PDT
(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.
Comment 8 Brady Eidson 2012-08-03 14:13:43 PDT
http://trac.webkit.org/changeset/124652

Sorry about that!
Comment 9 Gustavo Noronha (kov) 2012-08-03 14:19:18 PDT
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
Comment 10 Brady Eidson 2012-08-03 14:31:02 PDT
(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
Comment 11 Brady Eidson 2012-08-03 17:12:54 PDT
Created attachment 156485 [details]
Part 3 - Add the feature (with some tests)
Comment 12 WebKit Review Bot 2012-08-03 17:16:42 PDT
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.
Comment 13 Gustavo Noronha (kov) 2012-08-03 17:21:07 PDT
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
Comment 14 Brady Eidson 2012-08-03 17:23:57 PDT
(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.
Comment 15 Brady Eidson 2012-08-03 17:25:26 PDT
(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.
Comment 16 Brady Eidson 2012-08-03 17:27:26 PDT
(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?
Comment 17 Early Warning System Bot 2012-08-03 17:58:28 PDT
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
Comment 18 Build Bot 2012-08-03 19:24:53 PDT
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
Comment 19 Brady Eidson 2012-08-03 22:33:55 PDT
(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.  :)
Comment 20 Brady Eidson 2012-08-06 10:22:50 PDT
(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.
Comment 21 Brady Eidson 2012-08-06 10:25:47 PDT
I see, probably compiler differences with OwnPtr = 0.  Something my compiler supports but older ones don't, I guess.  Will fix.
Comment 22 Brady Eidson 2012-08-06 10:28:24 PDT
Created attachment 156721 [details]
Part 3 - v2 - Add the feature (with some tests)
Comment 23 Early Warning System Bot 2012-08-06 11:33:06 PDT
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
Comment 24 Brady Eidson 2012-08-06 11:42:54 PDT
(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.
Comment 25 Brady Eidson 2012-08-06 11:44:32 PDT
Created attachment 156734 [details]
Part 3 - v3 - Add the feature (with some tests) (and non-Mac build fixes)
Comment 26 Brady Eidson 2012-08-06 16:06:37 PDT
Created attachment 156781 [details]
Part 3 - v4 - Add the feature (with some tests) (and Qt build fixes) (and possibly GTK build fixes)
Comment 27 Brady Eidson 2012-08-06 16:07:26 PDT
GTK doesn't have sleep() declared... trying a direct include of <unistd.h> as a fix
Comment 28 WebKit Review Bot 2012-08-06 16:09:30 PDT
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.
Comment 29 Anders Carlsson 2012-08-06 16:12:56 PDT
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.
Comment 30 Brady Eidson 2012-08-06 16:23:02 PDT
Created attachment 156785 [details]
Part 3 - v5 - Add the feature, address review comments, and maybe have 100% working builds
Comment 31 Brady Eidson 2012-08-06 16:44:02 PDT
http://trac.webkit.org/changeset/124815