Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2154473004:
Add a sanity DCHECK when constructing a UIResourceRequest. (Closed)
|
Created:
4 years, 5 months ago by Donn Denman Modified:
4 years, 5 months ago Reviewers:
vmpstr CC:
chromium-reviews, cc-bugs_chromium.org, mdjones, Theresa Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a sanity DCHECK when constructing a UIResourceRequest.
Adds a DCHECK that we're actually deleting a resource when no bitmap
is specified in the constructor.
BUG=562353
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
Committed: https://crrev.com/8aa5fa8bd2cb17b294ad00bfbe98502b30c4e738
Cr-Commit-Position: refs/heads/master@{#405906}
Patch Set 1 #Patch Set 2 : Added a check that we're deleting when no bitmap is passed to the constructor. #
Total comments: 6
Patch Set 3 : Removed DCHECKS on references, fixed the resource DCHECK. #
Total comments: 2
Patch Set 4 : Removed qualifier that was not needed. #Messages
Total messages: 17 (8 generated)
Description was changed from ========== Add DCHECKs to UI resource processing to catch corruption. Adds a DCHECK for a null reference when creating a UIResourceRequest or processing them from the request queue. Hopefully it will help to catch whatever corruption is happening earlier. BUG=562353 ========== to ========== Add DCHECKs to UI resource processing to catch corruption. Adds a DCHECK for a null reference when creating a UIResourceRequest or processing them from the request queue. Hopefully it will help to catch whatever corruption is happening earlier. BUG=562353 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ==========
donnd@chromium.org changed reviewers: + vmpstr@chromium.org
Vlad, PTAL, and let me know if you think those other DCHECKS are not advisable.
Description was changed from ========== Add DCHECKs to UI resource processing to catch corruption. Adds a DCHECK for a null reference when creating a UIResourceRequest or processing them from the request queue. Hopefully it will help to catch whatever corruption is happening earlier. BUG=562353 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ========== to ========== Add DCHECKs to UI resource processing to catch corruption. Adds a DCHECK that we're actually deleting a resource when no bitmap is specified in the constructor. Adds a DCHECK for a null reference when creating a UIResourceRequest or processing them from the request queue. Hopefully it will help to catch whatever corruption is happening earlier. BUG=562353 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ==========
https://chromiumcodereview.appspot.com/2154473004/diff/20001/cc/resources/ui_... File cc/resources/ui_resource_request.cc (right): https://chromiumcodereview.appspot.com/2154473004/diff/20001/cc/resources/ui_... cc/resources/ui_resource_request.cc:14: DCHECK(type == UIResourceRequestType.DELETE); This should be UI_RESOURCE_DELETE, right? from https://cs.chromium.org/chromium/src/cc/resources/ui_resource_request.h?q=UIR... https://chromiumcodereview.appspot.com/2154473004/diff/20001/cc/trees/layer_t... File cc/trees/layer_tree_host_impl.cc (right): https://chromiumcodereview.appspot.com/2154473004/diff/20001/cc/trees/layer_t... cc/trees/layer_tree_host_impl.cc:3684: DCHECK(&bitmap); This shouldn't be here. We should be DCHECKing at the time we dereference the pointer if anything. https://chromiumcodereview.appspot.com/2154473004/diff/20001/cc/trees/layer_t... File cc/trees/layer_tree_impl.cc (right): https://chromiumcodereview.appspot.com/2154473004/diff/20001/cc/trees/layer_t... cc/trees/layer_tree_impl.cc:1487: DCHECK(&req); This looks wrong. I think we can assume that the object exists.
Description was changed from ========== Add DCHECKs to UI resource processing to catch corruption. Adds a DCHECK that we're actually deleting a resource when no bitmap is specified in the constructor. Adds a DCHECK for a null reference when creating a UIResourceRequest or processing them from the request queue. Hopefully it will help to catch whatever corruption is happening earlier. BUG=562353 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ========== to ========== Add a sanity DCHECK when constructing a UIResourceRequest. Adds a DCHECK that we're actually deleting a resource when no bitmap is specified in the constructor. BUG=562353 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ==========
Vlad, PTAL. Thanks for your help, and sorry for the sloppy work. https://chromiumcodereview.appspot.com/2154473004/diff/20001/cc/resources/ui_... File cc/resources/ui_resource_request.cc (right): https://chromiumcodereview.appspot.com/2154473004/diff/20001/cc/resources/ui_... cc/resources/ui_resource_request.cc:14: DCHECK(type == UIResourceRequestType.DELETE); On 2016/07/15 21:29:23, vmpstr wrote: > This should be UI_RESOURCE_DELETE, right? from > https://cs.chromium.org/chromium/src/cc/resources/ui_resource_request.h?q=UIR... Gee, how did that even compile? I'm pretty sure I compiled and tested, but this makes me think that didn't get done. How sloppy! Thanks for the catch. Done. https://chromiumcodereview.appspot.com/2154473004/diff/20001/cc/trees/layer_t... File cc/trees/layer_tree_host_impl.cc (right): https://chromiumcodereview.appspot.com/2154473004/diff/20001/cc/trees/layer_t... cc/trees/layer_tree_host_impl.cc:3684: DCHECK(&bitmap); On 2016/07/15 21:29:23, vmpstr wrote: > This shouldn't be here. We should be DCHECKing at the time we dereference the > pointer if anything. Removed. https://chromiumcodereview.appspot.com/2154473004/diff/20001/cc/trees/layer_t... File cc/trees/layer_tree_impl.cc (right): https://chromiumcodereview.appspot.com/2154473004/diff/20001/cc/trees/layer_t... cc/trees/layer_tree_impl.cc:1487: DCHECK(&req); On 2016/07/15 21:29:23, vmpstr wrote: > This looks wrong. I think we can assume that the object exists. Removed.
lgtm thanks! https://chromiumcodereview.appspot.com/2154473004/diff/40001/cc/resources/ui_... File cc/resources/ui_resource_request.cc (right): https://chromiumcodereview.appspot.com/2154473004/diff/40001/cc/resources/ui_... cc/resources/ui_resource_request.cc:14: DCHECK(type == UIResourceRequestType::UI_RESOURCE_DELETE); nit: I don't think you need UIResourceRequestType::
On 2016/07/15 22:10:24, vmpstr wrote: > lgtm thanks! > > https://chromiumcodereview.appspot.com/2154473004/diff/40001/cc/resources/ui_... > File cc/resources/ui_resource_request.cc (right): > > https://chromiumcodereview.appspot.com/2154473004/diff/40001/cc/resources/ui_... > cc/resources/ui_resource_request.cc:14: DCHECK(type == > UIResourceRequestType::UI_RESOURCE_DELETE); > nit: I don't think you need UIResourceRequestType:: Done. Thanks!
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2154473004/#ps60001 (title: "Removed qualifier that was not needed.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://chromiumcodereview.appspot.com/2154473004/diff/40001/cc/resources/ui_... File cc/resources/ui_resource_request.cc (right): https://chromiumcodereview.appspot.com/2154473004/diff/40001/cc/resources/ui_... cc/resources/ui_resource_request.cc:14: DCHECK(type == UIResourceRequestType::UI_RESOURCE_DELETE); On 2016/07/15 22:10:24, vmpstr wrote: > nit: I don't think you need UIResourceRequestType:: Done.
Message was sent while issue was closed.
Description was changed from ========== Add a sanity DCHECK when constructing a UIResourceRequest. Adds a DCHECK that we're actually deleting a resource when no bitmap is specified in the constructor. BUG=562353 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ========== to ========== Add a sanity DCHECK when constructing a UIResourceRequest. Adds a DCHECK that we're actually deleting a resource when no bitmap is specified in the constructor. BUG=562353 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add a sanity DCHECK when constructing a UIResourceRequest. Adds a DCHECK that we're actually deleting a resource when no bitmap is specified in the constructor. BUG=562353 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ========== to ========== Add a sanity DCHECK when constructing a UIResourceRequest. Adds a DCHECK that we're actually deleting a resource when no bitmap is specified in the constructor. BUG=562353 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel Committed: https://crrev.com/8aa5fa8bd2cb17b294ad00bfbe98502b30c4e738 Cr-Commit-Position: refs/heads/master@{#405906} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8aa5fa8bd2cb17b294ad00bfbe98502b30c4e738 Cr-Commit-Position: refs/heads/master@{#405906} |
