Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2295983002:
Fix memory leak in ThreadWatcher. (Closed)
|
Created:
4 years, 3 months ago by Joshua LeVasseur Modified:
4 years, 3 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix memory leak in ThreadWatcher.
BUG=none
TEST=unit tests
Committed: https://crrev.com/5ca77e86703646cb84be35ef827068ac6951be1e
Cr-Commit-Position: refs/heads/master@{#415729}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Cleanup ThreadWatcher initialization logic. #
Total comments: 10
Patch Set 3 : Address feedback from isherman in comment #17 #
Total comments: 2
Messages
Total messages: 29 (12 generated)
The CQ bit was checked by jlevasseur@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...
Description was changed from ========== Fix memory leak in ThreadWatcher. BUG=none TEST=unit tests ========== to ========== Fix memory leak in ThreadWatcher. BUG=none TEST=unit tests ==========
jlevasseur@chromium.org changed reviewers: + isherman@chromium.org, rtenneti@chromium.org, wzhong@chromium.org
jlevasseur@chromium.org changed reviewers: + alokp@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This is in preparation for turning ThreadWatcher into a component.
lgtm Thanks for the fix.
https://codereview.chromium.org/2295983002/diff/1/chrome/browser/metrics/thre... File chrome/browser/metrics/thread_watcher.cc (right): https://codereview.chromium.org/2295983002/diff/1/chrome/browser/metrics/thre... chrome/browser/metrics/thread_watcher.cc:91: watcher->ActivateThreadWatching(); Is this leaked?
On 2016/08/30 22:32:34, alokp wrote: > https://codereview.chromium.org/2295983002/diff/1/chrome/browser/metrics/thre... > File chrome/browser/metrics/thread_watcher.cc (right): > > https://codereview.chromium.org/2295983002/diff/1/chrome/browser/metrics/thre... > chrome/browser/metrics/thread_watcher.cc:91: watcher->ActivateThreadWatching(); > Is this leaked? No, in that case, it is registered with the ThreadWatcherList, which happens via the constructor of ThreadWatcher.
https://codereview.chromium.org/2295983002/diff/1/chrome/browser/metrics/thre... File chrome/browser/metrics/thread_watcher.cc (right): https://codereview.chromium.org/2295983002/diff/1/chrome/browser/metrics/thre... chrome/browser/metrics/thread_watcher.cc:91: watcher->ActivateThreadWatching(); On 2016/08/30 22:32:34, alokp wrote: > Is this leaked? watcher is stored in g_thread_watcher_list_. It is deleted in ThreadWatcherList::DeleteAll(). IsRegistered returns false if g_thread_watcher_list_ is null. This is the case when browser was shutting down even before we started watching threads. We wanted threads to stabilize during browser startup (they would be busy and threads may not be very responsive) before we start watching them (I think it 2 minutes after startup). If browser were to shutdown within 2 minutes and if g_thread_watcher_list_is already deleted, then the above case could occur. Writing from memory (so, I could be wrong).
https://codereview.chromium.org/2295983002/diff/1/chrome/browser/metrics/thre... File chrome/browser/metrics/thread_watcher.cc (right): https://codereview.chromium.org/2295983002/diff/1/chrome/browser/metrics/thre... chrome/browser/metrics/thread_watcher.cc:91: watcher->ActivateThreadWatching(); On 2016/08/30 22:45:12, ramant wrote: > On 2016/08/30 22:32:34, alokp wrote: > > Is this leaked? > > watcher is stored in g_thread_watcher_list_. It is deleted in > ThreadWatcherList::DeleteAll(). > > IsRegistered returns false if g_thread_watcher_list_ is null. > > This is the case when browser was shutting down even before we started watching > threads. > > We wanted threads to stabilize during browser startup (they would be busy and > threads may not be very responsive) before we start watching them (I think it 2 > minutes after startup). > > If browser were to shutdown within 2 minutes and if g_thread_watcher_list_is > already deleted, then the above case could occur. > > Writing from memory (so, I could be wrong). Thanks for the explanation. I think a comment about this would be nice.
lgtm
https://codereview.chromium.org/2295983002/diff/1/chrome/browser/metrics/thre... File chrome/browser/metrics/thread_watcher.cc (right): https://codereview.chromium.org/2295983002/diff/1/chrome/browser/metrics/thre... chrome/browser/metrics/thread_watcher.cc:88: delete watcher; Could you instead modify |watcher| to be a unique_ptr, and call ThreadWatcherList::Register() immediately after constructing the watcher, rather than as part of its constructor? Moreover, if you update ThreadWatcherList::Register() to return a boolean, I think you can get rid of ThreadWatcherList::IsRegistered() and ThreadWatcherList::Find(). I think the net result would be less, and easier-to-follow, code =)
On 2016/08/30 23:51:57, Ilya Sherman wrote: > https://codereview.chromium.org/2295983002/diff/1/chrome/browser/metrics/thre... > File chrome/browser/metrics/thread_watcher.cc (right): > > https://codereview.chromium.org/2295983002/diff/1/chrome/browser/metrics/thre... > chrome/browser/metrics/thread_watcher.cc:88: delete watcher; > Could you instead modify |watcher| to be a unique_ptr, and call > ThreadWatcherList::Register() immediately after constructing the watcher, rather > than as part of its constructor? Moreover, if you update > ThreadWatcherList::Register() to return a boolean, I think you can get rid of > ThreadWatcherList::IsRegistered() and ThreadWatcherList::Find(). I think the > net result would be less, and easier-to-follow, code =) Done. I modified it nearly as you suggested, but the unit tests need ThreadWatcherList::Find() to be kept around.
Thanks! LGTM % some nits: https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/thread_watcher.cc (right): https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/thread_watcher.cc:88: } nit: Please revert the added curly braces. https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/thread_watcher.h (right): https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/thread_watcher.h:141: virtual ~ThreadWatcher(); nit: Please move this to the top of the class definition: https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/thread_watcher.h:397: // It takes ownership of the ThreadWatcher. Returns true if it was nit: No need to document the ownership transfer -- that's clear just from the use of unique_ptr. https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/thread_watcher_unittest.cc (right): https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/thread_watcher_unittest.cc:286: EXPECT_TRUE(ThreadWatcherList::Register(std::move(io_watcher))); nit: This should be an ASSERT_TRUE, as the following line would crash if the watcher were somehow not registered. https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/thread_watcher_unittest.cc:294: EXPECT_TRUE(ThreadWatcherList::Register(std::move(db_watcher))); Ditto.
https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/thread_watcher.cc (right): https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/thread_watcher.cc:88: } On 2016/08/31 03:54:42, Ilya Sherman wrote: > nit: Please revert the added curly braces. Done. https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/thread_watcher.h (right): https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/thread_watcher.h:141: virtual ~ThreadWatcher(); On 2016/08/31 03:54:42, Ilya Sherman wrote: > nit: Please move this to the top of the class definition: > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/thread_watcher.h:397: // It takes ownership of the ThreadWatcher. Returns true if it was On 2016/08/31 03:54:42, Ilya Sherman wrote: > nit: No need to document the ownership transfer -- that's clear just from the > use of unique_ptr. Done. https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/thread_watcher_unittest.cc (right): https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/thread_watcher_unittest.cc:286: EXPECT_TRUE(ThreadWatcherList::Register(std::move(io_watcher))); On 2016/08/31 03:54:42, Ilya Sherman wrote: > nit: This should be an ASSERT_TRUE, as the following line would crash if the > watcher were somehow not registered. Done. https://codereview.chromium.org/2295983002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/thread_watcher_unittest.cc:294: EXPECT_TRUE(ThreadWatcherList::Register(std::move(db_watcher))); On 2016/08/31 03:54:42, Ilya Sherman wrote: > Ditto. Done.
The CQ bit was checked by jlevasseur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wzhong@chromium.org, rtenneti@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2295983002/#ps40001 (title: "Address feedback from isherman in comment #17")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix memory leak in ThreadWatcher. BUG=none TEST=unit tests ========== to ========== Fix memory leak in ThreadWatcher. BUG=none TEST=unit tests ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix memory leak in ThreadWatcher. BUG=none TEST=unit tests ========== to ========== Fix memory leak in ThreadWatcher. BUG=none TEST=unit tests Committed: https://crrev.com/5ca77e86703646cb84be35ef827068ac6951be1e Cr-Commit-Position: refs/heads/master@{#415729} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5ca77e86703646cb84be35ef827068ac6951be1e Cr-Commit-Position: refs/heads/master@{#415729}
Message was sent while issue was closed.
jkummerow@chromium.org changed reviewers: + jkummerow@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2295983002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/thread_watcher.cc (right): https://codereview.chromium.org/2295983002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/thread_watcher.cc:88: watcher->ActivateThreadWatching(); I believe using |watcher| after calling std::move() on it is a Bad Idea™.
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2299453005/ by jkummerow@chromium.org. The reason for reverting is: Current Canary is unusable due to browser process crashes in line 88 (Windows) and 389 (Mac) in thread_watcher.cc, see crbug.com/643109. BUG=643109.
Message was sent while issue was closed.
https://codereview.chromium.org/2295983002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/thread_watcher.cc (right): https://codereview.chromium.org/2295983002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/thread_watcher.cc:88: watcher->ActivateThreadWatching(); On 2016/09/01 13:35:17, Jakob wrote: > I believe using |watcher| after calling std::move() on it is a Bad Idea™. Ah, good call -- my bad for missing that! One possible way to restructure the code: Change ThreadWatcherList::Register() to also call ActivateThreadWatching(). Another: Have ThreadWatcherList::Register() return a pointer to the registered watcher, or null if registration failed. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
