[Webkit-unassigned] [Bug 83889] Crash in WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x5c.

Tue May 1 02:45:17 PDT 2012

https://bugs.webkit.org/show_bug.cgi?id=83889

--- Comment #20 from Takashi Sakamoto <tasak at google.com>  2012-05-01 02:45:16 PST ---
(In reply to comment #17)
> (From update of attachment 139084 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139084&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Crash in WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x5c.
> 
> Better title.
> s/WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x5c/WebCore::RenderBoxModelObject::paddingLeft
> 
> > Source/WebCore/ChangeLog:9
> > +        RenderScrllbarPart instances, set owningRenderer(creating)/0
> 
> typo RenderScrllbarPart

Thanks. Done.

> > Source/WebCore/rendering/RenderScrollbar.cpp:275
> > +        partRenderer->setParent(0);
> 
> Why do we need to null out parent when we are getting destroyed ?

I found that setParent(0) is needed when I tested the patch by using attached poc.html:

Program received signal SIGSEGV, Segmentation fault.
0x00000000019ce571 in WebCore::RenderObject::removeChild (this=0x7fffe85bbeb8, 
    oldChild=0x7fffe7cad2e8)
    at third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:326
326         ASSERT(children);

I think, the reason why setParent(0) is needed is that class RenderScrollbar is not a renderer class and class ScrollView (not renderer, either) owns an instance of the class RenderScrollbar.

When ScrollView tries to remove its children, i.e. RenderScrollbar instance, a RenderBox owning RenderScrollbarPart instances, might have already tried to destroy RenderScrollbarPart instances, i.e. the RenderBox instance has already removed RenderScrollbarPart instances from its children list. 
If so, when RenderScrollbar tries to destroy RenderScrollbarPart instances, RenderScrollbarPart instances, which were already removed, will try to remove themselves from their parent renderer (RenderObject::willBeDestroy’s behavior). This causes SIGSEGV.

Does this make sense?

> > LayoutTests/scrollbars/scrollbar-iframe-percent-padding-crash-expected.html:1
> > +<!DOCTYPE html>
> 
> Why do we need a ref-test for this. Can we not have a dumpAsText result that test did not crash.

I think, DumpRenderTree doesn't invoke the method,WebKit!WebCore::RenderScrollbarPart::paintIntoRect+0x87.

I tried to find where the paintIntoRect was invoked by using cs.chromium.org. The result was:

(a) RenderScrollbarPart::paintIntoRect (crash)
(b) RenderScrollbar::paint invokes RenderScrollbarPart::paintIntoRect
(c) RenderScrollbarTheme::paintScrollbarBackground,
RenderScrollbarTheme::paintTrackBackground,
RenderScrollbarTheme::paintTrackPiece,
RenderScrollbarTheme::paintButton,
RenderScrollbarTheme::paintThumb invoke RenderScrollbar::paint
(d) ScrollbarThemeGtk.cpp and ScrollbarThemeComposite.cpp's parent method invoke RenderScrollbarTheme::paintScrollbarBackground ...

As ScrollbarTheme is not a renderer, DumpRenderTree cannot take care of RenderScrollbarPart.

> > LayoutTests/scrollbars/scrollbar-iframe-percent-padding-crash.html:16
> > +<iframe contenteditable="false" webkitallowfullscreen="true" marginheight="8833" webkitallowfullscreen="true" marginheight="495px" webkitallowfullscreen="false" id="NNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNN">JJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJ
> 
> Please clean up the fuzzer test. There are lots of unneeded things like long ID, repeated attributes, etc.

Sure. I created a new test from "cluster fuzz reduction".

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.