[Sync] Require ProfileSyncService to have an uninitialized backend on shutdown

[Sync] Require ProfileSyncService to have an uninitialized backend on shutdown

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

[akalin, 2012-10-01 21:11:40]

+zea for review

[tim (not reviewing), 2012-10-01 21:18:57]

https://codereview.chromium.org/11037007/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/11037007/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:170: CHECK(!backend_initialized_);
Hmm. I think DCHECK is more appropriate here (using
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
as guiding principle) Unless this is temporarily a CHECK to see if it's actually
happening?

[Nicolas Zea, 2012-10-01 21:28:08]

Tim's comment aside, LGTM

[akalin, 2012-10-01 22:36:38]

On 2012/10/01 21:18:57, timsteele wrote:
>
https://codereview.chromium.org/11037007/diff/1/chrome/browser/sync/profile_s...
> File chrome/browser/sync/profile_sync_service.cc (right):
> 
>
https://codereview.chromium.org/11037007/diff/1/chrome/browser/sync/profile_s...
> chrome/browser/sync/profile_sync_service.cc:170: CHECK(!backend_initialized_);
> Hmm. I think DCHECK is more appropriate here (using
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
> as guiding principle) Unless this is temporarily a CHECK to see if it's
actually
> happening?

I think that guide is at best overly simplistic.  In this case, I believe
running without DCHECKs and destroying PSS without shutting down will cause a
hang (since SBH will block on shutting down the sync thread without calling
DoShutdown), so I think a CHECK is preferable.

[tim (not reviewing), 2012-10-02 01:10:39]

On 2012/10/01 22:36:38, akalin wrote:
> On 2012/10/01 21:18:57, timsteele wrote:
> >
>
https://codereview.chromium.org/11037007/diff/1/chrome/browser/sync/profile_s...
> > File chrome/browser/sync/profile_sync_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/11037007/diff/1/chrome/browser/sync/profile_s...
> > chrome/browser/sync/profile_sync_service.cc:170:
CHECK(!backend_initialized_);
> > Hmm. I think DCHECK is more appropriate here (using
> >
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
> > as guiding principle) Unless this is temporarily a CHECK to see if it's
> actually
> > happening?
> 
> I think that guide is at best overly simplistic.  In this case, I believe
> running without DCHECKs and destroying PSS without shutting down will cause a
> hang (since SBH will block on shutting down the sync thread without calling
> DoShutdown), so I think a CHECK is preferable.

I don't think it will hang since SBH's destructor is empty, and it seems like a
quintessential "method precondition" case to me.  But I don't think it's worth
holding the review up over. LGTM

[commit-bot: I haz the power, 2012-10-02 05:07:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/11037007/1

[akalin, 2012-10-02 05:10:10]

On 2012/10/02 05:07:38, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/akalin%40chromium.org/11037007/1

As we discussed offline, the destructor for the thread object implicitly waits,
so I think it'll hang.

Committing via commit bot.

[commit-bot: I haz the power, 2012-10-02 06:57:58]

Change committed as 159647