Bug 100265 - call to setNeedsLayout during RenderVideo::paintReplaced
: call to setNeedsLayout during RenderVideo::paintReplaced
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-10-24 10:20 PST by
Modified: 2012-12-11 16:36 PST (History)


Attachments
Patch (5.02 KB, patch)
2012-10-24 11:15 PST, Ami Fischman
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.01 KB, patch)
2012-10-24 11:32 PST, Ami Fischman
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.01 KB, patch)
2012-10-24 12:21 PST, Ami Fischman
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-10-24 10:20:13 PST
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.
------- Comment #1 From 2012-10-24 11:15:15 PST -------
Created an attachment (id=170434) [details]
Patch
------- Comment #2 From 2012-10-24 11:20:28 PST -------
(From update of attachment 170434 [details])
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 #3 From 2012-10-24 11:32:10 PST -------
(From update of attachment 170434 [details])
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.
------- Comment #4 From 2012-10-24 11:32:43 PST -------
Created an attachment (id=170439) [details]
Patch
------- Comment #5 From 2012-10-24 12:17:54 PST -------
(From update of attachment 170439 [details])
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
------- Comment #6 From 2012-10-24 12:21:14 PST -------
Created an attachment (id=170445) [details]
Patch
------- Comment #7 From 2012-10-24 12:21:24 PST -------
(From update of attachment 170439 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=170439&action=review

>> Source/WebCore/rendering/RenderObject.h:255
>> +        SetLayoutNeededForbiddenScope(RenderObject*);
> 
> explicit

Done.
------- Comment #8 From 2012-10-24 14:07:14 PST -------
(From update of attachment 170445 [details])
Clearing flags on attachment: 170445

Committed r132398: <http://trac.webkit.org/changeset/132398>
------- Comment #9 From 2012-10-24 14:07:18 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #10 From 2012-11-29 09:19:16 PST -------
*** Bug 98449 has been marked as a duplicate of this bug. ***
------- Comment #11 From 2012-12-11 16:16:02 PST -------
I believe that this patch caused https://bugs.webkit.org/show_bug.cgi?id=104735
------- Comment #12 From 2012-12-11 16:22:03 PST -------
(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).
------- Comment #13 From 2012-12-11 16:33:01 PST -------
(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.
------- Comment #14 From 2012-12-11 16:36:56 PST -------
Looks like this is getting more attention in bug 104735 so let's continue the conversation there.