Created attachment 75299 [details] Patch The bug report against chrome: http://crbug.com/57706
<rdar://problem/8718962>
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.