Bug 45855 – Plugin added dynamically to the DOM fails to paint after initial creation

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Ananta Iyengar]

Created attachment 67747 [details]
Test case.

This bug was originally logged in Chromium against Flash. http://code.google.com/p/chromium/issues/detail?id=55370
In this case it is a windowed plugin which does not display in Chrome. It displays in Safari as the windowed plugin
becomes visible when Widget::show is called. Chromium relies on a subsequent paint to make the plugin window visible.

However Safari would have the same bug if the plugin is a windowless plugin.

Based on our debugging it appears to be a bug in Webkit. If an object tag is added dynamically to the DOM it does not
paint. Based on a discussion with james robinson it seems that RenderWidget::setWidget should mark the widget as needs
painting.

A simple test case is attached.

The callstack in the debugger when the plugin element is added is as below.

ChildEBP RetAddr  
0012eabc 03286295 chrome_1c30000!webkit_glue::WebPluginImpl::updateGeometry+0x191 [z:\code\trunk\src\webkit\glue\plugins\webplugin_impl.cc @ 281]
0012eb5c 03285df4 chrome_1c30000!WebKit::WebPluginContainerImpl::reportGeometry+0xd5 [z:\code\trunk\src\third_party\webkit\webkit\chromium\src\webplugincontainerimpl.cpp @ 286]
0012eb6c 0440178a chrome_1c30000!WebKit::WebPluginContainerImpl::setParent+0x34 [z:\code\trunk\src\third_party\webkit\webkit\chromium\src\webplugincontainerimpl.cpp @ 221]
0012eba0 047e3273 chrome_1c30000!WebCore::ScrollView::addChild+0x7a [z:\code\trunk\src\third_party\webkit\webcore\platform\scrollview.cpp @ 69]
0012ebc8 047e316f chrome_1c30000!WebCore::moveWidgetToParentSoon+0x43 [z:\code\trunk\src\third_party\webkit\webcore\rendering\renderwidget.cpp @ 91]
0012ec20 04aceff2 chrome_1c30000!WebCore::RenderWidget::setWidget+0x18f [z:\code\trunk\src\third_party\webkit\webcore\rendering\renderwidget.cpp @ 211]
0012ec38 0472b56b chrome_1c30000!WebCore::RenderPart::setWidget+0x52 [z:\code\trunk\src\third_party\webkit\webcore\rendering\renderpart.cpp @ 54]
0012ec94 0472a732 chrome_1c30000!WebCore::SubframeLoader::loadPlugin+0x1ab [z:\code\trunk\src\third_party\webkit\webcore\loader\subframeloader.cpp @ 367]
0012ed80 0479d93c chrome_1c30000!WebCore::SubframeLoader::requestObject+0x242 [z:\code\trunk\src\third_party\webkit\webcore\loader\subframeloader.cpp @ 137]
0012edfc 044cc40c chrome_1c30000!WebCore::HTMLObjectElement::updateWidget+0x21c [z:\code\trunk\src\third_party\webkit\webcore\html\htmlobjectelement.cpp @ 292]
0012ee14 044cc58f chrome_1c30000!WebCore::FrameView::updateWidget+0xfc [z:\code\trunk\src\third_party\webkit\webcore\page\frameview.cpp @ 1607]
0012ee94 044cc7bb chrome_1c30000!WebCore::FrameView::updateWidgets+0x13f [z:\code\trunk\src\third_party\webkit\webcore\page\frameview.cpp @ 1640]
0012eec8 044c94aa chrome_1c30000!WebCore::FrameView::performPostLayoutTasks+0xdb [z:\code\trunk\src\third_party\webkit\webcore\page\frameview.cpp @ 1669]
0012ef7c 044cb6b8 chrome_1c30000!WebCore::FrameView::layout+0x95a [z:\code\trunk\src\third_party\webkit\webcore\page\frameview.cpp @ 849]
0012ef8c 044cf459 chrome_1c30000!WebCore::FrameView::layoutTimerFired+0x18 [z:\code\trunk\src\third_party\webkit\webcore\page\frameview.cpp @ 1362]
0012efa0 04707559 chrome_1c30000!WebCore::Timer<WebCore::FrameView>::fired+0x29 [z:\code\trunk\src\third_party\webkit\webcore\platform\timer.h @ 98]
0012efd8 04707476 chrome_1c30000!WebCore::ThreadTimers::sharedTimerFiredInternal+0xd9 [z:\code\trunk\src\third_party\webkit\webcore\platform\threadtimers.cpp @ 112]
0012efe0 0312a24b chrome_1c30000!WebCore::ThreadTimers::sharedTimerFired+0x16 [z:\code\trunk\src\third_party\webkit\webcore\platform\threadtimers.cpp @ 91]
0012eff0 0312b0dc chrome_1c30000!webkit_glue::WebKitClientImpl::DoTimeout+0x2b [z:\code\trunk\src\webkit\glue\webkitclient_impl.h @ 65]
0012effc 0312ab94 chrome_1c30000!DispatchToMethod<webkit_glue::WebKitClientImpl,void (__thiscall webkit_glue::WebKitClientImpl::*)(void)>+0xc [z:\code\trunk\src\base\tuple.h @ 537]

[James Robinson]

+cc some folks.

My first suspicion is that we need to mark a RenderWidget as needing a repaint on the first call to setWidget()

[Eric Seidel (no email)]

Is this a regression?  I recently was in this code.

[James Robinson]

We suspect it's not a regression, but is frequently covered up by further invalidations that cover the plugin.

[Ananta Iyengar]

Created attachment 87456 [details]
Initial patch

[Ananta Iyengar]

Created attachment 87459 [details]
Patch with tabs removed.

[James Robinson]

Comment on attachment 87459 [details]
Patch with tabs removed.

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

> LayoutTests/plugins/windowless_plugin_paint_test.html:8
> +      -webkit-transform: rotateZ(0deg) rotateX(0deg) rotateY(0deg);

translateZ(0) is the more typical way to force compositing

[Simon Fraser (smfr)]

Comment on attachment 87459 [details]
Patch with tabs removed.

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

>> LayoutTests/plugins/windowless_plugin_paint_test.html:8
>> +      -webkit-transform: rotateZ(0deg) rotateX(0deg) rotateY(0deg);
> 
> translateZ(0) is the more typical way to force compositing

Why does compositing have anything to do with it? I don't see any mention of this in the bug.

[Ananta Iyengar]

Created attachment 87598 [details]
Patch with review comments addressed

Removed the transform related code from the layout test.

[James Robinson]

Comment on attachment 87598 [details]
Patch with review comments addressed

Looks good

[WebKit Commit Bot]

Comment on attachment 87598 [details]
Patch with review comments addressed

Clearing flags on attachment: 87598

Committed r82616: <http://trac.webkit.org/changeset/82616>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Here's the result diff on Chromium Win 7:

--- E:\b\build\slave\Webkit_Win7\build\src\webkit\Release\..\../../layout-test-results\plugins/windowless_plugin_paint_test-expected.txt 
+++ E:\b\build\slave\Webkit_Win7\build\src\webkit\Release\..\../../layout-test-results\plugins/windowless_plugin_paint_test-actual.txt 
@@ -1,3 +1,3 @@
 This tests that dynamically added windowless plugins receive paint events on creation.
-SUCCESS
+FAILURE

[WebKit Review Bot]

http://trac.webkit.org/changeset/82616 might have broken Qt Linux Release

[James Robinson]

Ananta, can you take a look?  What platform did you test this on?

[Ananta Iyengar]

Created attachment 87805 [details]
Updated patch which has the fixes for the new layout test to ensure that it works correctly with DumpRenderTree

[Adam Barth]

Comment on attachment 87805 [details]
Updated patch which has the fixes for the new layout test to ensure that it works correctly with DumpRenderTree

Thanks!

[WebKit Commit Bot]

Comment on attachment 87805 [details]
Updated patch which has the fixes for the new layout test to ensure that it works correctly with DumpRenderTree

Clearing flags on attachment: 87805

Committed r82652: <http://trac.webkit.org/changeset/82652>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82652 might have broken Qt Linux Release
The following tests are not passing:
plugins/windowless_plugin_paint_test.html

[Adam Barth]

Still fails on Mac and Linux (as well as other non-Chromium ports)

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20-%20chromium.org&tests=plugins%2Fwindowless_plugin_paint_test.html

[James Robinson]

Do plugins block the onload event?

[Adam Barth]

Looks like this crashes on XP:

http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r82654%20(27040)/plugins/windowless_plugin_paint_test-crash-log.txt

[James Robinson]

Hmm yeah, manipulating the DOM during painting is probably a bad idea.

[Ananta Iyengar]

Created attachment 87827 [details]
Updated patch which has the fixes for the new layout test to ensure that it does not crash on Windows XP. This test needs to be skipped on chromium(linux, mac), qt and gtk

[Adam Barth]

Comment on attachment 87827 [details]
Updated patch which has the fixes for the new layout test to ensure that it does not crash on Windows XP. This test needs to be skipped on chromium(linux, mac), qt and gtk

Third time's the charm?

[Adam Barth]

Reopen so the commit-queue sees the patch.

[WebKit Commit Bot]

Comment on attachment 87827 [details]
Updated patch which has the fixes for the new layout test to ensure that it does not crash on Windows XP. This test needs to be skipped on chromium(linux, mac), qt and gtk

Clearing flags on attachment: 87827

Committed r82758: <http://trac.webkit.org/changeset/82758>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Sorry ananta, this test still fails (at least on Chromium-Linux).  :(

http://build.chromium.org/f/chromium/layout_test_results/Webkit_Linux/results/layout-test-results/plugins/windowless_plugin_paint_test-wdiff.html

[Adam Barth]

ananta set me straight.  This just needs a TIMEOUT expectation.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82758 might have broken SnowLeopard Intel Release (WebKit2 Tests)
The following tests are not passing:
inspector/styles/styles-add-blank-property.html
plugins/windowless_plugin_paint_test.html

[Ananta Iyengar]

Looking at the mac failure.