Summary: | Progressbar rendering crash | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sadrul Habib Chowdhury <sadrul> | ||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | adele, commit-queue, eric, inferno, tkent, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||
URL: | http://crbug.com/57706 | ||||||||||||||||||||
Attachments: |
|
Description
Sadrul Habib Chowdhury
2010-12-01 12:00:57 PST
This is not security. Null ptr crashes are not security bugs. We dont have security labels on the chromium bug either. Attachment 75299 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/rendering/RenderThemeChromiumSkia.cpp']" exit_code: 1
WebCore/rendering/RenderThemeChromiumSkia.cpp:844: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 75387 [details]
Fixed the style.
Comment on attachment 75299 [details]
Patch
You can use webkit-patch upload to uplaod patches and it will automatically obsolete previous patches for you.
Comment on attachment 75387 [details] Fixed the style. View in context: https://bugs.webkit.org/attachment.cgi?id=75387&action=review > WebCore/ChangeLog:13 > + No new tests. (OOPS!) This will block the commit-queue. Is this not testable? Please replace this line with a description of what tests this affects. > WebCore/rendering/RenderThemeChromiumSkia.cpp:844 > + if (width < 1) It seems like you just want std::min here. std::max I mean. Created attachment 75619 [details]
Updated patch to use std::max. Add a layouttest
PS: I am still figuring out how to use webkit-patch with non-sequential series of commits. So for now, I am uploading the patch manually.
Comment on attachment 75619 [details] Updated patch to use std::max. Add a layouttest View in context: https://bugs.webkit.org/attachment.cgi?id=75619&action=review Sadrul, thank you for the fix. > WebCore/rendering/RenderThemeChromiumSkia.cpp:-843 > - IntSize valueTileSize(static_cast<int>(valueImage->width() * tileScale), valueRect.height()); How about to move this computation out of if() and add the "0 < valueTileSize.width()" to that if's condition ? It looks basically the same kind of check. Comment on attachment 75619 [details] Updated patch to use std::max. Add a layouttest View in context: https://bugs.webkit.org/attachment.cgi?id=75619&action=review >> WebCore/rendering/RenderThemeChromiumSkia.cpp:-843 >> - IntSize valueTileSize(static_cast<int>(valueImage->width() * tileScale), valueRect.height()); > > How about to move this computation out of if() and add the "0 < valueTileSize.width()" to that if's condition ? > It looks basically the same kind of check. Using it as a condition for this block of code ends up not showing anything at all for the progress-bar. Using a lower-bound value of 1 for the width at least shows a very tiny progress bar. I think that's better? Comment on attachment 75619 [details] Updated patch to use std::max. Add a layouttest View in context: https://bugs.webkit.org/attachment.cgi?id=75619&action=review > LayoutTests/ChangeLog:6 > + Layouttest for progress element crash with style. > + http://crbug.com/57706 You should put the WebKit bug URL. > LayoutTests/fast/dom/HTMLProgressElement/progress-element-with-style-crash.html:13 > + window.setTimeout(function() { > + if (window.layoutTestController) > + layoutTestController.notifyDone(); > + }, 1000); Do we really need this 1000 msec delay? It mean this test takes at least 1000 msec. > WebCore/ChangeLog:5 > + Reviewed by NOBODY (OOPS!). > + > + Adjust the width of the tile to be non-zero. You should put a 1-line summary of the bug and the WebKit bug URL. See other ChangeLog entries. Comment on attachment 75619 [details] Updated patch to use std::max. Add a layouttest View in context: https://bugs.webkit.org/attachment.cgi?id=75619&action=review Thanks for reviews :) >> LayoutTests/ChangeLog:6 >> + http://crbug.com/57706 > > You should put the WebKit bug URL. Done. >> LayoutTests/fast/dom/HTMLProgressElement/progress-element-with-style-crash.html:13 >> + }, 1000); > > Do we really need this 1000 msec delay? It mean this test takes at least 1000 msec. A timeout is necessary to trigger the crash. I have reduced the timeout to 10 msec (it reliably reproduces the crash for me so the fix can be verified). >> WebCore/ChangeLog:5 >> + Adjust the width of the tile to be non-zero. > > You should put a 1-line summary of the bug and the WebKit bug URL. See other ChangeLog entries. Done. Created attachment 75900 [details]
Address reviews
Addressed the comments and updated the patch.
Comment on attachment 75900 [details] Address reviews View in context: https://bugs.webkit.org/attachment.cgi?id=75900&action=review > LayoutTests/ChangeLog:8 > + The scaled tile width can be very small at times (e.g. with 'style: font 1 > + required'). So use a minimum width of 1 instead of using 0 (which leads to a > + crash). > + https://bugs.webkit.org/show_bug.cgi?id=50341 Please make this synchronized with WebCore/ChangeLog > LayoutTests/fast/dom/HTMLProgressElement/progress-element-with-style-crash.html:13 > + window.setTimeout(function() { > + if (window.layoutTestController) > + layoutTestController.notifyDone(); > + }, 10); Would you add a comment about the reason of setTimeout()? Regarding the comment for setTimeout, should I add the comment in the test-file, or here as a comment? (In reply to comment #15) > Regarding the comment for setTimeout, should I add the comment in the test-file, or here as a comment? Please add a comment to progress-element-with-style-crash.html. Created attachment 75986 [details]
comments and changelog
Done. Thanks for reviewing.
Comment on attachment 75986 [details]
comments and changelog
ok
The commit-queue encountered the following flaky tests while processing attachment 75986 [details]: java/lc3/JSObject/ToObject-001.html Please file bugs against the tests. These tests were authored by ap@webkit.org. The commit-queue is continuing to process your patch. Comment on attachment 75986 [details] comments and changelog Rejecting patch 75986 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: sts/websocket/tests/workers ...... http/tests/workers ..... http/tests/xhtmlmp . http/tests/xmlhttprequest ............................................................................................................................................................................ http/tests/xmlhttprequest/web-apps ............... http/tests/xmlhttprequest/workers ......... 642.79s total testing time 22152 test cases (99%) succeeded 1 test case (<1%) was new 12 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/6980001 Comment on attachment 75986 [details] comments and changelog View in context: https://bugs.webkit.org/attachment.cgi?id=75986&action=review > LayoutTests/ChangeLog:13 > + * platform/chromium-linux/fast/dom/HTMLProgressElement/progress-element-with-style-crash-expected.txt: Added. This is platform-independent. It should be put to LayoutTests/fast/dom/HTMLProgressElement/. > LayoutTests/fast/dom/HTMLProgressElement/progress-element-with-style-crash.html:20 > + <h1>Test for <a href="https://crbug.com/57706">Chromium bug 57706</a>. It is not OK to crash.</h1> Do not have a link to crbug.com. It should be a link to bugs.webkit.org. Comment on attachment 75986 [details] comments and changelog View in context: https://bugs.webkit.org/attachment.cgi?id=75986&action=review >> LayoutTests/ChangeLog:13 >> + * platform/chromium-linux/fast/dom/HTMLProgressElement/progress-element-with-style-crash-expected.txt: Added. > > This is platform-independent. It should be put to LayoutTests/fast/dom/HTMLProgressElement/. Done. >> LayoutTests/fast/dom/HTMLProgressElement/progress-element-with-style-crash.html:20 >> + <h1>Test for <a href="https://crbug.com/57706">Chromium bug 57706</a>. It is not OK to crash.</h1> > > Do not have a link to crbug.com. It should be a link to bugs.webkit.org. Done. Created attachment 76014 [details]
updated
Comment on attachment 76014 [details]
updated
I found no changes since the previous patch.
Created attachment 76015 [details]
updated
My bad. Sorry about that. Uploading the correct patch.
Comment on attachment 76015 [details]
updated
ok
The commit-queue encountered the following flaky tests while processing attachment 76015 [details]: accessibility/aria-activedescendant-crash.html fast/dom/HTMLProgressElement/progress-element-with-style-crash.html Please file bugs against the tests. These tests were authored by adele@apple.com and cfleizach@apple.com. The commit-queue is continuing to process your patch. Comment on attachment 76015 [details] updated Rejecting patch 76015 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: t ... fast/dom/HTMLMetaElement . fast/dom/HTMLMeterElement ............ fast/dom/HTMLObjectElement .... fast/dom/HTMLObjectElement/form . fast/dom/HTMLOptionElement .... fast/dom/HTMLOutputElement ..... fast/dom/HTMLProgressElement .... fast/dom/HTMLProgressElement/progress-element-with-style-crash.html -> failed Exiting early after 1 failures. 7241 tests run. 191.46s total testing time 7240 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 3 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/6948007 (In reply to comment #28) > fast/dom/HTMLProgressElement/progress-element-with-style-crash.html -> failed I tried the test locally on SnowLeopard. The result diff: --- /tmp/layout-test-results/fast/dom/HTMLProgressElement/progress-element-with-style-crash-expected.txt 2010-12-09 22:31:19.000000000 +0900 +++ /tmp/layout-test-results/fast/dom/HTMLProgressElement/progress-element-with-style-crash-actual.txt 2010-12-09 22:31:19.000000000 +0900 @@ -1 +1,3 @@ Test for Bug 50341. It is not OK to crash. + + Created attachment 76095 [details]
blank lines
Added the blank lines at the end in the expected file.
Comment on attachment 76095 [details]
blank lines
ok.
But how did you make the expected results in the prior patches?
I created the expected results manually. Strangely enough, the tests pass for me with and without the blank lines at the end. (On linux, I do not know if that may be relevant) Comment on attachment 76095 [details] blank lines Rejecting patch 76095 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 1 ERROR: Working directory has local commits, pass --force-clean to continue. Full output: http://queues.webkit.org/results/6972022 Comment on attachment 76095 [details] blank lines bug 50766 hopefully fixed this cq error. Comment on attachment 76095 [details] blank lines Clearing flags on attachment: 76095 Committed r73685: <http://trac.webkit.org/changeset/73685> All reviewed patches have been landed. Closing bug. |