Make ProfileSigninConfirmationHelper delete itself instead of being refcounted.

Make ProfileSigninConfirmationHelper delete itself instead of being refcounted.

BUG=393149
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284075

[Bernhard Bauer, 2014-07-14 09:24:02]

Please review.

[Andrew T Wilson (Slow), 2014-07-14 11:04:46]

LGTM, with one question.

https://codereview.chromium.org/386023003/diff/20001/chrome/browser/ui/sync/p...
File chrome/browser/ui/sync/profile_signin_confirmation_helper.cc (right):

https://codereview.chromium.org/386023003/diff/20001/chrome/browser/ui/sync/p...
chrome/browser/ui/sync/profile_signin_confirmation_helper.cc:176: delete this;
Does this mean we leak if our callbacks aren't invoked? Not clear to me that
ScheduleDBTask() always invokes the callback.

[Bernhard Bauer, 2014-07-14 13:08:29]

https://codereview.chromium.org/386023003/diff/20001/chrome/browser/ui/sync/p...
File chrome/browser/ui/sync/profile_signin_confirmation_helper.cc (right):

https://codereview.chromium.org/386023003/diff/20001/chrome/browser/ui/sync/p...
chrome/browser/ui/sync/profile_signin_confirmation_helper.cc:176: delete this;
On 2014/07/14 11:04:45, Andrew T Wilson wrote:
> Does this mean we leak if our callbacks aren't invoked? Not clear to me that
> ScheduleDBTask() always invokes the callback.

True, if the history backend shuts down, this won't be called back, but that
might not actually be a bad thing, as this would happen during profile shutdown,
and the callbacks passed to CheckShouldPromptForNewProfile() usually pass in an
unretained profile pointer), and in any case it's a leak during shutdown, so all
the memory is freed anyway.

Fixing this would be relatively easy after
https://codereview.chromium.org/387923002/ and a little bit more involved before
that (as HasTypedURLsTask could be destroyed on any thread), but what I would
actually propose to do is change the contract so that the client will own this
object[*], which would make it moot. Let me know what you want me to do.

[*] Which would also allow us to cancel all DB operations on destruction.

[Andrew T Wilson (Slow), 2014-07-14 14:32:58]

https://codereview.chromium.org/386023003/diff/20001/chrome/browser/ui/sync/p...
File chrome/browser/ui/sync/profile_signin_confirmation_helper.cc (right):

https://codereview.chromium.org/386023003/diff/20001/chrome/browser/ui/sync/p...
chrome/browser/ui/sync/profile_signin_confirmation_helper.cc:176: delete this;
On 2014/07/14 13:08:29, Bernhard Bauer wrote:
> On 2014/07/14 11:04:45, Andrew T Wilson wrote:
> > Does this mean we leak if our callbacks aren't invoked? Not clear to me that
> > ScheduleDBTask() always invokes the callback.
> 
> True, if the history backend shuts down, this won't be called back, but that
> might not actually be a bad thing, as this would happen during profile
shutdown,
> and the callbacks passed to CheckShouldPromptForNewProfile() usually pass in
an
> unretained profile pointer), and in any case it's a leak during shutdown, so
all
> the memory is freed anyway.
> 
> Fixing this would be relatively easy after
> https://codereview.chromium.org/387923002/ and a little bit more involved
before
> that (as HasTypedURLsTask could be destroyed on any thread), but what I would
> actually propose to do is change the contract so that the client will own this
> object[*], which would make it moot. Let me know what you want me to do.
> 
> [*] Which would also allow us to cancel all DB operations on destruction.

Just comment that it leaks during profile shutdown, I guess. Have you run this
on the valgrind bots to make sure it's not triggering leaks in our existing
tests? If so, then I'm fine with landing this.

[dconnelly, 2014-07-15 11:20:01]

lgtm

https://codereview.chromium.org/386023003/diff/20001/chrome/browser/ui/sync/p...
File chrome/browser/ui/sync/profile_signin_confirmation_helper.cc (right):

https://codereview.chromium.org/386023003/diff/20001/chrome/browser/ui/sync/p...
chrome/browser/ui/sync/profile_signin_confirmation_helper.cc:176: delete this;
On 2014/07/14 14:32:57, Andrew T Wilson (OOO) wrote:
> On 2014/07/14 13:08:29, Bernhard Bauer wrote:
> > On 2014/07/14 11:04:45, Andrew T Wilson wrote:
> > > Does this mean we leak if our callbacks aren't invoked? Not clear to me
that
> > > ScheduleDBTask() always invokes the callback.
> > 
> > True, if the history backend shuts down, this won't be called back, but that
> > might not actually be a bad thing, as this would happen during profile
> shutdown,
> > and the callbacks passed to CheckShouldPromptForNewProfile() usually pass in
> an
> > unretained profile pointer), and in any case it's a leak during shutdown, so
> all
> > the memory is freed anyway.
> > 
> > Fixing this would be relatively easy after
> > https://codereview.chromium.org/387923002/ and a little bit more involved
> before
> > that (as HasTypedURLsTask could be destroyed on any thread), but what I
would
> > actually propose to do is change the contract so that the client will own
this
> > object[*], which would make it moot. Let me know what you want me to do.
> > 
> > [*] Which would also allow us to cancel all DB operations on destruction.
> 
> Just comment that it leaks during profile shutdown, I guess. Have you run this
> on the valgrind bots to make sure it's not triggering leaks in our existing
> tests? If so, then I'm fine with landing this.

I agree with just adding the comment.

[Bernhard Bauer, 2014-07-17 15:02:35]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2014-07-17 15:03:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bauerb@chromium.org/386023003/40001

[commit-bot: I haz the power, 2014-07-17 21:43:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-18 00:35:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-18 00:35:16]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Bernhard Bauer, 2014-07-18 10:10:49]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2014-07-18 10:11:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bauerb@chromium.org/386023003/40001

[commit-bot: I haz the power, 2014-07-18 13:08:42]

Change committed as 284075