Bug 100265 - call to setNeedsLayout during RenderVideo::paintReplaced
: call to setNeedsLayout during RenderVideo::paintReplaced
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Ami Fischman
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-24 10:20 PDT by Tony Chang
Modified: 2012-12-11 16:36 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2012-10-24 10:20:13 PDT
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 Ami Fischman 2012-10-24 11:15:15 PDT
Created attachment 170434 [details]
Patch
Comment 2 Simon Fraser (smfr) 2012-10-24 11:20:28 PDT
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 3 Ami Fischman 2012-10-24 11:32:10 PDT
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.
Comment 4 Ami Fischman 2012-10-24 11:32:43 PDT
Created attachment 170439 [details]
Patch
Comment 5 Tony Chang 2012-10-24 12:17:54 PDT
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
Comment 6 Ami Fischman 2012-10-24 12:21:14 PDT
Created attachment 170445 [details]
Patch
Comment 7 Ami Fischman 2012-10-24 12:21:24 PDT
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 8 WebKit Review Bot 2012-10-24 14:07:14 PDT
Comment on attachment 170445 [details]
Patch

Clearing flags on attachment: 170445

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