Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20777008:
Remove obsolete WeakPtr hack. (Closed)
|
Created:
7 years, 4 months ago by no sievers Modified:
7 years, 4 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, gavinp+memory_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemove obsolete WeakPtr hack.
BUG=234964
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215470
Patch Set 1 #
Total comments: 8
Patch Set 2 : #Patch Set 3 : #
Total comments: 10
Patch Set 4 : address comments #Patch Set 5 : #Messages
Total messages: 12 (0 generated)
Picking this up from https://codereview.chromium.org/19522006/ in a new issue here...
https://codereview.chromium.org/20777008/diff/1/base/memory/weak_ptr_unittest.cc File base/memory/weak_ptr_unittest.cc (right): https://codereview.chromium.org/20777008/diff/1/base/memory/weak_ptr_unittest... base/memory/weak_ptr_unittest.cc:372: arrow->target.reset(); Ok, to follow up on the original discussion: This test is legal because releasing the WeakPtr here does not invalidate the flag. And we only deref on the bound thread (line 369 and 378 after rebinding.) https://codereview.chromium.org/20777008/diff/1/base/memory/weak_ptr_unittest... base/memory/weak_ptr_unittest.cc:401: EXPECT_EQ(NULL, background.DeRef(arrow)); This is the same test as above, but it invalidates all WeakPtrs on the background thread before rebinding.
https://codereview.chromium.org/20777008/diff/1/base/memory/weak_ptr_unittest.cc File base/memory/weak_ptr_unittest.cc (right): https://codereview.chromium.org/20777008/diff/1/base/memory/weak_ptr_unittest... base/memory/weak_ptr_unittest.cc:372: arrow->target.reset(); On 2013/08/01 01:12:04, sievers wrote: > Ok, to follow up on the original discussion: > > This test is legal because releasing the WeakPtr here does not invalidate the > flag. And we only deref on the bound thread (line 369 and 378 after rebinding.) Right, so this test is really MoveOwnershipOfUnreferencedObject. https://codereview.chromium.org/20777008/diff/1/base/memory/weak_ptr_unittest... base/memory/weak_ptr_unittest.cc:400: background.InvalidateArrowsForTarget(&target); If you can rewrite this specific test to use a WeakPtrFactory<> and WeakPtr directly, rather than the Target and Arrow combination, it'll avoid SupportsWeakPtr needing an InvalidateWeakPtrs, and the extra boilerplate in Target. WDYT? https://codereview.chromium.org/20777008/diff/1/base/memory/weak_ptr_unittest... base/memory/weak_ptr_unittest.cc:404: arrow->target.reset(); Don't do this here - the test is specifically for the case of other objects continuing to hold WeakPtrs that used to refer to the object, but which are now safely NULL thanks you the invalidation. https://codereview.chromium.org/20777008/diff/1/base/memory/weak_ptr_unittest... base/memory/weak_ptr_unittest.cc:416: background.DeleteArrow(arrow); IIRC this aspect of WeakPtr semantics is tested elsewhere, so there's no real need for it here.
https://codereview.chromium.org/20777008/diff/1/base/memory/weak_ptr_unittest.cc File base/memory/weak_ptr_unittest.cc (right): https://codereview.chromium.org/20777008/diff/1/base/memory/weak_ptr_unittest... base/memory/weak_ptr_unittest.cc:372: arrow->target.reset(); On 2013/08/01 18:30:31, Wez wrote: > On 2013/08/01 01:12:04, sievers wrote: > > Ok, to follow up on the original discussion: > > > > This test is legal because releasing the WeakPtr here does not invalidate the > > flag. And we only deref on the bound thread (line 369 and 378 after > rebinding.) > > Right, so this test is really MoveOwnershipOfUnreferencedObject. Done. https://codereview.chromium.org/20777008/diff/1/base/memory/weak_ptr_unittest... base/memory/weak_ptr_unittest.cc:404: arrow->target.reset(); On 2013/08/01 18:30:31, Wez wrote: > Don't do this here - the test is specifically for the case of other objects > continuing to hold WeakPtrs that used to refer to the object, but which are now > safely NULL thanks you the invalidation. Yea, that's making sense now. Done. https://codereview.chromium.org/20777008/diff/9001/base/memory/weak_ptr_unitt... File base/memory/weak_ptr_unittest.cc (right): https://codereview.chromium.org/20777008/diff/9001/base/memory/weak_ptr_unitt... base/memory/weak_ptr_unittest.cc:374: Target* target = new Target; I could add some plumbing in Background to avoid instantiating SupportsWeakPtr here which makes it a bit confusing in the test, but it's all a bit constructed around Target/Arrow (Arrow also points to Target specifically). https://codereview.chromium.org/20777008/diff/9001/base/memory/weak_ptr_unitt... base/memory/weak_ptr_unittest.cc:392: arrow.target.reset(); This wouldn't be needed if I made the test the opposite and invalidated on the background thread.
https://codereview.chromium.org/20777008/diff/9001/base/memory/weak_ptr_unitt... File base/memory/weak_ptr_unittest.cc (right): https://codereview.chromium.org/20777008/diff/9001/base/memory/weak_ptr_unitt... base/memory/weak_ptr_unittest.cc:365: background.DeleteArrow(arrow); nit: This doesn't feel like it belongs in this test, since it's testing a different property (that a WeakPtr "owned" on thread A can be deleted on any other thread) - perhaps create a small test separately for that? https://codereview.chromium.org/20777008/diff/9001/base/memory/weak_ptr_unitt... base/memory/weak_ptr_unittest.cc:374: Target* target = new Target; On 2013/08/01 20:57:59, sievers wrote: > I could add some plumbing in Background to avoid instantiating SupportsWeakPtr > here which makes it a bit confusing in the test, but it's all a bit constructed > around Target/Arrow (Arrow also points to Target specifically). You can create a Target, wrap it with a WeakPtrFactory<Target> and pull a WeakPtr from that to assign to the Arrow. https://codereview.chromium.org/20777008/diff/9001/base/memory/weak_ptr_unitt... base/memory/weak_ptr_unittest.cc:374: Target* target = new Target; nit: scoped_ptr<Target> target = new Target, and then DeleteTarget(target.release()) later on. https://codereview.chromium.org/20777008/diff/9001/base/memory/weak_ptr_unitt... base/memory/weak_ptr_unittest.cc:391: // Make sure we can delete the factory on the main thread. This code's not deleting the factory, it's dropping the WeakPtr to the Target, right? https://codereview.chromium.org/20777008/diff/9001/base/memory/weak_ptr_unitt... base/memory/weak_ptr_unittest.cc:392: arrow.target.reset(); On 2013/08/01 20:57:59, sievers wrote: > This wouldn't be needed if I made the test the opposite and invalidated on the > background thread. Why is it needed at all? It's just clearing the WeakPtr, which can be done on any thread.
https://codereview.chromium.org/20777008/diff/9001/base/memory/weak_ptr_unitt... File base/memory/weak_ptr_unittest.cc (right): https://codereview.chromium.org/20777008/diff/9001/base/memory/weak_ptr_unitt... base/memory/weak_ptr_unittest.cc:392: arrow.target.reset(); On 2013/08/01 21:09:10, Wez wrote: > On 2013/08/01 20:57:59, sievers wrote: > > This wouldn't be needed if I made the test the opposite and invalidated on the > > background thread. > > Why is it needed at all? It's just clearing the WeakPtr, which can be done on > any thread. Because the factory's flag is now bound to the background thread and deleting the factory invalidates the flag. That fails a check unless there is no outstanding pointers to it.
lgtm
oops, pressed button too quick :) please make sure to address wez's test comments.
https://codereview.chromium.org/20777008/diff/9001/base/memory/weak_ptr_unitt... File base/memory/weak_ptr_unittest.cc (right): https://codereview.chromium.org/20777008/diff/9001/base/memory/weak_ptr_unitt... base/memory/weak_ptr_unittest.cc:365: background.DeleteArrow(arrow); On 2013/08/01 21:09:10, Wez wrote: > nit: This doesn't feel like it belongs in this test, since it's testing a > different property (that a WeakPtr "owned" on thread A can be deleted on any > other thread) - perhaps create a small test separately for that? Done. NonOwnerThreadCanDeleteWeakPtr already covers the other case you mention. https://codereview.chromium.org/20777008/diff/9001/base/memory/weak_ptr_unitt... base/memory/weak_ptr_unittest.cc:374: Target* target = new Target; On 2013/08/01 21:09:10, Wez wrote: > nit: scoped_ptr<Target> target = new Target, and then > DeleteTarget(target.release()) later on. Done.
LGTM. Thanks again for cleaning this up. :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/20777008/20001
Message was sent while issue was closed.
Change committed as 215470 |
