Bug 93894 - With asynchronous plug-in initialization, WebProcess and PluginProcess can deadlock
Summary: With asynchronous plug-in initialization, WebProcess and PluginProcess can de...
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-13 13:36 PDT by Brady Eidson
Modified: 2012-08-13 15:04 PDT (History)
4 users (show)

See Also:


Attachments
Patch v1 - Fix + regression test (23.56 KB, patch)
2012-08-13 13:50 PDT, Brady Eidson
ap: review+
gns: commit-queue-
Details | Formatted Diff | Diff
Reviewed patch for EWS testing (23.59 KB, patch)
2012-08-13 14:26 PDT, Brady Eidson
no flags 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-13 13:36:32 PDT
With asynchronous plug-in initialization, WebProcess and PluginProcess can deadlock (or ASSERT in debug builds)

Step 1 - Run ToT
Step 2 - Go to http://www.youtube.com/watch?v=rDvweiW5ZKQ&feature=plcp with Flash and with HTML5 video disabled

WebProcess is waiting on synchronous completion of plug-in initialization.
PluginProcess is sync-messaging to the WebProcess for NPP_Evaluate.

Both processes should be handling messages in the meantime but are not.

In radar as <rdar://problem/12062125>
Comment 1 Brady Eidson 2012-08-13 13:50:18 PDT
Created attachment 158094 [details]
Patch v1 - Fix + regression test
Comment 2 WebKit Review Bot 2012-08-13 13:54:56 PDT
Attachment 158094 [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
Tools/DumpRenderTree/TestNetscapePlugIn/Tests/EvaluateJSWithinNPP_New.cpp:39:  The parameter name "saved" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Gustavo Noronha (kov) 2012-08-13 13:57:48 PDT
Comment on attachment 158094 [details]
Patch v1 - Fix + regression test

Attachment 158094 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13482885
Comment 4 Alexey Proskuryakov 2012-08-13 14:04:30 PDT
Comment on attachment 158094 [details]
Patch v1 - Fix + regression test

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

> Source/WebKit2/PluginProcess/WebProcessConnection.cpp:274
> +        if (pluginControllerProxy->isInitializing()) {
> +            pluginControllerProxy->setInitializationReply(reply);

Is a single reply enough,or should we have a vector/set?

> Source/WebKit2/PluginProcess/WebProcessConnection.cpp:281
> +#elif

Whoa.

> Source/WebKit2/PluginProcess/WebProcessConnection.cpp:321
> +    // even if the web process is waiting on a synchronous reply itself.

Even if? I might be confused, but I think that it's likely or even certain to be waiting on a sync reply in this case.
Comment 5 Brady Eidson 2012-08-13 14:13:49 PDT
(In reply to comment #4)
> (From update of attachment 158094 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158094&action=review
> 
> > Source/WebKit2/PluginProcess/WebProcessConnection.cpp:274
> > +        if (pluginControllerProxy->isInitializing()) {
> > +            pluginControllerProxy->setInitializationReply(reply);
> 
> Is a single reply enough,or should we have a vector/set?

Yes it is enough.  In no case should any one plug-in instance (represented by a PluginControllerProxy) have more than one interested party waiting for it's synchronous initialization.

There is an assertion to this effect in PluginControllerProxy::setInitializationReply()

> > Source/WebKit2/PluginProcess/WebProcessConnection.cpp:321
> > +    // even if the web process is waiting on a synchronous reply itself.
> 
> Even if? I might be confused, but I think that it's likely or even certain to be waiting on a sync reply in this case.

Definitely not certain.  This only happens if something in the WebProcess triggers access of the plugin script object which forces it to wait until initialization is completed.

That happens in the Youtube example, but is not something that happens on most sites.

Thanks for the review!
Comment 6 Brady Eidson 2012-08-13 14:14:09 PDT
Digging into the GTK failure now
Comment 7 Brady Eidson 2012-08-13 14:15:39 PDT
(In reply to comment #3)
> (From update of attachment 158094 [details])
> Attachment 158094 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/13482885

500 characters is (not surprisingly) not enough of a buffer to show the nature of the failure.
Comment 8 Brady Eidson 2012-08-13 14:25:09 PDT
(In reply to comment #7)
> (In reply to comment #3)
> > (From update of attachment 158094 [details] [details])
> > Attachment 158094 [details] [details] did not pass gtk-ews (gtk):
> > Output: http://queues.webkit.org/results/13482885
> 
> 500 characters is (not surprisingly) not enough of a buffer to show the nature of the failure.

NM, I got it.

These unformatted logs are difficult to hunt through  :)
Comment 9 Brady Eidson 2012-08-13 14:26:32 PDT
Created attachment 158104 [details]
Reviewed patch for EWS testing
Comment 10 Early Warning System Bot 2012-08-13 14:55:38 PDT
Comment on attachment 158094 [details]
Patch v1 - Fix + regression test

Attachment 158094 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13493282
Comment 11 Brady Eidson 2012-08-13 15:04:27 PDT
http://trac.webkit.org/changeset/125457