Bug 83889 – Crash in WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x5c.

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

[Mario Gomes]

Tested On Windows 7 SP1
Apple Safari 5.1.5, Webkit Nightly r112531 and Google Chrome 20.0.1096.1 dev-m

Reproduce:
1. Open poc.html.
2. Wait...
3. See the crash.

After the crash, we have a CALL, but seems just a null ptr.

Stacktrace(Symbols from Nightly Build r112531)
==================
(1360.1500): Access violation - code c0000005 (!!! second chance !!!)
eax=00000000 ebx=00000000 ecx=7f560c40 edx=00000000 esi=7f504e00 edi=00000000
eip=53dac0dc esp=0022eb64 ebp=0022eb78 iopl=0         nv up ei pl zr na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010246
WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x5c:
53dac0dc 8b10            mov     edx,dword ptr [eax]  ds:0023:00000000=????????
0:000> .exr -1
ExceptionAddress: 53dac0dc (WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x0000005c)
   ExceptionCode: c0000005 (Access violation)
  ExceptionFlags: 00000000
NumberParameters: 2
   Parameter[0]: 00000000
   Parameter[1]: 00000000
Attempt to read from address 00000000
0:000> .lastevent
Last event: 1360.1500: Access violation - code c0000005 (!!! second chance !!!)
  debugger time: Fri Apr 13 10:45:45.771 2012 (UTC - 3:00)
0:000> k
ChildEBP RetAddr  
0022eb78 53e0c065 WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x5c [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderboxmodelobject.cpp @ 583]
0022ec3c 53e11e1e WebKit!WebCore::RenderBoxModelObject::paintFillLayerExtended+0x365 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderboxmodelobject.cpp @ 761]
0022ec7c 53e1680b WebKit!WebCore::RenderBox::paintFillLayers+0x5e [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderbox.cpp @ 1123]
0022ecac 53e18ceb WebKit!WebCore::RenderBox::paintBackground+0xcb [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderbox.cpp @ 1030]
0022ed24 53e341a5 WebKit!WebCore::RenderBox::paintBoxDecorations+0x11b [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderbox.cpp @ 1007]
0022ed64 53e1bbb5 WebKit!WebCore::RenderBlock::paintObject+0x45 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderblock.cpp @ 2887]
0022ed98 53d90597 WebKit!WebCore::RenderBlock::paint+0x105 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderblock.cpp @ 2658]
0022ede8 53dd27f5 WebKit!WebCore::RenderScrollbarPart::paintIntoRect+0x87 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderscrollbarpart.cpp @ 178]
0022ee0c 542202d5 WebKit!WebCore::RenderScrollbarTheme::paintButton+0x55 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderscrollbartheme.cpp @ 128]
0022eec4 54205865 WebKit!WebCore::ScrollbarThemeComposite::paint+0x255 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\platform\scrollbarthemecomposite.cpp @ 90]
0022eef4 53e1af1e WebKit!WebCore::Scrollbar::paint+0x75 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\platform\scrollbar.cpp @ 198]
0022ef0c 54078c25 WebKit!WebCore::RenderScrollbar::paint+0x2e [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderscrollbar.cpp @ 115]
0022ef1c 54020a6c WebKit!WebCore::ScrollView::paintScrollbar+0x15 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\platform\scrollview.cpp @ 983]
0022ef64 540795c7 WebKit!WebCore::FrameView::paintScrollbar+0x8c [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\page\frameview.cpp @ 2781]
0022ef90 5407ac49 WebKit!WebCore::ScrollView::paintScrollbars+0x67 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\platform\scrollview.cpp @ 1006]
0022effc 53e2ba0c WebKit!WebCore::ScrollView::paint+0x2b9 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\platform\scrollview.cpp @ 1077]
0022f090 53e1f05c WebKit!WebCore::RenderWidget::paint+0x20c [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderwidget.cpp @ 301]
0022f1cc 53e3dd2d WebKit!WebCore::RenderLayer::paintLayerContents+0x47c [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderlayer.cpp @ 2940]
0022f1f8 53e3e483 WebKit!WebCore::RenderLayerBacking::paintIntoLayer+0x7d [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderlayerbacking.cpp @ 1144]
0022f24c 541df3b7 WebKit!WebCore::RenderLayerBacking::paintContents+0x1a3 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderlayerbacking.cpp @ 1177]
0022f288 54385a3a WebKit!WebCore::GraphicsLayer::paintGraphicsLayerContents+0x87 [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\platform\graphics\graphicslayer.cpp @ 300]
0022f5ec 541eb0bc WebKit!WebCore::PlatformCALayerWinInternal::displayCallback+0x11a [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\platform\graphics\ca\win\platformcalayerwininternal.cpp @ 87]
*** ERROR: Symbol file could not be found.  Defaulted to export symbols for C:\Program Files\Safari\Apple Application Support\QuartzCore.dll - 
0022f5fc 5a97c37f WebKit!displayCallback+0x1c [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\platform\graphics\ca\win\platformcalayerwin.cpp @ 102]
WARNING: Stack unwind information not available. Following frames may be wrong.
0022f690 5a97c481 QuartzCore!CACFLayerLayoutSublayers+0xef
0022f6bc 5a97c1c8 QuartzCore!CACFLayerDisplay+0xf1
*** ERROR: Symbol file could not be found.  Defaulted to export symbols for C:\Program Files\Safari\Apple Application Support\CoreFoundation.dll - 
0022f6d4 5b481c32 QuartzCore!CACFLayerSetDisplayCallback+0x38
00000000 00000000 CoreFoundation!CFArrayGetValueAtIndex+0x4c
0:000> !load winext/msec.dll
0:000> !exploitable
Exploitability Classification: PROBABLY_EXPLOITABLE
Recommended Bug Title: Probably Exploitable - Data from Faulting Address controls Code Flow starting at WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x000000000000005c (Hash=0x37195059.0x2d2a5760)

The data from the faulting address is later used as the target for a branch.
0:000> u
WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x5c [c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\renderboxmodelobject.cpp @ 583]:
53dac0dc 8b10            mov     edx,dword ptr [eax]
53dac0de 8bc8            mov     ecx,eax
53dac0e0 8b8230030000    mov     eax,dword ptr [edx+330h]
53dac0e6 ffd0            call    eax
53dac0e8 8bf8            mov     edi,eax
53dac0ea eb14            jmp     WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x80 (53dac100)
53dac0ec 0fb6c1          movzx   eax,cl
53dac0ef 83c0f9          add     eax,0FFFFFFF9h

[Mario Gomes]

Created attachment 137080 [details]
poc.html

[Abhishek Arya]

I dont understand why are filing these null ptr bugs as security bugs. First you filed it in chromium land and now in webkit land.

[Mario Gomes]

I'm so sorry about this. I thought that was reported as a bug.

(In reply to comment #2)
> I dont understand why are filing these null ptr bugs as security bugs. First you filed it in chromium land and now in webkit land.

[Radar WebKit Bug Importer]

<rdar://problem/11244432>

[Takashi Sakamoto]

Created attachment 138138 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 138138 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        No new tests, because no behavior change.

But there is a behavior change, since this is fixing a crash.

[Takashi Sakamoto]

Created attachment 138208 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #6)
> (From update of attachment 138138 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138138&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        No new tests, because no behavior change.
> 
> But there is a behavior change, since this is fixing a crash.

Sorry. I agree with you.
However it is difficult to reproduce this crash by using DumpRenderTree.
I tried poc.html, but DumpRenderTree normally exists.
(It is easy to see crash by using chrome.)

So I would like to know what is the best (at least, allowable) way to do in this case. Is it ok to "No new tests"?

[Takashi Sakamoto]

(In reply to comment #7)
> Created an attachment (id=138208) [details]
> Patch

I recreate and uploaded patch again.

As class RenderScrollbar creates class RenderScrollbarPart instances without any render parent, padding left cannot get valid containingBlock.

I think, there are (at least) two ways to fix this issue:
(1) RenderScrollbar, use setParent to set RenderScrollbarPart's parent renderer.
   (needs to make RenderScrollbar RenderScrollbarPart's friend class)
(2) RenderBoxModelObject, check whether containingBlock exists or not when calculating paddingXXX. If not, use 0 as containingBlock width / height.

The recreated patch is created according to (1). And previous patch is created according to (2).

[Tim Horton]

Comment on attachment 138208 [details]
Patch

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

> Source/WebCore/rendering/RenderScrollbarPart.h:71
> +    friend class RenderScrollbar;

I don't have the code here so I'm not 100%, but don't we usually put friends at the top of the class?

[Takashi Sakamoto]

Created attachment 138412 [details]
Patch

[Hajime Morrita]

Comment on attachment 138412 [details]
Patch

You can layoutTestController.waitUntilDone() to prevent DRT from exiting.
When a series of animations is finished. you can call layoutTestController.notifyDone() to
finish the test. I guess that will help it to reproduce on DRT.

[Julien Chaffraix]

Comment on attachment 138412 [details]
Patch

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

r- on the premises that it should be tested.

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

typo: RenderScrollbarPart.

> Source/WebCore/ChangeLog:14
> +        No new tests.

OK, normally you would explain here why you couldn't make a DRT test case out of the posted one (timing dependent or whatever the explination). Not providing at test case is frown upon and most reviewers will just r- your patch based on that.

Now, it's your lucky day as it turns out that cluster fuzz has a reduced test case for that. I will post it.

[Julien Chaffraix]

Created attachment 138690 [details]
cluster fuzz reduction

[Takashi Sakamoto]

Created attachment 139084 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #13)
> (From update of attachment 138412 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138412&action=review
> 
> r- on the premises that it should be tested.
> 
> > Source/WebCore/ChangeLog:9
> > +        RenderScrllbarPart instances, set owningRenderer(creating)/0
> 
> typo: RenderScrollbarPart.
> 
> > Source/WebCore/ChangeLog:14
> > +        No new tests.
> 
> OK, normally you would explain here why you couldn't make a DRT test case out of the posted one (timing dependent or whatever the explination). Not providing at test case is frown upon and most reviewers will just r- your patch based on that.
> 
> Now, it's your lucky day as it turns out that cluster fuzz has a reduced test case for that. I will post it.

Thank you.

I think, DumpRenderTree doesn't render any scrollbars, i.e. doesn't call WebCore::RenderScrollbarPart::paintIntRect(). I used gdb and added breakpoints to the method.
However I found that ref test can easily reproduce this crash. 

So I added one new ref test to LayoutTests/scrollbars.

[Abhishek Arya]

Comment on attachment 139084 [details]
Patch

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

> 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="NNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNN">JJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJJ

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

[Takashi Sakamoto]

Created attachment 139601 [details]
Patch

[Takashi Sakamoto]

Created attachment 139602 [details]
Patch

[Takashi Sakamoto]

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

[Abhishek Arya]

Comment on attachment 139602 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Crash in WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x5c.

You forgot to fix the title. Same for Layouttests/Changelog.

> LayoutTests/scrollbars/scrollbar-percent-padding-crash.html:1
> +<style>

I could not reproduce this ref-test failure without your patch. Did it work for you ?.

[Julien Chaffraix]

Comment on attachment 139602 [details]
Patch

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

The code change is fine.

>> LayoutTests/scrollbars/scrollbar-percent-padding-crash.html:1
>> +<style>
> 
> I could not reproduce this ref-test failure without your patch. Did it work for you ?.

For the record, I don't really like a ref-test that is really not one. Have the 2 files (test & reference) be exactly the same doesn't make a good ref-test as any change would still make the test pass. This should be a dumpAsText(true) output as it would at least catch up variation on the pixel output. If that makes the output platform specific, we may need to find another way.

> LayoutTests/scrollbars/scrollbar-percent-padding-crash.html:13
> +</script>
> +<div style="height: 1000px;">

Test *should* have the following information:
* bug title
* bug id or URL (I prefer URL in a clickable format)
* explain what they expect

Without those information, the test is at most ambiguous and it will be difficult for any maintainer to know what is expected and if any variation is expected.

[Takashi Sakamoto]

Created attachment 139764 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #21)
> (From update of attachment 139602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139602&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Crash in WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x5c.
> 
> You forgot to fix the title. Same for Layouttests/Changelog.

Thank you. I fixed.

> > LayoutTests/scrollbars/scrollbar-percent-padding-crash.html:1
> > +<style>
> 
> I could not reproduce this ref-test failure without your patch. Did it work for you ?.

I would like to confirm what happens. Did you mean that my patch causes new failure? 

I ran ./webkit/tools/layout_tests/run_webkit_tests.sh --debug scrollbars/scrollbar-percent-padding-crash\* on master branch (I copied the ref-test) and obtained the following:
-----
  scrollbars/scrollbar-percent-padding-crash.html -> unexpected crash
0 tests ran as expected, 1 didn't:

Regressions: Unexpected crashes : (1)
  scrollbars/scrollbar-percent-padding-crash.html = CRASH
-----

And after applying the patch, I ran the same command and obtained the following:
-----
All 1 tests ran as expected.
-----

[Takashi Sakamoto]

(In reply to comment #22)
> (From update of attachment 139602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139602&action=review
> 
> The code change is fine.
> 
> >> LayoutTests/scrollbars/scrollbar-percent-padding-crash.html:1
> >> +<style>
> > 
> > I could not reproduce this ref-test failure without your patch. Did it work for you ?.
> 
> For the record, I don't really like a ref-test that is really not one. Have the 2 files (test & reference) be exactly the same doesn't make a good ref-test as any change would still make the test pass. This should be a dumpAsText(true) output as it would at least catch up variation on the pixel output. If that makes the output platform specific, we may need to find another way.

I would like to use dumpAsText. However I don't know how to enable both dumpPixels and dumpAsText. To reproduce the crash, I think, "dumpPixels" is needed.
The following is a DumpRenderTree's crash log:
-----
crash log for DumpRenderTree (pid 27006):
STDOUT: <empty>
STDERR: [27006:27006:105850062961:ERROR:process_util_posix.cc(143)] Received signal 11
STDERR: 	base::debug::StackTrace::StackTrace() [0x86d2aa]
STDERR: 	base::(anonymous namespace)::StackDumpSignalHandler() [0x82dec9]
STDERR: 	0x7f57dd470af0
STDERR: 	WebCore::RenderBoxModelObject::computedCSSPaddingLeft() [0x194088a]
STDERR: 	WebCore::RenderBoxModelObject::paddingLeft() [0x106ed72]
.... snip ....
STDERR: 	WebKit::WebViewImpl::paint() [0x4d0d62]
STDERR: 	WebViewHost::paintRect() [0x481215]
STDERR: 	WebViewHost::paintInvalidatedRegion() [0x48149c]
STDERR: 	TestShell::dump() [0x46cec0]
STDERR: 	TestShell::testFinished() [0x46b6ca]
STDERR: 	LayoutTestController::WorkQueue::processWorkSoon() [0x44a9c0]
STDERR: 	LayoutTestController::locationChangeDone() [0x44bef1]
STDERR: 	WebViewHost::locationChangeDone() [0x480708]
STDERR: 	WebViewHost::didFinishLoad() [0x47d986]
-----

The "m_webViewHost->paintInvalidatedRegion();" is invoked only when m_params.dumpPixels && shouldGeneratePixelResults. To enable dumpPixels, I don't know any other way except to use ref-test, i.e. imagediff. 

I would like to ask your idea.

> 
> > LayoutTests/scrollbars/scrollbar-percent-padding-crash.html:13
> > +</script>
> > +<div style="height: 1000px;">
> 
> Test *should* have the following information:
> * bug title
> * bug id or URL (I prefer URL in a clickable format)
> * explain what they expect
> 
> Without those information, the test is at most ambiguous and it will be difficult for any maintainer to know what is expected and if any variation is expected.

I see. I added.

[Abhishek Arya]

> > 
> > I could not reproduce this ref-test failure without your patch. Did it work for you ?.
> 
> I would like to confirm what happens. Did you mean that my patch causes new failure? 
> 
> I ran ./webkit/tools/layout_tests/run_webkit_tests.sh --debug scrollbars/scrollbar-percent-padding-crash\* on master branch (I copied the ref-test) and obtained the following:
> -----
>   scrollbars/scrollbar-percent-padding-crash.html -> unexpected crash
> 0 tests ran as expected, 1 didn't:
> 
> Regressions: Unexpected crashes : (1)
>   scrollbars/scrollbar-percent-padding-crash.html = CRASH
> -----
> 
> And after applying the patch, I ran the same command and obtained the following:
> -----
> All 1 tests ran as expected.
> -----

I was testing on webkit mac, looks like this is only reproducible through the chromium pipeline. I was able to reproduce this on chromium linux. Btw Julien's trick of dumpAsText(true) works and it does cause the crash. dumpAsText(true) triggers the pixel result generation, keeping the result as still text. e.g. http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/LayoutTests/css3/flexbox/repaint.html&exact_package=chromium&q=%22dumpAstext(true%22&type=cs&l=44. you don't need a ref test.

void LayoutTestController::dumpAsText(const CppArgumentList& arguments, CppVariant* result)
{
    m_dumpAsText = true;
    m_generatePixelResults = false;

    // Optional paramater, describing whether it's allowed to dump pixel results in dumpAsText mode.
    if (arguments.size() > 0 && arguments[0].isBool())
        m_generatePixelResults = arguments[0].value.boolValue;

    result->setNull();
}

[Abhishek Arya]

Comment on attachment 139764 [details]
Patch

Almost there. Just needs the fix for the test as per c#26.

[Takashi Sakamoto]

Created attachment 140471 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #26)
> > > 
> > > I could not reproduce this ref-test failure without your patch. Did it work for you ?.
> > 
> > I would like to confirm what happens. Did you mean that my patch causes new failure? 
> > 
> > I ran ./webkit/tools/layout_tests/run_webkit_tests.sh --debug scrollbars/scrollbar-percent-padding-crash\* on master branch (I copied the ref-test) and obtained the following:
> > -----
> >   scrollbars/scrollbar-percent-padding-crash.html -> unexpected crash
> > 0 tests ran as expected, 1 didn't:
> > 
> > Regressions: Unexpected crashes : (1)
> >   scrollbars/scrollbar-percent-padding-crash.html = CRASH
> > -----
> > 
> > And after applying the patch, I ran the same command and obtained the following:
> > -----
> > All 1 tests ran as expected.
> > -----
> 
> I was testing on webkit mac, looks like this is only reproducible through the chromium pipeline. I was able to reproduce this on chromium linux. Btw Julien's trick of dumpAsText(true) works and it does cause the crash. dumpAsText(true) triggers the pixel result generation, keeping the result as still text. e.g. http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/LayoutTests/css3/flexbox/repaint.html&exact_package=chromium&q=%22dumpAstext(true%22&type=cs&l=44. you don't need a ref test.
> 
> void LayoutTestController::dumpAsText(const CppArgumentList& arguments, CppVariant* result)
> {
>     m_dumpAsText = true;
>     m_generatePixelResults = false;
> 
>     // Optional paramater, describing whether it's allowed to dump pixel results in dumpAsText mode.
>     if (arguments.size() > 0 && arguments[0].isBool())
>         m_generatePixelResults = arguments[0].value.boolValue;
> 
>     result->setNull();
> }

Thank you for information.

I looked at repaint.html, tested dumpAsText(true) and found that dumpAsText(true) requires a PNG image used as an expected result. I'm afraid that adding an expected image is too much for testing this issue.
However I found that layoutTestController.display() can invoke the method: WebCore::RenderScrollbarPart::paintIntoRect(). So I modified the test in the following way:
- added layoutTestController.dumpAsText() and layoutTestController.display(), and
- added *-expected.txt and removed *-expected.html.

Does this look ok?  Or is it better to add an expected PNG image?

[Abhishek Arya]

(In reply to comment #29)
> (In reply to comment #26)
> > > > 
> > > > I could not reproduce this ref-test failure without your patch. Did it work for you ?.
> > > 
> > > I would like to confirm what happens. Did you mean that my patch causes new failure? 
> > > 
> > > I ran ./webkit/tools/layout_tests/run_webkit_tests.sh --debug scrollbars/scrollbar-percent-padding-crash\* on master branch (I copied the ref-test) and obtained the following:
> > > -----
> > >   scrollbars/scrollbar-percent-padding-crash.html -> unexpected crash
> > > 0 tests ran as expected, 1 didn't:
> > > 
> > > Regressions: Unexpected crashes : (1)
> > >   scrollbars/scrollbar-percent-padding-crash.html = CRASH
> > > -----
> > > 
> > > And after applying the patch, I ran the same command and obtained the following:
> > > -----
> > > All 1 tests ran as expected.
> > > -----
> > 
> > I was testing on webkit mac, looks like this is only reproducible through the chromium pipeline. I was able to reproduce this on chromium linux. Btw Julien's trick of dumpAsText(true) works and it does cause the crash. dumpAsText(true) triggers the pixel result generation, keeping the result as still text. e.g. http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/LayoutTests/css3/flexbox/repaint.html&exact_package=chromium&q=%22dumpAstext(true%22&type=cs&l=44. you don't need a ref test.
> > 
> > void LayoutTestController::dumpAsText(const CppArgumentList& arguments, CppVariant* result)
> > {
> >     m_dumpAsText = true;
> >     m_generatePixelResults = false;
> > 
> >     // Optional paramater, describing whether it's allowed to dump pixel results in dumpAsText mode.
> >     if (arguments.size() > 0 && arguments[0].isBool())
> >         m_generatePixelResults = arguments[0].value.boolValue;
> > 
> >     result->setNull();
> > }
> 
> Thank you for information.
> 
> I looked at repaint.html, tested dumpAsText(true) and found that dumpAsText(true) requires a PNG image used as an expected result. I'm afraid that adding an expected image is too much for testing this issue.
> However I found that layoutTestController.display() can invoke the method: WebCore::RenderScrollbarPart::paintIntoRect(). So I modified the test in the following way:
> - added layoutTestController.dumpAsText() and layoutTestController.display(), and
> - added *-expected.txt and removed *-expected.html.
> 
> Does this look ok?  Or is it better to add an expected PNG image?

This looks ok. With the PNG, it will make the results platform specific, and we don't need it in this case.

[Abhishek Arya]

Takashi, do you want to use commit-queue for this ?

[Takashi Sakamoto]

(In reply to comment #31)
> Takashi, do you want to use commit-queue for this ?

Yes. I missed --request-commit when I uploaded the patch.
I would like to use commit-queue for this.

[Eric Seidel (no email)]

Comment on attachment 140471 [details]
Patch

It's possible to set commit-queue=? via the website too. :)

[WebKit Review Bot]

Comment on attachment 140471 [details]
Patch

Rejecting attachment 140471 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12649592

[Eric Seidel (no email)]

Comment on attachment 140471 [details]
Patch

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

> LayoutTests/ChangeLog:5
> +

Looks like you removed the Reviewed by NOBODY (OOPS!). line from this ChangeLog. :( Sadly the commit-queue depends on this confusing line as a marker for where to insert the reviewer name. :)

[Takashi Sakamoto]

Created attachment 140901 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #35)
> (From update of attachment 140471 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140471&action=review
> 
> > LayoutTests/ChangeLog:5
> > +
> 
> Looks like you removed the Reviewed by NOBODY (OOPS!). line from this ChangeLog. :( Sadly the commit-queue depends on this confusing line as a marker for where to insert the reviewer name. :)

Thank you. I fixed and uploaded a new patch.

[WebKit Review Bot]

Comment on attachment 140901 [details]
Patch

Clearing flags on attachment: 140901

Committed r116527: <http://trac.webkit.org/changeset/116527>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 86199

[Abhishek Arya]

*** This bug has been marked as a duplicate of bug 86162 ***