Summary: | Crash in WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x5c. | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mario Gomes <netfuzzer> | ||||||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
Status: | RESOLVED DUPLICATE | ||||||||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, eric, inferno, mitz, simon.fraser, tasak, webkit-bug-importer, webkit.review.bot | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||
Bug Depends on: | 86199 | ||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||
Attachments: |
|
Description
Mario Gomes
2012-04-13 06:53:05 PDT
Created attachment 137080 [details]
poc.html
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. 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. Created attachment 138138 [details]
Patch
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. Created attachment 138208 [details]
Patch
(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"? (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). 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? Created attachment 138412 [details]
Patch
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.
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. Created attachment 138690 [details]
cluster fuzz reduction
Created attachment 139084 [details]
Patch
(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. 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. Created attachment 139601 [details]
Patch
Created attachment 139602 [details]
Patch
(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". 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 ?. 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. Created attachment 139764 [details]
Patch
(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. ----- (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. > > > > 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(); } Comment on attachment 139764 [details]
Patch
Almost there. Just needs the fix for the test as per c#26.
Created attachment 140471 [details]
Patch
(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? (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. Takashi, do you want to use commit-queue for this ? (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. Comment on attachment 140471 [details]
Patch
It's possible to set commit-queue=? via the website too. :)
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 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. :) Created attachment 140901 [details]
Patch
(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. Comment on attachment 140901 [details] Patch Clearing flags on attachment: 140901 Committed r116527: <http://trac.webkit.org/changeset/116527> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 86199 *** This bug has been marked as a duplicate of bug 86162 *** |