Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1772343007:
Two HandleVerifier thread safety fixes. (Closed)
|
Created:
4 years, 9 months ago by Will Harris Modified:
4 years, 9 months ago Reviewers:
scottmg CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTwo HandleVerifier thread safety fixes.
Two changes in this CL:
1. Before setting the g_active_verifier, check that another thread hasn't
already set this. This is to cater for the situation where multiple
threads attempt to create the handle verifier in the same module at the
same time. The g_lock was not protecting the check to see if the handle
verifier had already been instantiated by another thread before setting
it. This could result in a handle verifier leaking and some handles
being tracked in the leaked verifier.
2. Always instantiate the handle verifier inside the main module, unless
the main module does not link with base. This means that there should be
always one instance that lives in the main module.
BUG=592753
TEST=base_unittests
Committed: https://crrev.com/678736a103c3e64acdce5e76910ff077b17cc6c3
Cr-Commit-Position: refs/heads/master@{#380581}
Patch Set 1 #
Total comments: 2
Patch Set 2 : no functionality changes #
Total comments: 4
Patch Set 3 : nit #Messages
Total messages: 34 (15 generated)
Description was changed from ========== Two HandleVerifier thread safety fixes. Two changes in this CL: 1. Before setting the g_active_verifier, check that another thread hasn't already set this. This is to cater for the situation where multiple threads attempt to create the handle verifier in the same module at the same time. The g_lock was not protecting the check to see if the handle verifier had already been instantiated by another thread before setting it. This could result in the handle verifier leaking. 2. Always instantiate the handle verifier inside the main module, unless the main module does not link with base. This means that there should be always one instance that lives in the main module. BUG=592753 TEST=base_unittests ========== to ========== Two HandleVerifier thread safety fixes. Two changes in this CL: 1. Before setting the g_active_verifier, check that another thread hasn't already set this. This is to cater for the situation where multiple threads attempt to create the handle verifier in the same module at the same time. The g_lock was not protecting the check to see if the handle verifier had already been instantiated by another thread before setting it. This could result in a handle verifier leaking and some handles being tracked in the leaked verifier. 2. Always instantiate the handle verifier inside the main module, unless the main module does not link with base. This means that there should be always one instance that lives in the main module. BUG=592753 TEST=base_unittests ==========
wfh@chromium.org changed reviewers: + scottmg@chromium.org
I think I'm convinced, lgtm. https://codereview.chromium.org/1772343007/diff/1/base/win/scoped_handle.cc File base/win/scoped_handle.cc (right): https://codereview.chromium.org/1772343007/diff/1/base/win/scoped_handle.cc#n... base/win/scoped_handle.cc:158: if (g_active_verifier) Maybe an extra comment here describing what we discussed about this part.
ps2 should have no functionality changes, just moved the code around. ptal since it's more than just a small change. https://codereview.chromium.org/1772343007/diff/1/base/win/scoped_handle.cc File base/win/scoped_handle.cc (right): https://codereview.chromium.org/1772343007/diff/1/base/win/scoped_handle.cc#n... base/win/scoped_handle.cc:158: if (g_active_verifier) On 2016/03/10 20:31:26, scottmg wrote: > Maybe an extra comment here describing what we discussed about this part. Done.
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1772343007/#ps20001 (title: "no functionality changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772343007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772343007/20001
lgtm https://codereview.chromium.org/1772343007/diff/20001/base/win/scoped_handle.cc File base/win/scoped_handle.cc (right): https://codereview.chromium.org/1772343007/diff/20001/base/win/scoped_handle.... base/win/scoped_handle.cc:118: bool enabled = true) { Is this allowed now? I think just passing true in the one call that doesn't pass true/false is better.
https://codereview.chromium.org/1772343007/diff/20001/base/win/scoped_handle.cc File base/win/scoped_handle.cc (right): https://codereview.chromium.org/1772343007/diff/20001/base/win/scoped_handle.... base/win/scoped_handle.cc:118: bool enabled = true) { On 2016/03/10 22:11:22, scottmg wrote: > Is this allowed now? I think just passing true in the one call that doesn't pass > true/false is better. it's not disallowed, and I see other examples in base/ so I went with it.
https://codereview.chromium.org/1772343007/diff/20001/base/win/scoped_handle.cc File base/win/scoped_handle.cc (right): https://codereview.chromium.org/1772343007/diff/20001/base/win/scoped_handle.... base/win/scoped_handle.cc:118: bool enabled = true) { On 2016/03/10 22:14:23, Will Harris wrote: > On 2016/03/10 22:11:22, scottmg wrote: > > Is this allowed now? I think just passing true in the one call that doesn't > pass > > true/false is better. > > it's not disallowed, and I see other examples in base/ so I went with it. OK, I still don't think there's any utility when 3/4 calls pass an explicit value.
https://codereview.chromium.org/1772343007/diff/20001/base/win/scoped_handle.cc File base/win/scoped_handle.cc (right): https://codereview.chromium.org/1772343007/diff/20001/base/win/scoped_handle.... base/win/scoped_handle.cc:118: bool enabled = true) { On 2016/03/10 22:17:16, scottmg wrote: > On 2016/03/10 22:14:23, Will Harris wrote: > > On 2016/03/10 22:11:22, scottmg wrote: > > > Is this allowed now? I think just passing true in the one call that doesn't > > pass > > > true/false is better. > > > > it's not disallowed, and I see other examples in base/ so I went with it. > > OK, I still don't think there's any utility when 3/4 calls pass an explicit > value. it tickled my OCD passing an ignored value for the case on line 163, but sure I can change it back.
The CQ bit was unchecked by wfh@chromium.org
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1772343007/#ps40001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772343007/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772343007/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wfh@chromium.org
On 2016/03/10 23:08:52, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) not making any changes to linux, this is a flake.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772343007/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772343007/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/03/11 00:17:07, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) why are linux bots failing for a win only change.
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772343007/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772343007/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/03/11 01:08:35, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Why are you lying to me, bot!
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772343007/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772343007/40001
Message was sent while issue was closed.
Description was changed from ========== Two HandleVerifier thread safety fixes. Two changes in this CL: 1. Before setting the g_active_verifier, check that another thread hasn't already set this. This is to cater for the situation where multiple threads attempt to create the handle verifier in the same module at the same time. The g_lock was not protecting the check to see if the handle verifier had already been instantiated by another thread before setting it. This could result in a handle verifier leaking and some handles being tracked in the leaked verifier. 2. Always instantiate the handle verifier inside the main module, unless the main module does not link with base. This means that there should be always one instance that lives in the main module. BUG=592753 TEST=base_unittests ========== to ========== Two HandleVerifier thread safety fixes. Two changes in this CL: 1. Before setting the g_active_verifier, check that another thread hasn't already set this. This is to cater for the situation where multiple threads attempt to create the handle verifier in the same module at the same time. The g_lock was not protecting the check to see if the handle verifier had already been instantiated by another thread before setting it. This could result in a handle verifier leaking and some handles being tracked in the leaked verifier. 2. Always instantiate the handle verifier inside the main module, unless the main module does not link with base. This means that there should be always one instance that lives in the main module. BUG=592753 TEST=base_unittests ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Two HandleVerifier thread safety fixes. Two changes in this CL: 1. Before setting the g_active_verifier, check that another thread hasn't already set this. This is to cater for the situation where multiple threads attempt to create the handle verifier in the same module at the same time. The g_lock was not protecting the check to see if the handle verifier had already been instantiated by another thread before setting it. This could result in a handle verifier leaking and some handles being tracked in the leaked verifier. 2. Always instantiate the handle verifier inside the main module, unless the main module does not link with base. This means that there should be always one instance that lives in the main module. BUG=592753 TEST=base_unittests ========== to ========== Two HandleVerifier thread safety fixes. Two changes in this CL: 1. Before setting the g_active_verifier, check that another thread hasn't already set this. This is to cater for the situation where multiple threads attempt to create the handle verifier in the same module at the same time. The g_lock was not protecting the check to see if the handle verifier had already been instantiated by another thread before setting it. This could result in a handle verifier leaking and some handles being tracked in the leaked verifier. 2. Always instantiate the handle verifier inside the main module, unless the main module does not link with base. This means that there should be always one instance that lives in the main module. BUG=592753 TEST=base_unittests Committed: https://crrev.com/678736a103c3e64acdce5e76910ff077b17cc6c3 Cr-Commit-Position: refs/heads/master@{#380581} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/678736a103c3e64acdce5e76910ff077b17cc6c3 Cr-Commit-Position: refs/heads/master@{#380581} |
