Bug 100265 – call to setNeedsLayout during RenderVideo::paintReplaced

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Tony Chang]

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.

[Ami Fischman]

Created attachment 170434 [details]
Patch

[Simon Fraser (smfr)]

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?

[Ami Fischman]

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.

[Ami Fischman]

Created attachment 170439 [details]
Patch

[Tony Chang]

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

[Ami Fischman]

Created attachment 170445 [details]
Patch

[Ami Fischman]

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.

[WebKit Review Bot]

Comment on attachment 170445 [details]
Patch

Clearing flags on attachment: 170445

Committed r132398: <http://trac.webkit.org/changeset/132398>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Abhishek Arya]

*** Bug 98449 has been marked as a duplicate of this bug. ***

[Beth Dakin]

I believe that this patch caused https://bugs.webkit.org/show_bug.cgi?id=104735

[Ami Fischman]

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

[Beth Dakin]

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

[Ami Fischman]

Looks like this is getting more attention in bug 104735 so let's continue the conversation there.