Bug 85912 – Crash in computedCSSPadding* functions due to RenderImage::imageDimensionsChanged called during attachment

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

Created attachment 140779 [details]
Reproduction

Example stacktrace:

0:000> kp
ChildEBP RetAddr  
0012ed7c 0311270d chrome_1c30000!WebCore::RenderBoxModelObject::computedCSSPaddingBottom(void)+0x5b [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\rendering\renderboxmodelobject.cpp @ 633]
0012ed8c 0310278c chrome_1c30000!WebCore::RenderBoxModelObject::paddingBottom(void)+0xd [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\rendering\renderboxmodelobject.h @ 102]
0012edcc 03355ae7 chrome_1c30000!WebCore::RenderBox::contentBoxRect(void)+0x3c [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\rendering\renderbox.h @ 138]
0012ee6c 03355c9a chrome_1c30000!WebCore::RenderImage::imageDimensionsChanged(bool imageSizeChanged = false, class WebCore::IntRect * rect = 0x00000000)+0x1a7 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\rendering\renderimage.cpp @ 225]
0012ee90 03425a00 chrome_1c30000!WebCore::RenderImage::imageChanged(void * newImage = 0x01a71700, class WebCore::IntRect * rect = 0x00000000)+0x12a [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\rendering\renderimage.cpp @ 176]
0012eeb0 033df56e chrome_1c30000!WebCore::CachedImage::didAddClient(class WebCore::CachedResourceClient * c = 0x01a3411c)+0xa0 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\loader\cache\cachedimage.cpp @ 117]
0012eec4 033423f2 chrome_1c30000!WebCore::CachedResource::addClient(class WebCore::CachedResourceClient * client = 0x01a3411c)+0x1e [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\loader\cache\cachedresource.cpp @ 381]
0012eed4 0335481d chrome_1c30000!WebCore::RenderImageResourceStyleImage::initialize(class WebCore::RenderObject * renderer = 0x01a3411c)+0x32 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\rendering\renderimageresourcestyleimage.cpp @ 53]
0012eee4 032da771 chrome_1c30000!WebCore::RenderImage::setImageResource(class WTF::PassOwnPtr<WebCore::RenderImageResource> imageResource = class WTF::PassOwnPtr<WebCore::RenderImageResource>)+0x2d [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\rendering\renderimage.cpp @ 74]
0012eefc 036d4a27 chrome_1c30000!WebCore::RenderObject::createObject(class WebCore::Node * node = 0x01284200, class WebCore::RenderStyle * style = 0x01b4d010)+0x91 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\rendering\renderobject.cpp @ 135]
0012ef10 037c3132 chrome_1c30000!WebCore::HTMLElement::createRenderer(class WebCore::RenderArena * arena = 0x019fd380, class WebCore::RenderStyle * style = 0x01b4e000)+0x47 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\html\htmlelement.cpp @ 781]
0012ef28 037c329d chrome_1c30000!WebCore::NodeRendererFactory::createRenderer(void)+0x22 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\dom\noderenderingcontext.cpp @ 351]
0012ef44 03788396 chrome_1c30000!WebCore::NodeRendererFactory::createRendererIfNeeded(void)+0xfd [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\dom\noderenderingcontext.cpp @ 400]
0012ef6c 03791506 chrome_1c30000!WebCore::Node::createRendererIfNeeded(void)+0x16 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\dom\node.cpp @ 1427]
0012ef7c 03739036 chrome_1c30000!WebCore::Element::attach(void)+0x16 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\dom\element.cpp @ 977]
0012ef80 03739b80 chrome_1c30000!WebCore::executeTask(struct WebCore::HTMLConstructionSiteTask * task = 0x01a3411c)+0x66 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\html\parser\htmlconstructionsite.cpp @ 103]
0012efb4 0372f5f2 chrome_1c30000!WebCore::HTMLConstructionSite::executeQueuedTasks(void)+0x60 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\html\parser\htmlconstructionsite.cpp @ 140]
0012efc4 0372f820 chrome_1c30000!WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken(class WebCore::AtomicHTMLToken * token = 0x0012efd8)+0xb2 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\html\parser\htmltreebuilder.cpp @ 482]
0012effc 03710c49 chrome_1c30000!WebCore::HTMLTreeBuilder::constructTreeFromToken(class WebCore::HTMLToken * rawToken = 0x019e805c)+0x30 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\html\parser\htmltreebuilder.cpp @ 461]
0012f03c 03710fc9 chrome_1c30000!WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode mode = AllowYield (0))+0x119 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp @ 278]

It looks like the code already knows about the issue as we NULL-check for containingBlock() in RenderImage::imageDimensionsChanged.

[Julien Chaffraix]

Created attachment 140785 [details]
Proposed fix: bail out as that's what we were already doing (but forgot the repainting logic).

[Eric Seidel (no email)]

Comment on attachment 140785 [details]
Proposed fix: bail out as that's what we were already doing (but forgot the repainting logic).

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

> Source/WebCore/rendering/RenderImage.cpp:197
> +    if (!containingBlock())

Why is it safe to leave this w/o setting setNeedsLayout(false)?

[Julien Chaffraix]

Comment on attachment 140785 [details]
Proposed fix: bail out as that's what we were already doing (but forgot the repainting logic).

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

>> Source/WebCore/rendering/RenderImage.cpp:197
>> +    if (!containingBlock())
> 
> Why is it safe to leave this w/o setting setNeedsLayout(false)?

I assume you mean setNeedsLayout(true) here as there is no requirement for imageDimensionsChanged to call setNeedsLayout(false).

If you don't have a containingBlock() that means you are unrooted. The addChild machinery will take care of setting the needs layout bits later so we don't need to do that now.

[Eric Seidel (no email)]

Comment on attachment 140785 [details]
Proposed fix: bail out as that's what we were already doing (but forgot the repainting logic).

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

>>> Source/WebCore/rendering/RenderImage.cpp:197
>>> +    if (!containingBlock())
>> 
>> Why is it safe to leave this w/o setting setNeedsLayout(false)?
> 
> I assume you mean setNeedsLayout(true) here as there is no requirement for imageDimensionsChanged to call setNeedsLayout(false).
> 
> If you don't have a containingBlock() that means you are unrooted. The addChild machinery will take care of setting the needs layout bits later so we don't need to do that now.

No, I was confused and thought I was reading a layout() method, since you're calling computedLogicalWidth/ehgith below, which are layout() subroutines. :)

> Source/WebCore/rendering/RenderImage.cpp:208
> +        // lets see if we need to relayout at all..
> +        int oldwidth = width();
> +        int oldheight = height();
> +        if (!preferredLogicalWidthsDirty())
> +            setPreferredLogicalWidthsDirty(true);
> +        computeLogicalWidth();
> +        computeLogicalHeight();

This feels really odd to do outside of layout.  I guess this is trying to avoid crawling the whole tree to layout this one image.  I really have trouble imagining that this is saving us time, since doesn't calling setPreferredLogicalWidthsDirty walk up the tree anyway?
    void setPreferredLogicalWidthsDirty(bool, MarkingBehavior = MarkContainingBlockChain);

So this is going to end up causing a layout regardless, no?  Or at least a pre-widths calc?

[Julien Chaffraix]

Comment on attachment 140785 [details]
Proposed fix: bail out as that's what we were already doing (but forgot the repainting logic).

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

>> Source/WebCore/rendering/RenderImage.cpp:208
>> +        computeLogicalHeight();
> 
> This feels really odd to do outside of layout.  I guess this is trying to avoid crawling the whole tree to layout this one image.  I really have trouble imagining that this is saving us time, since doesn't calling setPreferredLogicalWidthsDirty walk up the tree anyway?
>     void setPreferredLogicalWidthsDirty(bool, MarkingBehavior = MarkContainingBlockChain);
> 
> So this is going to end up causing a layout regardless, no?  Or at least a pre-widths calc?

I think this code is fine, though a bit twisted.

As we have changed our preferred logical size, we need to mark our ancestors as needing preferred logical widths recalculation. setPreferredLogicalWidthsDirty(true) doesn't trigger a relayout though and we only for a layout if we have actually changed our size below. Those checks are saving us a full layout if we end up not changing our logical width or height.

[Eric Seidel (no email)]

Comment on attachment 140785 [details]
Proposed fix: bail out as that's what we were already doing (but forgot the repainting logic).

OK.

[Julien Chaffraix]

Thanks Eric!

[WebKit Review Bot]

Comment on attachment 140785 [details]
Proposed fix: bail out as that's what we were already doing (but forgot the repainting logic).

Clearing flags on attachment: 140785

Committed r116693: <http://trac.webkit.org/changeset/116693>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/11522851>