Bug 93894 – With asynchronous plug-in initialization, WebProcess and PluginProcess can deadlock

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

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>

[Brady Eidson]

Created attachment 158094 [details]
Patch v1 - Fix + regression test

[WebKit Review Bot]

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.

[Gustavo Noronha (kov)]

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

[Alexey Proskuryakov]

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.

[Brady Eidson]

(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!

[Brady Eidson]

Digging into the GTK failure now

[Brady Eidson]

(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.

[Brady Eidson]

(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  :)

[Brady Eidson]

Created attachment 158104 [details]
Reviewed patch for EWS testing

[Early Warning System Bot]

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

[Brady Eidson]

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