Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
75133004:
Close duplicated handle on windows. (Closed)
|
Created:
7 years, 1 month ago by dsinclair Modified:
6 years, 11 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionClose duplicated handle on windows.
When setting up the ThreadIdNameManager we need to make a duplicate of the
thread handle on Windows. That handle is currently be leaked as pointed out by
Dr.Memory. This CL closes the handle after the ThreadIdNameManager has removed
the relevant entry.
BUG=315203
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243330
Patch Set 1 : #
Total comments: 4
Patch Set 2 : #
Total comments: 9
Patch Set 3 : Rebase to master #Patch Set 4 : #Patch Set 5 : Rebase to master. #
Total comments: 4
Patch Set 6 : #
Total comments: 4
Patch Set 7 : Rebase to master #Patch Set 8 : #
Total comments: 2
Patch Set 9 : #Messages
Total messages: 25 (0 generated)
jam, zhaoqin, PTAL. I'm still trying to figure out if the telemetry failures are flakes, or issues caused by the CL.
It seems DuplicateHandle is redundant. You only need use it if you want to close a handle in another process. You can test the result by downloading the latest Dr.Memory from http://build.chromium.org/p/client.drmemory/builds/, and run test with Dr.Memory: ./bin/drmemory.exe -handle_leaks_only -- your_test_name https://codereview.chromium.org/75133004/diff/190001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/190001/base/threading/platform_... base/threading/platform_thread_win.cc:85: NULL, This NULL will cause a handle leak. It seems that Windows will create a handle no matter if you pass NULL or not. I think CloseHandle below should just do the trick. You do not need DuplicateHandle here. https://codereview.chromium.org/75133004/diff/190001/base/threading/platform_... base/threading/platform_thread_win.cc:90: CloseHandle(platform_handle); If you use DUPLICATE_CLOSE_SOURCE above in DuplicateHandle, it will close the source handle regardless of any error status returned. So CloseHandle here should fail.
I'm not an owner in base, I suggest brettw
https://codereview.chromium.org/75133004/diff/190001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/190001/base/threading/platform_... base/threading/platform_thread_win.cc:85: NULL, On 2013/11/26 20:35:56, zhaoqin wrote: > This NULL will cause a handle leak. > It seems that Windows will create a handle no matter if you pass NULL or not. I > think CloseHandle below should just do the trick. You do not need > DuplicateHandle here. Done. https://codereview.chromium.org/75133004/diff/190001/base/threading/platform_... base/threading/platform_thread_win.cc:90: CloseHandle(platform_handle); On 2013/11/26 20:35:56, zhaoqin wrote: > If you use DUPLICATE_CLOSE_SOURCE above in DuplicateHandle, it will close the > source handle regardless of any error status returned. So CloseHandle here > should fail. Done.
I am not sure if this is the right place to call close_handle. I would fix it by myself if it is the case. https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... base/threading/platform_thread_win.cc:80: CloseHandle(platform_handle); Actually I am not sure this is the correct solution. The handle is created hereusing DuplicateHandle. Then the newly created handle is passed to ThreadIdNameManager::RegisterThread and added into a list. see src/base/threading/thread_id_name_manager.cc void ThreadIdNameManager::RegisterThread(PlatformThreadHandle::Handle handle, PlatformThreadId id) { AutoLock locked(lock_); thread_id_to_handle_[id] = handle; thread_handle_to_interned_name_[handle] = name_to_interned_name_[kDefaultName]; } So if you close the handle here, the reference in thread_id_to_handle_[id] will be invalid.
https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... base/threading/platform_thread_win.cc:80: CloseHandle(platform_handle); On 2013/11/28 16:07:33, zhaoqin wrote: > Actually I am not sure this is the correct solution. > > The handle is created hereusing DuplicateHandle. Then the newly created handle > is passed to ThreadIdNameManager::RegisterThread and added into a list. > see src/base/threading/thread_id_name_manager.cc > > void ThreadIdNameManager::RegisterThread(PlatformThreadHandle::Handle handle, > PlatformThreadId id) { > AutoLock locked(lock_); > thread_id_to_handle_[id] = handle; > thread_handle_to_interned_name_[handle] = > name_to_interned_name_[kDefaultName]; > } > > So if you close the handle here, the reference in thread_id_to_handle_[id] will > be invalid. Right, but the line immediately above: ThreadIdNameManager::GetInstance()->RemoveName( platform_handle, PlatformThread::CurrentId()); Removes the handle from the maps. It's cleared from the thread_handle_to_interned_name_ map on line 100 and from the thread_id_to_handle_ map on line 110 of thread_id_name_manager.cc.
should CloseHandle be called in ThreadIdNameManager::RemoveName https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... base/threading/platform_thread_win.cc:80: CloseHandle(platform_handle); On 2013/11/28 16:13:40, dsinclair wrote: > On 2013/11/28 16:07:33, zhaoqin wrote: > > Actually I am not sure this is the correct solution. > > > > The handle is created hereusing DuplicateHandle. Then the newly created > handle > > is passed to ThreadIdNameManager::RegisterThread and added into a list. > > see src/base/threading/thread_id_name_manager.cc > > > > void ThreadIdNameManager::RegisterThread(PlatformThreadHandle::Handle handle, > > PlatformThreadId id) { > > AutoLock locked(lock_); > > thread_id_to_handle_[id] = handle; > > thread_handle_to_interned_name_[handle] = > > name_to_interned_name_[kDefaultName]; > > } > > > > So if you close the handle here, the reference in thread_id_to_handle_[id] > will > > be invalid. > > Right, but the line immediately above: > > ThreadIdNameManager::GetInstance()->RemoveName( > platform_handle, > PlatformThread::CurrentId()); > > Removes the handle from the maps. I see, my bad. > > It's cleared from the thread_handle_to_interned_name_ map on line 100 and from > the thread_id_to_handle_ map on line 110 of thread_id_name_manager.cc. In that case, should CloseHandle be called in ThreadIdNameManager::RemoveName right after the handle is removed?
https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... base/threading/platform_thread_win.cc:80: CloseHandle(platform_handle); On 2013/11/28 16:25:41, zhaoqin wrote: > On 2013/11/28 16:13:40, dsinclair wrote: > > On 2013/11/28 16:07:33, zhaoqin wrote: > > > Actually I am not sure this is the correct solution. > > > > > > The handle is created hereusing DuplicateHandle. Then the newly created > > handle > > > is passed to ThreadIdNameManager::RegisterThread and added into a list. > > > see src/base/threading/thread_id_name_manager.cc > > > > > > void ThreadIdNameManager::RegisterThread(PlatformThreadHandle::Handle > handle, > > > PlatformThreadId id) { > > > AutoLock locked(lock_); > > > thread_id_to_handle_[id] = handle; > > > thread_handle_to_interned_name_[handle] = > > > name_to_interned_name_[kDefaultName]; > > > } > > > > > > So if you close the handle here, the reference in thread_id_to_handle_[id] > > will > > > be invalid. > > > > Right, but the line immediately above: > > > > ThreadIdNameManager::GetInstance()->RemoveName( > > platform_handle, > > PlatformThread::CurrentId()); > > > > Removes the handle from the maps. > > I see, my bad. > > > > > It's cleared from the thread_handle_to_interned_name_ map on line 100 and from > > the thread_id_to_handle_ map on line 110 of thread_id_name_manager.cc. > > In that case, should CloseHandle be called in ThreadIdNameManager::RemoveName > right after the handle is removed? I don't think so, ThreadIdNameManager doesn't know the handle should be closed. It just knows it shouldn't have the mapping to it anymore. As well, how to close the handle as it's platform dependent.
> I don't think so, ThreadIdNameManager doesn't know the handle should be closed. > It just knows it shouldn't have the mapping to it anymore. > > As well, how to close the handle as it's platform dependent. I see, thanks, then it LGTM. You may still need others approval to commit the code.
> I don't think so, ThreadIdNameManager doesn't know the handle should be closed. > It just knows it shouldn't have the mapping to it anymore. > > As well, how to close the handle as it's platform dependent. I see, thanks, then it LGTM. You may still need others approval to commit the code.
jar, PTAL for base OWNERS.
This is pretty Windows specific, so I'd like Ricardo's LGTM as well. This does look like a giant leak... and probably should end up going into release branches (assuming this is not a recent issue). https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... base/threading/platform_thread_win.cc:60: DuplicateHandle( Is it worth checking the return value for success? What will the CloseHandle() do if we failed here?
On 2013/11/28 19:01:22, jar wrote: > This is pretty Windows specific, so I'd like Ricardo's LGTM as well. > > This does look like a giant leak... and probably should end up going into > release branches (assuming this is not a recent issue). > > https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... > File base/threading/platform_thread_win.cc (right): > > https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... > base/threading/platform_thread_win.cc:60: DuplicateHandle( > Is it worth checking the return value for success? > > What will the CloseHandle() do if we failed here? according to http://msdn.microsoft.com/en-us/library/windows/desktop/ms724211(v=vs.85).aspx If the application is running under a debugger, the function will throw an exception if it receives either a handle value that is not valid or a pseudo-handle value. This can happen if you close a handle twice, or if you callCloseHandle on a handle returned by the FindFirstFile function instead of calling the FindClose function. Basically, error usually means a wrong/invalid handle is passed in to close.
https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... base/threading/platform_thread_win.cc:58: * thread name mapping. */ Use c++ style comments https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... base/threading/platform_thread_win.cc:60: DuplicateHandle( On 2013/11/28 19:01:22, jar wrote: > Is it worth checking the return value for success? > > What will the CloseHandle() do if we failed here? Yes, check the return value!. Create a win::ScopedHandle after success.
https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... base/threading/platform_thread_win.cc:58: * thread name mapping. */ On 2013/12/02 20:19:37, rvargas wrote: > Use c++ style comments Done. https://codereview.chromium.org/75133004/diff/230001/base/threading/platform_... base/threading/platform_thread_win.cc:60: DuplicateHandle( On 2013/12/02 20:19:37, rvargas wrote: > On 2013/11/28 19:01:22, jar wrote: > > Is it worth checking the return value for success? > > > > What will the CloseHandle() do if we failed here? > > Yes, check the return value!. Create a win::ScopedHandle after success. Done.
I opted out of base reviewing... so you'll need to get a current owner.
brettw for base/OWNERS.
lgtm https://codereview.chromium.org/75133004/diff/310001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/310001/base/threading/platform_... base/threading/platform_thread_win.cc:61: GetCurrentProcess(), This indentation is a bit weird. I'd do them all aligned after the ( on DuplicateHandle in this case. https://codereview.chromium.org/75133004/diff/310001/base/threading/platform_... base/threading/platform_thread_win.cc:83: This addition seems unnecessary.
rvargas, PTAL. https://codereview.chromium.org/75133004/diff/310001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/310001/base/threading/platform_... base/threading/platform_thread_win.cc:61: GetCurrentProcess(), On 2013/12/12 21:58:29, brettw wrote: > This indentation is a bit weird. I'd do them all aligned after the ( on > DuplicateHandle in this case. Done. https://codereview.chromium.org/75133004/diff/310001/base/threading/platform_... base/threading/platform_thread_win.cc:83: On 2013/12/12 21:58:29, brettw wrote: > This addition seems unnecessary. Done.
https://codereview.chromium.org/75133004/diff/320001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/320001/base/threading/platform_... base/threading/platform_thread_win.cc:67: NOTREACHED(); This is a nop on release builds so this code could still close a random handle on the next line. I guess you cannot continue after this point, so either crash (if that's really what you want), return here, or avoid registration of this thread. https://codereview.chromium.org/75133004/diff/320001/base/threading/platform_... base/threading/platform_thread_win.cc:73: platform_handle, it doesn't matter much, but the "correct" thing at this point is to use scoped_platform_handle.Get() because that's the current owner of the handle.
https://codereview.chromium.org/75133004/diff/320001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/320001/base/threading/platform_... base/threading/platform_thread_win.cc:67: NOTREACHED(); On 2013/12/14 02:29:55, rvargas wrote: > This is a nop on release builds so this code could still close a random handle > on the next line. > > I guess you cannot continue after this point, so either crash (if that's really > what you want), return here, or avoid registration of this thread. Done. https://codereview.chromium.org/75133004/diff/320001/base/threading/platform_... base/threading/platform_thread_win.cc:73: platform_handle, On 2013/12/14 02:29:55, rvargas wrote: > it doesn't matter much, but the "correct" thing at this point is to use > scoped_platform_handle.Get() because that's the current owner of the handle. Done.
lgtm after the nit. https://codereview.chromium.org/75133004/diff/390001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/390001/base/threading/platform_... base/threading/platform_thread_win.cc:60: BOOL didDup = DuplicateHandle(GetCurrentProcess(), nit: did_dup
https://codereview.chromium.org/75133004/diff/390001/base/threading/platform_... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/75133004/diff/390001/base/threading/platform_... base/threading/platform_thread_win.cc:60: BOOL didDup = DuplicateHandle(GetCurrentProcess(), On 2014/01/07 01:15:39, rvargas wrote: > nit: did_dup Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dsinclair@chromium.org/75133004/440001
Message was sent while issue was closed.
Change committed as 243330 |
