Ami did some detective work in bug 89733, comment 8 and got the following stack: WebCore::RenderObject::setNeedsLayout() [0x7f7e5781e4ff] WebCore::RenderVideo::updateIntrinsicSize() [0x7f7e57a5353a] WebCore::RenderVideo::updatePlayer() [0x7f7e57a53d79] WebCore::RenderVideo::paintReplaced() [0x7f7e57a53acc] WebCore::RenderReplaced::paint() [0x7f7e579f6299] WebCore::RenderImage::paint() [0x7f7e5794c0ca] WebCore::RenderLayer::paintLayerContents() [0x7f7e5796c35e] WebCore::RenderLayer::paintLayerContentsAndReflection() [0x7f7e5796b63f] WebCore::RenderLayer::paintLayer() [0x7f7e5796ac36] WebCore::RenderLayer::paintList() [0x7f7e5796d3c5] WebCore::RenderLayer::paintLayerContents() [0x7f7e5796c58d] WebCore::RenderLayer::paintLayerContentsAndReflection() [0x7f7e5796b63f] WebCore::RenderLayer::paintLayer() [0x7f7e5796ac36] WebCore::RenderLayer::paintList() [0x7f7e5796d3c5] WebCore::RenderLayer::paintLayerContents() [0x7f7e5796c5fb] WebCore::RenderLayer::paintLayerContentsAndReflection() [0x7f7e5796b63f] WebCore::RenderLayer::paintLayer() [0x7f7e5796ac36] WebCore::RenderLayer::paint() [0x7f7e5796a49d] WebCore::FrameView::paintContents() [0x7f7e58865327] WebCore::ScrollView::paint() [0x7f7e57eba158] WebKit::PageWidgetDelegate::paint() [0x7f7e573f0956] WebKit::WebViewImpl::paint() [0x7f7e574b55eb] content::RenderWidget::PaintRect() [0x7f7e55d97da6] content::RenderWidget::DoDeferredUpdate() [0x7f7e55d9312d] content::RenderWidget::DoDeferredUpdateAndSendInputAck() [0x7f7e55d97319] content::RenderWidget::InvalidationCallback() [0x7f7e55d98a7a] Looking at the code, I'm not sure why we're trying to update the layout during paint. That seems wrong. My random guess would be that we can just remove the call to updatePlayer since RenderVideo::layout has already called it at this point.
Created attachment 170434 [details] Patch
Comment on attachment 170434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170434&action=review > Source/WebCore/rendering/RenderObject.cpp:95 > +RenderObject::SetLayoutNeededForbiddingScope::SetLayoutNeededForbiddingScope(RenderObject* renderObject) I'd call this SetLayoutNeededForbiddenScope > Source/WebCore/rendering/RenderObject.cpp:97 > + , m_preExistingForbidden(m_renderObject->isSetNeedsLayoutForbidden()) m_preexistingForbidden perhaps?
Comment on attachment 170434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170434&action=review Thanks for the review. Does this mean you think the change to RenderVideo is correct? >> Source/WebCore/rendering/RenderObject.cpp:95 >> +RenderObject::SetLayoutNeededForbiddingScope::SetLayoutNeededForbiddingScope(RenderObject* renderObject) > > I'd call this SetLayoutNeededForbiddenScope Done. >> Source/WebCore/rendering/RenderObject.cpp:97 >> + , m_preExistingForbidden(m_renderObject->isSetNeedsLayoutForbidden()) > > m_preexistingForbidden perhaps? Done.
Created attachment 170439 [details] Patch
Comment on attachment 170439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170439&action=review Looks fine to me, but let's wait for the mac ews bot to finish. Also, the ews bots run in Release, so we should watch the waterfall closely after this lands for additional failing tests. > Source/WebCore/rendering/RenderObject.h:255 > + SetLayoutNeededForbiddenScope(RenderObject*); explicit
Created attachment 170445 [details] Patch
Comment on attachment 170439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170439&action=review >> Source/WebCore/rendering/RenderObject.h:255 >> + SetLayoutNeededForbiddenScope(RenderObject*); > > explicit Done.
Comment on attachment 170445 [details] Patch Clearing flags on attachment: 170445 Committed r132398: <http://trac.webkit.org/changeset/132398>
All reviewed patches have been landed. Closing bug.
*** Bug 98449 has been marked as a duplicate of this bug. ***
I believe that this patch caused https://bugs.webkit.org/show_bug.cgi?id=104735
(In reply to comment #11) > I believe that this patch caused https://bugs.webkit.org/show_bug.cgi?id=104735 s/caused/revealed/ ? Per bug 89733 calling setNeedsLayout() during a paint() is a programming bug. The patch I landed in this bug merely makes the programming bug apparent at the time of violation, instead of an arbitrary amount of time later (e.g. the causes of bug 89733).
(In reply to comment #12) > (In reply to comment #11) > > I believe that this patch caused https://bugs.webkit.org/show_bug.cgi?id=104735 > > s/caused/revealed/ ? > Per bug 89733 calling setNeedsLayout() during a paint() is a programming bug. The patch I landed in this bug merely makes the programming bug apparent at the time of violation, instead of an arbitrary amount of time later (e.g. the causes of bug 89733). This patch caused bots to be red(der). That is something that needs to be dealt with immediately. Unfortunately it has been a while, but I will deal with it now. But yes, longer term, there is obviously a plugin bug to be fixed.
Looks like this is getting more attention in bug 104735 so let's continue the conversation there.