Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2043873005:
Fix for rare crash in Poly::addEdge(). (Closed)
|
Created:
4 years, 6 months ago by Stephen White Modified:
4 years, 6 months ago Reviewers:
bsalomon CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionFix for rare crash in Poly::addEdge().
Don't add an edge if the bottom vertex was already added, or
if an island vertex has a left poly but no right poly.
(Sorry for the lack of test, but the only reduction I could create was still a huge path and only crashes in Chrome.)
BUG=617907
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043873005
Committed: https://skia.googlesource.com/skia/+/93e3fff79eaaa86bc2fb740a42111a074ccc73ab
Patch Set 1 #
Total comments: 2
Patch Set 2 : Cleanup #Patch Set 3 : Revert inadvertent changes #Messages
Total messages: 23 (10 generated)
Description was changed from ========== Fix for rare crash in Poly::addEdge(). Don't add an edge if the bottom vertex was already added, or if an island vertex has a left poly but no right poly. BUG=617907 ========== to ========== Fix for rare crash in Poly::addEdge(). Don't add an edge if the bottom vertex was already added, or if an island vertex has a left poly but no right poly. BUG=617907 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043873005 ==========
Description was changed from ========== Fix for rare crash in Poly::addEdge(). Don't add an edge if the bottom vertex was already added, or if an island vertex has a left poly but no right poly. BUG=617907 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043873005 ========== to ========== Fix for rare crash in Poly::addEdge(). Don't add an edge if the bottom vertex was already added, or if an island vertex has a left poly but no right poly. (Sorry for the lack of test, but the only reduction I could create was still a huge path and only crashes in Chrome.) BUG=617907 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043873005 ==========
senorblanco@chromium.org changed reviewers: + robertphillips@chromium.org
Robert: PTAL. Thanks!
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043873005/1
senorblanco@chromium.org changed reviewers: + bsalomon@google.com - robertphillips@chromium.org
Brian: PTAL. Thanks!
On 2016/06/07 16:03:18, Stephen White wrote: > Brian: PTAL. Thanks! Would the path that crashed Chrome cause a valgrind/MSAN error in a test?
On 2016/06/07 16:07:57, bsalomon wrote: > On 2016/06/07 16:03:18, Stephen White wrote: > > Brian: PTAL. Thanks! > > Would the path that crashed Chrome cause a valgrind/MSAN error in a test? I tried valgrind; no error.
https://codereview.chromium.org/2043873005/diff/1/src/gpu/GrTessellator.cpp File src/gpu/GrTessellator.cpp (right): https://codereview.chromium.org/2043873005/diff/1/src/gpu/GrTessellator.cpp#n... src/gpu/GrTessellator.cpp:421: } else if (side == fTail->fSide) { Would it be clearer to write this as if (!fTail) { fHead = fTail = ALLOC_NEW(MonotonePoly, (e, side), alloc); fCount += 2; } else if (e->fBottom != fTail->fLastEdge->fBottom) { if (side == fTail->fSide) { fTail->addEdge(e); fCount++ } else { e = ALLOC_NEW(Edge, (fTail->fLastEdge->fBottom, e->fBottom, 1), alloc); ... } } return poly; ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
New patch up. PTAL. https://codereview.chromium.org/2043873005/diff/1/src/gpu/GrTessellator.cpp File src/gpu/GrTessellator.cpp (right): https://codereview.chromium.org/2043873005/diff/1/src/gpu/GrTessellator.cpp#n... src/gpu/GrTessellator.cpp:421: } else if (side == fTail->fSide) { On 2016/06/07 16:21:53, bsalomon wrote: > Would it be clearer to write this as > > if (!fTail) { > fHead = fTail = ALLOC_NEW(MonotonePoly, (e, side), alloc); > fCount += 2; > } else if (e->fBottom != fTail->fLastEdge->fBottom) { > if (side == fTail->fSide) { > fTail->addEdge(e); > fCount++ > } else { > e = ALLOC_NEW(Edge, (fTail->fLastEdge->fBottom, e->fBottom, 1), alloc); > ... > } > } > return poly; > > ? Good idea; I've done something similar but with an early-out.
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043873005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by senorblanco@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043873005/40001
Message was sent while issue was closed.
Description was changed from ========== Fix for rare crash in Poly::addEdge(). Don't add an edge if the bottom vertex was already added, or if an island vertex has a left poly but no right poly. (Sorry for the lack of test, but the only reduction I could create was still a huge path and only crashes in Chrome.) BUG=617907 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043873005 ========== to ========== Fix for rare crash in Poly::addEdge(). Don't add an edge if the bottom vertex was already added, or if an island vertex has a left poly but no right poly. (Sorry for the lack of test, but the only reduction I could create was still a huge path and only crashes in Chrome.) BUG=617907 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043873005 Committed: https://skia.googlesource.com/skia/+/93e3fff79eaaa86bc2fb740a42111a074ccc73ab ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/93e3fff79eaaa86bc2fb740a42111a074ccc73ab |
