[Sync] Avoid leaking in WeakHandle even when the owner thread is gone

[Sync] Avoid leaking in WeakHandle even when the owner thread is gone

Destroy the underlying WeakPtr right before the owner thread is destroyed.

Remove suppression for the leak in the bug.

BUG=91465
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95679

[akalin, 2011-08-04 01:05:27]

+zea for review

[akalin, 2011-08-04 18:56:15]

On 2011/08/04 01:05:27, akalin wrote:
> +zea for review

Ping!

[Nicolas Zea, 2011-08-04 19:54:10]

http://codereview.chromium.org/7572010/diff/1/chrome/browser/sync/weak_handle.cc
File chrome/browser/sync/weak_handle.cc (right):

http://codereview.chromium.org/7572010/diff/1/chrome/browser/sync/weak_handle...
chrome/browser/sync/weak_handle.cc:33: DestroyOnOwnerThread();
What's the purpose to this call? All DestroyOnOwnerThread does is unregister
itself as a destruction observer right? Is that necessary when the loop is
already being destroyed?

http://codereview.chromium.org/7572010/diff/1/chrome/browser/sync/weak_handle.h
File chrome/browser/sync/weak_handle.h (right):

http://codereview.chromium.org/7572010/diff/1/chrome/browser/sync/weak_handle...
chrome/browser/sync/weak_handle.h:131: // whichever comes first.  If you
override this, make to still call
"make sure to still"

http://codereview.chromium.org/7572010/diff/1/chrome/browser/sync/weak_handle...
chrome/browser/sync/weak_handle.h:144: void DestroyAndDelete();
I find the difference between "destroy" and "delete" to be confusing. Why
doesn't destroy already delete? What exactly is destroy doing other than delete?

http://codereview.chromium.org/7572010/diff/1/chrome/browser/sync/weak_handle...
chrome/browser/sync/weak_handle.h:158: bool destroyed_on_owner_thread_;
Is this variable just used for the purpose of CHECK's? It doesn't seem to be
used in any actual logic.

http://codereview.chromium.org/7572010/diff/1/chrome/browser/sync/weak_handle...
chrome/browser/sync/weak_handle.h:259: if (!Get().get()) {
:(
Maybe Get() can  be changed to GetScopedPtr()?

[akalin, 2011-08-04 21:56:52]

Addressed all comments, PTAL

http://codereview.chromium.org/7572010/diff/1/chrome/browser/sync/weak_handle.cc
File chrome/browser/sync/weak_handle.cc (right):

http://codereview.chromium.org/7572010/diff/1/chrome/browser/sync/weak_handle...
chrome/browser/sync/weak_handle.cc:33: DestroyOnOwnerThread();
On 2011/08/04 19:54:10, nzea wrote:
> What's the purpose to this call? All DestroyOnOwnerThread does is unregister
> itself as a destruction observer right? Is that necessary when the loop is
> already being destroyed?

Not quite.  It's overridden by WeakHandle (to destroy the WeakPtr).  Added
clarifying comment.

http://codereview.chromium.org/7572010/diff/1/chrome/browser/sync/weak_handle.h
File chrome/browser/sync/weak_handle.h (right):

http://codereview.chromium.org/7572010/diff/1/chrome/browser/sync/weak_handle...
chrome/browser/sync/weak_handle.h:131: // whichever comes first.  If you
override this, make to still call
On 2011/08/04 19:54:10, nzea wrote:
> "make sure to still"

Rendered moot.

http://codereview.chromium.org/7572010/diff/1/chrome/browser/sync/weak_handle...
chrome/browser/sync/weak_handle.h:144: void DestroyAndDelete();
On 2011/08/04 19:54:10, nzea wrote:
> I find the difference between "destroy" and "delete" to be confusing. Why
> doesn't destroy already delete? What exactly is destroy doing other than
delete?

Good point.  Renamed 'Destroy' to 'Cleanup' and 'Delete' to 'Destroy'.  I think
that makes more sense.

http://codereview.chromium.org/7572010/diff/1/chrome/browser/sync/weak_handle...
chrome/browser/sync/weak_handle.h:158: bool destroyed_on_owner_thread_;
On 2011/08/04 19:54:10, nzea wrote:
> Is this variable just used for the purpose of CHECK's? It doesn't seem to be
> used in any actual logic.

Yes.  I think it's still worth having, though.

http://codereview.chromium.org/7572010/diff/1/chrome/browser/sync/weak_handle...
chrome/browser/sync/weak_handle.h:259: if (!Get().get()) {
On 2011/08/04 19:54:10, nzea wrote:
> :(
> Maybe Get() can  be changed to GetScopedPtr()?

WeakPtr<T> can be cast to T*, so just changed to Get().  (it's a weakptr btw,
not a scoped_ptr)

[akalin, 2011-08-05 19:32:17]

ping!

[Nicolas Zea, 2011-08-05 19:46:59]

LGTM. I had actually sent one earlier, but it didn't save the comment for some
reason.