[webkit-reviews] review denied: [Bug 83889] Crash in WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x5c. : [Attachment 139084] Patch

Sun Apr 29 01:11:04 PDT 2012

Abhishek Arya <inferno at chromium.org> has denied Takashi Sakamoto
<tasak at google.com>'s request for review:
Bug 83889: Crash in WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x5c.
https://bugs.webkit.org/show_bug.cgi?id=83889

Attachment 139084: Patch
https://bugs.webkit.org/attachment.cgi?id=139084&action=review

------- Additional Comments from Abhishek Arya <inferno at chromium.org>
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::RenderBoxMode
lObject::paddingLeft

> Source/WebCore/ChangeLog:9
> +	   RenderScrllbarPart instances, set owningRenderer(creating)/0

typo RenderScrllbarPart

> Source/WebCore/rendering/RenderScrollbar.cpp:275
> +	   partRenderer->setParent(0);

Why do we need to null out parent when we are getting destroyed ?

> 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.

> LayoutTests/scrollbars/scrollbar-iframe-percent-padding-crash.html:16
> +<iframe contenteditable="false" webkitallowfullscreen="true"
marginheight="8833" webkitallowfullscreen="true" marginheight="495px"
webkitallowfullscreen="false"
id="NNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNN
NNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNN">JJJ
JJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJ

Please clean up the fuzzer test. There are lots of unneeded things like long
ID, repeated attributes, etc.