Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2485243002:
Do not call blitV with 0 height (Closed)
DescriptionDo not call blitV with 0 height
This is causing SkAAClipBlitter to crash.
BUG=chromium:662952
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2485243002
Committed: https://skia.googlesource.com/skia/+/c78eff97549e8e346394d3e228395ceb8a467b35
Patch Set 1 #Patch Set 2 : Add unit test #
Messages
Total messages: 23 (11 generated)
Description was changed from ========== Do not call blitV with 0 height This is causing SkAAClipBlitter to crash. BUG=chromium:662952 ========== to ========== Do not call blitV with 0 height This is causing SkAAClipBlitter to crash. BUG=chromium:662952 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2485243002 ==========
Description was changed from ========== Do not call blitV with 0 height This is causing SkAAClipBlitter to crash. BUG=chromium:662952 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2485243002 ========== to ========== Do not call blitV with 0 height This is causing SkAAClipBlitter to crash. BUG=chromium:662952 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2485243002 ==========
liyuqian@google.com changed reviewers: + caryclark@google.com, fmalita@chromium.org, reed@google.com
The CQ bit was checked by liyuqian@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Is it possible to include a test that triggers the old crashing behavior?
On 2016/11/08 21:33:35, caryclark wrote: > lgtm > > Is it possible to include a test that triggers the old crashing behavior? The minimized testcase (see the link below) in chromium:662952 will trigger the old crashing behavior (in content_shell), and it will be fixed by this patch. Minimized testcase: https://cluster-fuzz.appspot.com/download/AMIfv97xNbBofjYWm02kouSSW6FbhM6ZufM...
That's great! But, I've found that the cluster-fuzz test cases disappear after a while. I'd recommend adding the minimum that reproduces the crash to a test somewhere.
On 2016/11/08 21:52:09, caryclark wrote:
> That's great! But, I've found that the cluster-fuzz test cases disappear after
a
> while. I'd recommend adding the minimum that reproduces the crash to a test
> somewhere.
Oh, I don't know that. I love fuzz tests! I thought that Google never delete any
data (the original GMail gave me an impression that Google never ran out of disk
space)... Anyway, the html is really minimal so I'm pasting it below:
<svg class="CLASS4 CLASS11">
<foreignObject class="CLASS0 CLASS4" width="1em" height="1cm">
<style>
.CLASS0{text-decoration:ActiveBorder blink
!important;border-left-color:InactiveCaption;border-style:dotted;text-align:match-parent
'Qj:}Bjdrsd7OT&~7~b9Z\A[petWcqZ^<LZx$QT~cb]|t`bqC}p[hgg%jH\9jO^tlM&R.*}*rFd^E#6{v\A
24t\)YH';}
.CLASS11{text-decoration:currentColor blink;zoom:5%;
I'm suggesting that a test that Skia can run would be the most helpful way to ensure that this bug doesn't accidentally reoccur, with the added bonus of validating that the bug is squashed across multiple platforms. The way I do this in path ops is to modify the fuzzer code to spit out the problematic data to the console, via path.dumpHex(). The trouble with capturing the HTML in this case is that the implementation of SVG in Chrome may change, obscuring the bug.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by liyuqian@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by liyuqian@google.com
On 2016/11/08 22:21:46, caryclark wrote: > I'm suggesting that a test that Skia can run would be the most helpful way to > ensure that this bug doesn't accidentally reoccur, with the added bonus of > validating that the bug is squashed across multiple platforms. > > The way I do this in path ops is to modify the fuzzer code to spit out the > problematic data to the console, via path.dumpHex(). > > The trouble with capturing the HTML in this case is that the implementation of > SVG in Chrome may change, obscuring the bug. Oh, sorry that I missed this reply and almost committed the change. I'll dump the path and add it to Skia unittest tomorrow. BTW, your dumpHex suggestion is really helpful :D
Hi Cary, I've added the unit test. Could you please check if it looks good?
lgtm
The CQ bit was checked by liyuqian@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Do not call blitV with 0 height This is causing SkAAClipBlitter to crash. BUG=chromium:662952 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2485243002 ========== to ========== Do not call blitV with 0 height This is causing SkAAClipBlitter to crash. BUG=chromium:662952 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2485243002 Committed: https://skia.googlesource.com/skia/+/c78eff97549e8e346394d3e228395ceb8a467b35 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/c78eff97549e8e346394d3e228395ceb8a467b35 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
