Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779413002:
Duplicate process handle in process_metrics_win.cc (Closed)
DescriptionDuplicate process handle in process_metrics_win.cc
This is needed to avoid using an invalid handle to update metrics when
there is a race condition with the process terminating.
BUG=702339
Review-Url: https://codereview.chromium.org/2779413002
Cr-Commit-Position: refs/heads/master@{#461237}
Committed: https://chromium.googlesource.com/chromium/src/+/e73f1a4f6e871894fc00f6129b4f1bc0847580cf
Patch Set 1 #
Total comments: 2
Patch Set 2 : Used ScopedHandle #
Messages
Total messages: 27 (17 generated)
The CQ bit was checked by stanisc@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
Description was changed from ========== Duplicate handle in process_metrics_win.cc BUG= ========== to ========== Duplicate process handle in process_metrics_win.cc This is needed to avoid using an invalid handle to update metrics when there is a race condition with the process terminating. BUG=702339 ==========
stanisc@chromium.org changed reviewers: + brucedawson@chromium.org, dcheng@chromium.org
stanisc@chromium.org changed required reviewers: + dcheng@chromium.org
dcheng@, please take a look! brucedawson@ is an optional reviewer.
https://codereview.chromium.org/2779413002/diff/1/base/process/process_metric... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2779413002/diff/1/base/process/process_metric... base/process/process_metrics_win.cc:36: ::CloseHandle(process_); Shouldn't this use a handle wrapper class rather than explicitly closing and initializing the handle?
https://codereview.chromium.org/2779413002/diff/1/base/process/process_metric... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2779413002/diff/1/base/process/process_metric... base/process/process_metrics_win.cc:36: ::CloseHandle(process_); On 2017/03/30 23:09:46, brucedawson wrote: > Shouldn't this use a handle wrapper class rather than explicitly closing and > initializing the handle? It should, but process_ field is defined in the header file which is shared between all platforms.
> It should, but process_ field is defined in the header file which is shared > between all platforms. Is it practical to #ifdef the type in the header file? If not then you should add a comment explaining why you're not using a wrapper class.
On 2017/03/30 23:28:24, brucedawson wrote: > > It should, but process_ field is defined in the header file which is shared > > between all platforms. > > Is it practical to #ifdef the type in the header file? If not then you should > add a comment explaining why you're not using a wrapper class. +1... we should document the ownership in the header in the constructor at least (since it looks like none of the other platforms bother dup'ing the handle -- is it not a problem for other platforms?)
The CQ bit was checked by stanisc@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...
The CQ bit was unchecked by stanisc@chromium.org
The CQ bit was checked by stanisc@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/03/31 01:06:15, dcheng wrote: > On 2017/03/30 23:28:24, brucedawson wrote: > > > It should, but process_ field is defined in the header file which is shared > > > between all platforms. > > > > Is it practical to #ifdef the type in the header file? If not then you should > > add a comment explaining why you're not using a wrapper class. > > +1... we should document the ownership in the header in the constructor at least > (since it looks like none of the other platforms bother dup'ing the handle -- is > it not a problem for other platforms?) Indeed, using a ScopedHandle under #ifdef in the header file is better because that is self-explanatory. I've uploaded a new patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
LGTM
The CQ bit was checked by stanisc@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": 20001, "attempt_start_ts": 1490996200238630,
"parent_rev": "c2cffeaaa19ba03e23cbce4291be97a85e14dd78", "commit_rev":
"e73f1a4f6e871894fc00f6129b4f1bc0847580cf"}
Message was sent while issue was closed.
Description was changed from ========== Duplicate process handle in process_metrics_win.cc This is needed to avoid using an invalid handle to update metrics when there is a race condition with the process terminating. BUG=702339 ========== to ========== Duplicate process handle in process_metrics_win.cc This is needed to avoid using an invalid handle to update metrics when there is a race condition with the process terminating. BUG=702339 Review-Url: https://codereview.chromium.org/2779413002 Cr-Commit-Position: refs/heads/master@{#461237} Committed: https://chromium.googlesource.com/chromium/src/+/e73f1a4f6e871894fc00f6129b4f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/e73f1a4f6e871894fc00f6129b4f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
