ChromeOS: fix crash in TokenHandleFetcher destructor.

ChromeOS: fix crash in TokenHandleFetcher destructor.

Profile may be destroyed while its TokenHandleFetcher exists.
So TokenHandleFetcher should watch profile destruction.

BUG=513131
TEST=manual

Committed: https://crrev.com/eddbadd44e45fa659ac2956482996924b0dcf8a0
Cr-Commit-Position: refs/heads/master@{#345490}

[Alexander Alekseev, 2015-08-18 22:29:41]

alemate@chromium.org changed reviewers:
+ dzhioev@chromium.org

[Alexander Alekseev, 2015-08-18 22:30:17]

Please review.

[Alexander Alekseev, 2015-08-18 22:30:38]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-18 22:31:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1294543008/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1294543008/1

[achuithb, 2015-08-18 22:38:40]

achuith@chromium.org changed reviewers:
+ achuith@chromium.org

[achuithb, 2015-08-18 22:38:41]

Believe this is fixed with:
https://codereview.chromium.org/1285363003/

[commit-bot: I haz the power, 2015-08-18 23:40:37]

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

[commit-bot: I haz the power, 2015-08-18 23:40:37]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2015-08-19 14:17:01]

On 2015/08/18 22:38:41, achuithb wrote:
> Believe this is fixed with:
> https://codereview.chromium.org/1285363003/

Do you think we also need this CL to make TokenHandleFetcher directly depend on
keyed service ?

[Alexander Alekseev, 2015-08-20 18:01:06]

Achuith, what do you think?

On 2015/08/19 14:17:01, Alexander Alekseev wrote:
> On 2015/08/18 22:38:41, achuithb wrote:
> > Believe this is fixed with:
> > https://codereview.chromium.org/1285363003/
> 
> Do you think we also need this CL to make TokenHandleFetcher directly depend
on
> keyed service ?

[achuithb, 2015-08-20 19:42:08]

On 2015/08/20 18:01:06, Alexander Alekseev wrote:
> Achuith, what do you think?

Personally I don't see the benefit. It's a lot of boilerplate that doesn't fix
any current bug.

Pavel - what do you think?

[dzhioev (left Google), 2015-08-20 23:12:52]

On 2015/08/20 19:42:08, achuithb wrote:
> On 2015/08/20 18:01:06, Alexander Alekseev wrote:
> > Achuith, what do you think?
> 
> Personally I don't see the benefit. It's a lot of boilerplate that doesn't fix
> any current bug.
> 
> Pavel - what do you think?

I don't think this change is needed.
And as far as I understand this change would have not fixed the issue,
if https://codereview.chromium.org/1285363003/ has not been landed:
1. ProfileOAuth2TokenService is gone.
2. TokenHandleFetcher::OnProfileDestroyed: callback_.Run(user_id_, false)
3. UserSessionManager::OnTokenHandleObtained: token_handle_fetcher_.reset()
4. TokenHandleFetcher::~TokenHandleFetcher: token_service_->RemoveObserver(this)
<-- Failure

[Alexander Alekseev, 2015-08-21 13:53:14]

On 2015/08/20 23:12:52, dzhioev wrote:
> On 2015/08/20 19:42:08, achuithb wrote:
> > On 2015/08/20 18:01:06, Alexander Alekseev wrote:
> > > Achuith, what do you think?
> > 
> > Personally I don't see the benefit. It's a lot of boilerplate that doesn't
fix
> > any current bug.
> > 
> > Pavel - what do you think?
> 
> I don't think this change is needed.
> And as far as I understand this change would have not fixed the issue,
> if https://codereview.chromium.org/1285363003/ has not been landed:
> 1. ProfileOAuth2TokenService is gone.
> 2. TokenHandleFetcher::OnProfileDestroyed: callback_.Run(user_id_, false)
> 3. UserSessionManager::OnTokenHandleObtained: token_handle_fetcher_.reset()
> 4. TokenHandleFetcher::~TokenHandleFetcher:
token_service_->RemoveObserver(this)
> <-- Failure

As far as I understand, ProfileOAuth2TokenService is still alive while it's
watchers are deleted. So it is safe to call RemoveObserver.

[dzhioev (left Google), 2015-08-22 01:14:30]

On 2015/08/21 13:53:14, Alexander Alekseev wrote:
> On 2015/08/20 23:12:52, dzhioev wrote:
> > On 2015/08/20 19:42:08, achuithb wrote:
> > > On 2015/08/20 18:01:06, Alexander Alekseev wrote:
> > > > Achuith, what do you think?
> > > 
> > > Personally I don't see the benefit. It's a lot of boilerplate that doesn't
> fix
> > > any current bug.
> > > 
> > > Pavel - what do you think?
> > 
> > I don't think this change is needed.
> > And as far as I understand this change would have not fixed the issue,
> > if https://codereview.chromium.org/1285363003/ has not been landed:
> > 1. ProfileOAuth2TokenService is gone.
> > 2. TokenHandleFetcher::OnProfileDestroyed: callback_.Run(user_id_, false)
> > 3. UserSessionManager::OnTokenHandleObtained: token_handle_fetcher_.reset()
> > 4. TokenHandleFetcher::~TokenHandleFetcher:
> token_service_->RemoveObserver(this)
> > <-- Failure
> 
> As far as I understand, ProfileOAuth2TokenService is still alive while it's
> watchers are deleted. So it is safe to call RemoveObserver.

Then I don't understand what was the reason of the crash. =) Can you please
explain?

[Alexander Alekseev, 2015-08-24 22:32:54]

On 2015/08/22 01:14:30, dzhioev wrote:
> On 2015/08/21 13:53:14, Alexander Alekseev wrote:
> > On 2015/08/20 23:12:52, dzhioev wrote:
> > > On 2015/08/20 19:42:08, achuithb wrote:
> > > > On 2015/08/20 18:01:06, Alexander Alekseev wrote:
> > > > > Achuith, what do you think?
> > > > 
> > > > Personally I don't see the benefit. It's a lot of boilerplate that
doesn't
> > fix
> > > > any current bug.
> > > > 
> > > > Pavel - what do you think?
> > > 
> > > I don't think this change is needed.
> > > And as far as I understand this change would have not fixed the issue,
> > > if https://codereview.chromium.org/1285363003/ has not been landed:
> > > 1. ProfileOAuth2TokenService is gone.
> > > 2. TokenHandleFetcher::OnProfileDestroyed: callback_.Run(user_id_, false)
> > > 3. UserSessionManager::OnTokenHandleObtained:
token_handle_fetcher_.reset()
> > > 4. TokenHandleFetcher::~TokenHandleFetcher:
> > token_service_->RemoveObserver(this)
> > > <-- Failure
> > 
> > As far as I understand, ProfileOAuth2TokenService is still alive while it's
> > watchers are deleted. So it is safe to call RemoveObserver.
> 
> Then I don't understand what was the reason of the crash. =) Can you please
> explain?

It looks like UserSessionManager::FinalizePrepareProfile creates
TokenHandleFetcher , which waits forever for new refresh token. Then Profile is
destroyed (when test is completed), but TokenHandleFetcher is still alive and
holds invalid pointer to token service.

Then it crashes in:
          
TokenHandleFetcher::~TokenHandleFetcher() {                                     

  if (waiting_for_refresh_token_)                                               

    token_service_->RemoveObserver(this);                                       

}

[dzhioev (left Google), 2015-08-25 23:16:27]

On 2015/08/24 22:32:54, Alexander Alekseev wrote:
> On 2015/08/22 01:14:30, dzhioev wrote:
> > On 2015/08/21 13:53:14, Alexander Alekseev wrote:
> > > On 2015/08/20 23:12:52, dzhioev wrote:
> > > > On 2015/08/20 19:42:08, achuithb wrote:
> > > > > On 2015/08/20 18:01:06, Alexander Alekseev wrote:
> > > > > > Achuith, what do you think?
> > > > > 
> > > > > Personally I don't see the benefit. It's a lot of boilerplate that
> doesn't
> > > fix
> > > > > any current bug.
> > > > > 
> > > > > Pavel - what do you think?
> > > > 
> > > > I don't think this change is needed.
> > > > And as far as I understand this change would have not fixed the issue,
> > > > if https://codereview.chromium.org/1285363003/ has not been landed:
> > > > 1. ProfileOAuth2TokenService is gone.
> > > > 2. TokenHandleFetcher::OnProfileDestroyed: callback_.Run(user_id_,
false)
> > > > 3. UserSessionManager::OnTokenHandleObtained:
> token_handle_fetcher_.reset()
> > > > 4. TokenHandleFetcher::~TokenHandleFetcher:
> > > token_service_->RemoveObserver(this)
> > > > <-- Failure
> > > 
> > > As far as I understand, ProfileOAuth2TokenService is still alive while
it's
> > > watchers are deleted. So it is safe to call RemoveObserver.
> > 
> > Then I don't understand what was the reason of the crash. =) Can you please
> > explain?
> 
> It looks like UserSessionManager::FinalizePrepareProfile creates
> TokenHandleFetcher , which waits forever for new refresh token. Then Profile
is
> destroyed (when test is completed), but TokenHandleFetcher is still alive and
> holds invalid pointer to token service.
> 
> Then it crashes in:
>           
> TokenHandleFetcher::~TokenHandleFetcher() {                                   
 
> 
>   if (waiting_for_refresh_token_)                                             
 
> 
>     token_service_->RemoveObserver(this);                                     
 
> 
> }
LGTM

[Alexander Alekseev, 2015-08-25 23:27:09]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2015-08-25 23:27:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1294543008/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1294543008/1

[commit-bot: I haz the power, 2015-08-25 23:58:16]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-08-25 23:58:49]

Patchset 1 (id:??) landed as
https://crrev.com/eddbadd44e45fa659ac2956482996924b0dcf8a0
Cr-Commit-Position: refs/heads/master@{#345490}