Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2657003003:
Avoid unneeded refcount bump on disk_cache::File (Closed)
DescriptionAvoid unneeded refcount bump on disk_cache::File
A PostTaskAndReply in disk_cache::File implementation has unneeded
refcount bump, which is currently safe and going to be racy. This CL
removes the count bump to avoid the race on the upcoming change.
base::Callback retains the reference to disk_cache::File, and releases it
on the callback destruction. PostTaskAndReply currently guarantees that it
drops the reference to both `Task` callback and `Reply` callback on the origin
thread. However after a proposed change for PostTaskAndReply, the `Task`
callback will always be destroyed on the target thread of PostTaskAndReply.
That implies that the refcount of disk_cache::File will be dropped on a
worker thread, and causes a race on the refcount bump.
https://codereview.chromium.org/2657603004/
Review-Url: https://codereview.chromium.org/2657003003
Cr-Commit-Position: refs/heads/master@{#446770}
Committed: https://chromium.googlesource.com/chromium/src/+/dc974fef99b96b1a153fa7e7c35949de285c7e67
Patch Set 1 #
Messages
Total messages: 21 (14 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Description was changed from ========== Avoid unneeded refcount bump on disk_cache::File ========== to ========== Avoid unneeded refcount bump on disk_cache::File A PostTaskAndReply in disk_cache::File implementation has unneeded refcount bump, which is currently safe and going to be racy. This CL removes the count bump. base::Callback retains the reference to disk_cache::File, and releases it on the callback destruction. PostTaskAndReply currently guarantees that it drops the reference to both `Task` callback and `Reply` callback on the origin thread. However after a proposed change for PostTaskAndReply, the `Task` callback will always be destroyed on the target thread of PostTaskAndReply. That implies that the refcount of disk_cache::File will be dropped on a worker thread, and causes a race on the refcount bump. ==========
Description was changed from ========== Avoid unneeded refcount bump on disk_cache::File A PostTaskAndReply in disk_cache::File implementation has unneeded refcount bump, which is currently safe and going to be racy. This CL removes the count bump. base::Callback retains the reference to disk_cache::File, and releases it on the callback destruction. PostTaskAndReply currently guarantees that it drops the reference to both `Task` callback and `Reply` callback on the origin thread. However after a proposed change for PostTaskAndReply, the `Task` callback will always be destroyed on the target thread of PostTaskAndReply. That implies that the refcount of disk_cache::File will be dropped on a worker thread, and causes a race on the refcount bump. ========== to ========== Avoid unneeded refcount bump on disk_cache::File A PostTaskAndReply in disk_cache::File implementation has unneeded refcount bump, which is currently safe and going to be racy. This CL removes the count bump to avoid the race on the upcoming change. base::Callback retains the reference to disk_cache::File, and releases it on the callback destruction. PostTaskAndReply currently guarantees that it drops the reference to both `Task` callback and `Reply` callback on the origin thread. However after a proposed change for PostTaskAndReply, the `Task` callback will always be destroyed on the target thread of PostTaskAndReply. That implies that the refcount of disk_cache::File will be dropped on a worker thread, and causes a race on the refcount bump. ==========
Description was changed from ========== Avoid unneeded refcount bump on disk_cache::File A PostTaskAndReply in disk_cache::File implementation has unneeded refcount bump, which is currently safe and going to be racy. This CL removes the count bump to avoid the race on the upcoming change. base::Callback retains the reference to disk_cache::File, and releases it on the callback destruction. PostTaskAndReply currently guarantees that it drops the reference to both `Task` callback and `Reply` callback on the origin thread. However after a proposed change for PostTaskAndReply, the `Task` callback will always be destroyed on the target thread of PostTaskAndReply. That implies that the refcount of disk_cache::File will be dropped on a worker thread, and causes a race on the refcount bump. ========== to ========== Avoid unneeded refcount bump on disk_cache::File A PostTaskAndReply in disk_cache::File implementation has unneeded refcount bump, which is currently safe and going to be racy. This CL removes the count bump to avoid the race on the upcoming change. base::Callback retains the reference to disk_cache::File, and releases it on the callback destruction. PostTaskAndReply currently guarantees that it drops the reference to both `Task` callback and `Reply` callback on the origin thread. However after a proposed change for PostTaskAndReply, the `Task` callback will always be destroyed on the target thread of PostTaskAndReply. That implies that the refcount of disk_cache::File will be dropped on a worker thread, and causes a race on the refcount bump. https://codereview.chromium.org/2657603004/ ==========
tzik@chromium.org changed reviewers: + mmenke@chromium.org
PTAL
mmenke@chromium.org changed reviewers: + jkarlin@chromium.org
[+jkarlin]: Don't suppose you know the blockfile cache well enough to review this?
On 2017/01/26 at 20:11:38, mmenke wrote: > [+jkarlin]: Don't suppose you know the blockfile cache well enough to review this? I'm not familiar with blockfile cache at all but the change lgtm (note that I'm not a net/ owner) since the reply task keeps a reference to this.
The CQ bit was checked by jkarlin@chromium.org 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...
On 2017/01/27 15:39:04, jkarlin wrote: > On 2017/01/26 at 20:11:38, mmenke wrote: > > [+jkarlin]: Don't suppose you know the blockfile cache well enough to review > this? > > I'm not familiar with blockfile cache at all but the change lgtm (note that I'm > not a net/ owner) since the reply task keeps a reference to this. LGTM, deferring to jkarlin
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gavinp@chromium.org changed reviewers: + gavinp@chromium.org
lgtm
The CQ bit was checked by tzik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1485550618950800, "parent_rev":
"89009f5cec91679c6f2b99642825d27a85866100", "commit_rev":
"dc974fef99b96b1a153fa7e7c35949de285c7e67"}
Message was sent while issue was closed.
Description was changed from ========== Avoid unneeded refcount bump on disk_cache::File A PostTaskAndReply in disk_cache::File implementation has unneeded refcount bump, which is currently safe and going to be racy. This CL removes the count bump to avoid the race on the upcoming change. base::Callback retains the reference to disk_cache::File, and releases it on the callback destruction. PostTaskAndReply currently guarantees that it drops the reference to both `Task` callback and `Reply` callback on the origin thread. However after a proposed change for PostTaskAndReply, the `Task` callback will always be destroyed on the target thread of PostTaskAndReply. That implies that the refcount of disk_cache::File will be dropped on a worker thread, and causes a race on the refcount bump. https://codereview.chromium.org/2657603004/ ========== to ========== Avoid unneeded refcount bump on disk_cache::File A PostTaskAndReply in disk_cache::File implementation has unneeded refcount bump, which is currently safe and going to be racy. This CL removes the count bump to avoid the race on the upcoming change. base::Callback retains the reference to disk_cache::File, and releases it on the callback destruction. PostTaskAndReply currently guarantees that it drops the reference to both `Task` callback and `Reply` callback on the origin thread. However after a proposed change for PostTaskAndReply, the `Task` callback will always be destroyed on the target thread of PostTaskAndReply. That implies that the refcount of disk_cache::File will be dropped on a worker thread, and causes a race on the refcount bump. https://codereview.chromium.org/2657603004/ Review-Url: https://codereview.chromium.org/2657003003 Cr-Commit-Position: refs/heads/master@{#446770} Committed: https://chromium.googlesource.com/chromium/src/+/dc974fef99b96b1a153fa7e7c359... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/dc974fef99b96b1a153fa7e7c359... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
