Bug 85912 - Crash in computedCSSPadding* functions due to RenderImage::imageDimensionsChanged called during attachment
Summary: Crash in computedCSSPadding* functions due to RenderImage::imageDimensionsCha...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Julien Chaffraix
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-05-08 14:10 PDT by Julien Chaffraix
Modified: 2012-05-24 00:57 PDT (History)
4 users (show)

See Also:


Attachments
Reproduction (528 bytes, text/html)
2012-05-08 14:10 PDT, Julien Chaffraix
no flags Details
Proposed fix: bail out as that's what we were already doing (but forgot the repainting logic). (8.19 KB, patch)
2012-05-08 14:29 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-05-08 14:10:19 PDT
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.
Comment 1 Julien Chaffraix 2012-05-08 14:29:00 PDT
Created attachment 140785 [details]
Proposed fix: bail out as that's what we were already doing (but forgot the repainting logic).
Comment 2 Eric Seidel (no email) 2012-05-08 15:23:08 PDT
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)?
Comment 3 Julien Chaffraix 2012-05-08 17:21:59 PDT
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.
Comment 4 Eric Seidel (no email) 2012-05-08 17:41:25 PDT
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?
Comment 5 Julien Chaffraix 2012-05-09 09:57:32 PDT
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.
Comment 6 Eric Seidel (no email) 2012-05-10 14:36:14 PDT
Comment on attachment 140785 [details]
Proposed fix: bail out as that's what we were already doing (but forgot the repainting logic).

OK.
Comment 7 Julien Chaffraix 2012-05-10 14:40:01 PDT
Thanks Eric!
Comment 8 WebKit Review Bot 2012-05-10 15:07:47 PDT
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>
Comment 9 WebKit Review Bot 2012-05-10 15:07:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2012-05-24 00:57:20 PDT
<rdar://problem/11522851>