Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1910663002:
Fix ImageUploadTaskImpl with null dependencies. (Closed)
DescriptionFix ImageUploadTaskImpl with null dependencies.
We were adding empty TileTasks to a vector, which led to a null
dereference later. We should have only been adding non-null values.
BUG=605193
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/1535bf509aed790aaf00d9a00754978275e04f95
Cr-Commit-Position: refs/heads/master@{#389341}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : feedback #
Total comments: 4
Patch Set 4 : rebase #Patch Set 5 : sk_sp rebase #Messages
Total messages: 36 (17 generated)
Description was changed from ========== Fix ImageUploadTaskImpl with null dependencies. We were adding empty TileTasks to a vector, which led to a null dereference later. We should have only been adding non-null values. BUG=605193 ========== to ========== Fix ImageUploadTaskImpl with null dependencies. We were adding empty TileTasks to a vector, which led to a null dereference later. We should have only been adding non-null values. BUG=605193 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
ericrk@chromium.org changed reviewers: + vmpstr@chromium.org
lgtm https://codereview.chromium.org/1910663002/diff/20001/cc/tiles/gpu_image_deco... File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/1910663002/diff/20001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_controller.cc:113: if (decode_dependency) { nit: no braces Also, the unittest makes it clear, but can you add a short comment to explain when this might be null?
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1910663002/diff/20001/cc/tiles/gpu_image_deco... File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/1910663002/diff/20001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_controller.cc:113: if (decode_dependency) { On 2016/04/20 22:26:44, vmpstr wrote: > nit: no braces > > Also, the unittest makes it clear, but can you add a short comment to explain > when this might be null? Done.
The CQ bit was checked by ericrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/1910663002/#ps60001 (title: "feedback")
The CQ bit was unchecked by ericrk@chromium.org
The CQ bit was checked by ericrk@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1900953004 Patch 120001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
prashant.n@samsung.com changed reviewers: + prashant.n@samsung.com
lgtm Add DCHECK in tile manager. Lgtm once you check the comments. https://codereview.chromium.org/1910663002/diff/60001/cc/tiles/gpu_image_deco... File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/1910663002/diff/60001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_controller.cc:115: if (decode_dependency) Add DCHECK(decode_dependency) here. https://codereview.chromium.org/1910663002/diff/60001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_controller.cc:247: this, draw_image, GetImageDecodeTaskAndRef(draw_image, prepare_tiles_id), Should we check here decode_task = GetImageDecodeTaskAndRef(); if (!decode_task) { RefImage(draw_image); *task = nullptr; return; } So that we don't need to add upload task itself.
The CQ bit was checked by ericrk@chromium.org
https://codereview.chromium.org/1910663002/diff/60001/cc/tiles/gpu_image_deco... File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/1910663002/diff/60001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_controller.cc:115: if (decode_dependency) On 2016/04/21 21:57:51, prashant.n wrote: > Add DCHECK(decode_dependency) here. We can have legitimate cases where an upload task doesn't need a decode dependency - in cases where we just need to upload data, but it was already decoded. https://codereview.chromium.org/1910663002/diff/60001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_controller.cc:247: this, draw_image, GetImageDecodeTaskAndRef(draw_image, prepare_tiles_id), On 2016/04/21 21:57:51, prashant.n wrote: > Should we check here > > decode_task = GetImageDecodeTaskAndRef(); > > if (!decode_task) { > RefImage(draw_image); > *task = nullptr; > return; > } > > So that we don't need to add upload task itself. We actually still need the upload task. In this case, we have a decode dependency that is already decoded/locked, but still needs to be uploaded (the upload, but not the decode, was cancelled in a previous iteration).
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1900953004 Patch 120001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
> We actually still need the upload task. In this case, we have a decode > dependency that is already decoded/locked, but still needs to be uploaded (the > upload, but not the decode, was cancelled in a previous iteration). Do we have only upload tasks without decode dependencies? If yes then your comment is right. I may be wrong but if there is decode task going on then there must be an associated upload task. So for new upload task referring to this decode task is not needed to be added, we just ref the image and return appropriately.
From code it looks like, we don't create any orphaned decode tasks.
On 2016/04/22 12:37:42, prashant.n wrote: > From code it looks like, we don't create any orphaned decode tasks. Pls. ignore my comment. I missed - (the upload, but not the decode, was cancelled in a previous iteration).
> (the upload, but not the decode, was cancelled in a previous iteration). One query, if upload task is not cancelled, would there be 2 upload tasks? Anyways while uploading we check not to upload again, but it would be good if not appended that task.
On 2016/04/22 16:28:40, prashant.n wrote: > > (the upload, but not the decode, was cancelled in a previous iteration). > > One query, if upload task is not cancelled, would there be 2 upload tasks? > Anyways while uploading we check not to upload again, but it would be good if > not appended that task. If an upload task is not cancelled, we should find and return the existing task.
The CQ bit was checked by ericrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, prashant.n@samsung.com Link to the patchset: https://codereview.chromium.org/1910663002/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1910663002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1910663002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by ericrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, prashant.n@samsung.com Link to the patchset: https://codereview.chromium.org/1910663002/#ps100001 (title: "sk_sp rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1910663002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1910663002/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fix ImageUploadTaskImpl with null dependencies. We were adding empty TileTasks to a vector, which led to a null dereference later. We should have only been adding non-null values. BUG=605193 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Fix ImageUploadTaskImpl with null dependencies. We were adding empty TileTasks to a vector, which led to a null dereference later. We should have only been adding non-null values. BUG=605193 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/1535bf509aed790aaf00d9a00754978275e04f95 Cr-Commit-Position: refs/heads/master@{#389341} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1535bf509aed790aaf00d9a00754978275e04f95 Cr-Commit-Position: refs/heads/master@{#389341}
Message was sent while issue was closed.
> If an upload task is not cancelled, we should find and return the existing task. Yes. |
