Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10690080:
Rename/re-comment tests and added new death tests for WeakPtr (Closed)
|
Created:
8 years, 5 months ago by kaiwang Modified:
8 years, 4 months ago CC:
chromium-reviews, erikwright (departed), brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionSecond try of http://codereview.chromium.org/10698063/
Rename/re-comment tests and added new death tests for WeakPtr.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146212
Patch Set 1 #
Total comments: 14
Patch Set 2 : #
Total comments: 14
Patch Set 3 : #Patch Set 4 : Rename/re-comment tests and added new death tests for WeakPtr. #Patch Set 5 : #Messages
Total messages: 11 (0 generated)
https://chromiumcodereview.appspot.com/10690080/diff/1/base/memory/weak_ptr.h File base/memory/weak_ptr.h (right): https://chromiumcodereview.appspot.com/10690080/diff/1/base/memory/weak_ptr.h... base/memory/weak_ptr.h:19: // WeakPtrs are deleted. To work around of this, call nit: To work around this https://chromiumcodereview.appspot.com/10690080/diff/1/base/memory/weak_ptr.h... base/memory/weak_ptr.h:20: // SupportsWeakPtr::DetachFromThread(). I'm not sure we should encourage calling DetachFromThread(). The behaviour being displayed is very much intentional - the WeakPtrFactory should only disappear on the thread that is associated with all of the WeakPtrs. The ThreadChecker that exists (and is used by DetachFromThread) is meant to guard that. Doing Thread 1: WeakPtr<Foo> foo = controller->AsWeakPtr(); controller->DetachFromThread(); thread_2->DeleteSoon(FROM_HERE, controller) Is inherently dangerous, and it seems to be what the comment is suggesting. https://chromiumcodereview.appspot.com/10690080/diff/1/base/memory/weak_ptr_u... File base/memory/weak_ptr_unittest.cc (right): https://chromiumcodereview.appspot.com/10690080/diff/1/base/memory/weak_ptr_u... base/memory/weak_ptr_unittest.cc:45: struct Arrow { WeakPtr<Target> target; }; style nit: struct Arrow { WeakPtr<Target> target; }; (Same on line 40) https://chromiumcodereview.appspot.com/10690080/diff/1/base/memory/weak_ptr_u... base/memory/weak_ptr_unittest.cc:245: // but uses it on another. This tests that we do not trip runtime checks that nit: uses -> use (since this is an active statement, "Test that it is ok to ...") https://chromiumcodereview.appspot.com/10690080/diff/1/base/memory/weak_ptr_u... base/memory/weak_ptr_unittest.cc:391: #ifndef NDEBUG // Thread ownership is only checked in debug version. This isn't correct It should be #if !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON) https://chromiumcodereview.appspot.com/10690080/diff/1/base/memory/weak_ptr_u... base/memory/weak_ptr_unittest.cc:406: // ownership. the comments here and on 423 are backwards (this one derefs, 423 deletes) https://chromiumcodereview.appspot.com/10690080/diff/1/base/memory/weak_ptr_u... base/memory/weak_ptr_unittest.cc:417: Target* target = new Target(); Pretty sure this object will show up in the valgrind bots as leaked. That's because ASSERT_DEATH forks off a new process. While the child will try to delete |target| on the background thread, in the parent process, this will be leaked. I'd suggest reworking this as follows: scoped_ptr<Target> target(new Target); ASSERT_DEATH(background.DeleteTarget(target.Release()), ""); the code within the ASSERT_DEATH will /only/ run in the child process on the background thread. in the parent process, |target| will be deleted on the parent thread. If that's too complex, then you should put this entire bit within a helper function and then ASSERT_DEATH(SomeHelperFunction(), ""), to ensure that the allocate-on-parent/free-on-child only happens in the spawned proc.
http://codereview.chromium.org/10690080/diff/1/base/memory/weak_ptr.h File base/memory/weak_ptr.h (right): http://codereview.chromium.org/10690080/diff/1/base/memory/weak_ptr.h#newcode19 base/memory/weak_ptr.h:19: // WeakPtrs are deleted. To work around of this, call On 2012/07/10 00:06:44, Ryan Sleevi wrote: > nit: To work around this Done. http://codereview.chromium.org/10690080/diff/1/base/memory/weak_ptr.h#newcode20 base/memory/weak_ptr.h:20: // SupportsWeakPtr::DetachFromThread(). Changed the wording. http://codereview.chromium.org/10690080/diff/1/base/memory/weak_ptr_unittest.cc File base/memory/weak_ptr_unittest.cc (right): http://codereview.chromium.org/10690080/diff/1/base/memory/weak_ptr_unittest.... base/memory/weak_ptr_unittest.cc:45: struct Arrow { WeakPtr<Target> target; }; On 2012/07/10 00:06:44, Ryan Sleevi wrote: > style nit: > > struct Arrow { > WeakPtr<Target> target; > }; > > (Same on line 40) Done. http://codereview.chromium.org/10690080/diff/1/base/memory/weak_ptr_unittest.... base/memory/weak_ptr_unittest.cc:245: // but uses it on another. This tests that we do not trip runtime checks that On 2012/07/10 00:06:44, Ryan Sleevi wrote: > nit: uses -> use (since this is an active statement, "Test that it is ok to > ...") Done. http://codereview.chromium.org/10690080/diff/1/base/memory/weak_ptr_unittest.... base/memory/weak_ptr_unittest.cc:391: #ifndef NDEBUG // Thread ownership is only checked in debug version. On 2012/07/10 00:06:44, Ryan Sleevi wrote: > This isn't correct > > It should be > > #if !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON) Done. http://codereview.chromium.org/10690080/diff/1/base/memory/weak_ptr_unittest.... base/memory/weak_ptr_unittest.cc:406: // ownership. On 2012/07/10 00:06:44, Ryan Sleevi wrote: > the comments here and on 423 are backwards (this one derefs, 423 deletes) Done. http://codereview.chromium.org/10690080/diff/1/base/memory/weak_ptr_unittest.... base/memory/weak_ptr_unittest.cc:417: Target* target = new Target(); On 2012/07/10 00:06:44, Ryan Sleevi wrote: > Pretty sure this object will show up in the valgrind bots as leaked. > > That's because ASSERT_DEATH forks off a new process. While the child will try to > delete |target| on the background thread, in the parent process, this will be > leaked. > > I'd suggest reworking this as follows: > > scoped_ptr<Target> target(new Target); > > ASSERT_DEATH(background.DeleteTarget(target.Release()), ""); > > the code within the ASSERT_DEATH will /only/ run in the child process on the > background thread. in the parent process, |target| will be deleted on the parent > thread. > > If that's too complex, then you should put this entire bit within a helper > function and then ASSERT_DEATH(SomeHelperFunction(), ""), to ensure that the > allocate-on-parent/free-on-child only happens in the spawned proc. Done.
I think these updates look good - a few more nits. Since Jim originally reviewed, and because it's base/, you probably want his final stamp, but I think this looks good. http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr.h File base/memory/weak_ptr.h (right): http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr.h#newc... base/memory/weak_ptr.h:57: // and can not be deleted from other threads unless all WeakPtrs are deleted. nit: s/unless/until/ http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr.h#newc... base/memory/weak_ptr.h:59: // the thread binding, but it's not recommended and unsafe. I almost wonder if we shouldn't remove DetachFromThread. I'm curious if it's being used, and if the consumers are correct. However, for sake of simplicitly, I think providing examples of "how to use it correctly" may be... well.. too complex. http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr.h#newc... base/memory/weak_ptr.h:61: // WeakPtrs are usually copied by other threads. Note the copied WeakPtr is nit: s/are usually/may be/. nit: drop the "Note" Write the comments assuming no other code exists - talking about how it should/can be used, not how it commonly is used. The following may be better "WeakPtrs may be copy-constructed, assigned, or destructed on threads other than the thread they are bound to. However, any copies will remain bound to the thread that the WeakPtr was originally created on, and thus may only be dereferenced on that thread." http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr_unitte... File base/memory/weak_ptr_unittest.cc (right): http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr_unitte... base/memory/weak_ptr_unittest.cc:399: // (introduces deadlock on linux). nit: s/can not well/does not/ s/linux/Linux/ Same with the other comments http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr_unitte... base/memory/weak_ptr_unittest.cc:413: background.CreateArrowFromArrow(&arrow_copy, &arrow); You're now leaking the arrow_copy, I believe. This should be a scoped_ptr<>.
http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr_unitte... File base/memory/weak_ptr_unittest.cc (right): http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr_unitte... base/memory/weak_ptr_unittest.cc:215: Target f; Sorry I didn't notice this before... (and this existed before your changes): nit: please get rid of most all of these one-letter-varibles. The code you wrote did so. same on line 221, and maybe on line 169 and 183. http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr_unitte... base/memory/weak_ptr_unittest.cc:322: // violation. Will this leak arrow?
Rename/re-comment tests and added new death tests for WeakPtr.
http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr.h File base/memory/weak_ptr.h (right): http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr.h#newc... base/memory/weak_ptr.h:57: // and can not be deleted from other threads unless all WeakPtrs are deleted. On 2012/07/10 22:41:54, Ryan Sleevi wrote: > nit: s/unless/until/ Done. http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr.h#newc... base/memory/weak_ptr.h:59: // the thread binding, but it's not recommended and unsafe. Searched the code and seems this function is used but not in many places. http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr.h#newc... base/memory/weak_ptr.h:61: // WeakPtrs are usually copied by other threads. Note the copied WeakPtr is Done. Good suggestion :) http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr_unitte... File base/memory/weak_ptr_unittest.cc (right): http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr_unitte... base/memory/weak_ptr_unittest.cc:215: Target f; Done, for 169 and 183, it's just an int. So I think leave them a one letter variable should be fine.. http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr_unitte... base/memory/weak_ptr_unittest.cc:322: // violation. On 2012/07/10 23:00:56, jar wrote: > Will this leak arrow? Done. http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr_unitte... base/memory/weak_ptr_unittest.cc:399: // (introduces deadlock on linux). On 2012/07/10 22:41:54, Ryan Sleevi wrote: > nit: > s/can not well/does not/ > s/linux/Linux/ > > Same with the other comments Done. http://codereview.chromium.org/10690080/diff/6001/base/memory/weak_ptr_unitte... base/memory/weak_ptr_unittest.cc:413: background.CreateArrowFromArrow(&arrow_copy, &arrow); since we are modifying the pointer in background thread, scoped_ptr does not work. I manually added a DeleteArrow below as in above tests.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaiwang@chromium.org/10690080/11001
Try job failure for 10690080-11001 (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaiwang@chromium.org/10690080/8004
Change committed as 146212 |
