Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2481703004:
Fix the meaning of stop_y (Closed)
DescriptionFix the meaning of stop_y
stop_y means that we should stop exactly at stop_y, so the last row should be
[stop_y - 1, stop_y], not [stop_y, stop_y + 1].
Somehow this misunderstanding didn't trigger any issue until Chrome exercises
SkAAClip with some websites (e.g.,
http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programmes-deux-visions-de-l-amerique_5026444_829254.html).
When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return
nullptr by findRow. Later when that nullptr row is used to findX, the crash
happened.
BUG=chromium:662925, chromium:662776
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004
Committed: https://skia.googlesource.com/skia/+/3ce89dad5bc324dad9c4b77393e16f3bcb7396a7
Patch Set 1 #Patch Set 2 : Fix the meaning of stop_y #
Messages
Total messages: 23 (15 generated)
Description was changed from ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chrome:662905,chrome:662776 ========== to ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chrome:662905,chrome:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ==========
Description was changed from ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chrome:662905,chrome:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ========== to ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chrome:662905,chrome:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ==========
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...
Description was changed from ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chrome:662905,chrome:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ========== to ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chromium:662905,chromium:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ==========
Description was changed from ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chromium:662905,chromium:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ========== to ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chromium:662925,chromium:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ==========
is it clearer to name the field kLastY ?
On 2016/11/07 21:23:10, reed1 wrote: > is it clearer to name the field kLastY ? kLastY and stop_y seem to be similarly misleading to me (kLastY is even a little more misleading to me). But I'm not good at making language decisions. I'll change the name to whatever you think is the most appropriate.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/07 21:27:00, liyuqian wrote: > On 2016/11/07 21:23:10, reed1 wrote: > > is it clearer to name the field kLastY ? > > kLastY and stop_y seem to be similarly misleading to me (kLastY is even a little > more misleading to me). But I'm not good at making language decisions. I'll > change the name to whatever you think is the most appropriate. Hi Mike, I just realized that I sent in ir.fBottom as stop_y so everything should be clear as long as the meaning of ir.fBottom is clear. So the root of my understanding is the meaning of fBottom of SkIRect. I don't think that we want to change the name of that field?
Description was changed from ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chromium:662925,chromium:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ========== to ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chromium:6629252, chromium:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ==========
Description was changed from ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chromium:6629252, chromium:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ========== to ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chromium:662952, chromium:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ==========
Description was changed from ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chromium:662952, chromium:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ========== to ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chromium:662952, chromium:662925, chromium:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ==========
Description was changed from ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chromium:662952, chromium:662925, chromium:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ========== to ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chromium:662925, chromium:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ==========
agreed. fBottom always means the coordinate that is past the valid Y coordinates.
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 ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chromium:662925, chromium:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 ========== to ========== Fix the meaning of stop_y stop_y means that we should stop exactly at stop_y, so the last row should be [stop_y - 1, stop_y], not [stop_y, stop_y + 1]. Somehow this misunderstanding didn't trigger any issue until Chrome exercises SkAAClip with some websites (e.g., http://www.lemonde.fr/elections-americaines/article/2016/11/07/deux-programme...). When we blitter the extra row [stop_y, stop_y + 1], the SkAAClip will return nullptr by findRow. Later when that nullptr row is used to findX, the crash happened. BUG=chromium:662925, chromium:662776 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481703004 Committed: https://skia.googlesource.com/skia/+/3ce89dad5bc324dad9c4b77393e16f3bcb7396a7 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/3ce89dad5bc324dad9c4b77393e16f3bcb7396a7 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
