Bug 50341 – Progressbar rendering crash

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Sadrul Habib Chowdhury]

Created attachment 75299 [details]
Patch

The bug report against chrome: http://crbug.com/57706

[Lucas Forschler]

<rdar://problem/8718962>

[Abhishek Arya]

This is not security. Null ptr crashes are not security bugs. We dont have security labels on the chromium bug either.

[WebKit Review Bot]

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.

[Sadrul Habib Chowdhury]

Created attachment 75387 [details]
Fixed the style.

[Eric Seidel (no email)]

Comment on attachment 75299 [details]
Patch

You can use webkit-patch upload to uplaod patches and it will automatically obsolete previous patches for you.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

std::max I mean.

[Sadrul Habib Chowdhury]

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.

[Hajime Morrita]

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.

[Sadrul Habib Chowdhury]

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?

[Kent Tamura]

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.

[Sadrul Habib Chowdhury]

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.

[Sadrul Habib Chowdhury]

Created attachment 75900 [details]
Address reviews

Addressed the comments and updated the patch.

[Kent Tamura]

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()?

[Sadrul Habib Chowdhury]

Regarding the comment for setTimeout, should I add the comment in the test-file, or here as a comment?

[Kent Tamura]

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

[Sadrul Habib Chowdhury]

Created attachment 75986 [details]
comments and changelog

Done. Thanks for reviewing.

[Kent Tamura]

Comment on attachment 75986 [details]
comments and changelog

ok

[WebKit Commit Bot]

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 [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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

[Kent Tamura]

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.

[Sadrul Habib Chowdhury]

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.

[Sadrul Habib Chowdhury]

Created attachment 76014 [details]
updated

[Kent Tamura]

Comment on attachment 76014 [details]
updated

I found no changes since the previous patch.

[Sadrul Habib Chowdhury]

Created attachment 76015 [details]
updated

My bad. Sorry about that. Uploading the correct patch.

[Kent Tamura]

Comment on attachment 76015 [details]
updated

ok

[WebKit Commit Bot]

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 [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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

[Kent Tamura]

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

[Sadrul Habib Chowdhury]

Created attachment 76095 [details]
blank lines

Added the blank lines at the end in the expected file.

[Kent Tamura]

Comment on attachment 76095 [details]
blank lines

ok.
But how did you make the expected results in the prior patches?

[Sadrul Habib Chowdhury]

I created the expected results manually. Strangely enough, the tests pass for me with and without the blank lines at the end.

[Sadrul Habib Chowdhury]

(On linux, I do not know if that may be relevant)

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 76095 [details]
blank lines

bug 50766  hopefully fixed this cq error.

[WebKit Commit Bot]

Comment on attachment 76095 [details]
blank lines

Clearing flags on attachment: 76095

Committed r73685: <http://trac.webkit.org/changeset/73685>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.