Fix crash when affiliated invalidation service becomes available

Fix crash when affiliated invalidation service becomes available

When an affiliated per-profile invalidation service becomes available,
the AffiliatedInvalidationServiceProviderImpl should:
1) tell consumers to stop using the device-global invalidation service
2) destroy the device-global invalidation service

These two steps were swapped, causing a crash as the consumers did not
get a chance to remove their handlers from the device-global
invalidation service before it is destroyed.

BUG=455504
TEST=Extended unit test to protect against regressions

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

[bartfab (slow), 2015-02-05 11:07:41]

bartfab@chromium.org changed reviewers:
+ pneubeck@chromium.org

[bartfab (slow), 2015-02-05 11:07:41]

Hi Philipp,

Could you take a look?

[pneubeck (no reviews), 2015-02-05 12:39:05]

lgtm

https://codereview.chromium.org/897113003/diff/1/chrome/browser/chromeos/poli...
File
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/897113003/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc:75:
int invalidation_service_set_count_;
you can use static initializers here:
 int invalidation_service_set_count_ = 0;
 invalidation::InvalidationService* invalidation_service_ = nullptr;

https://codereview.chromium.org/897113003/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc:372:
// Register a consumer. Verify that the consumer is not called back
I know you didn't add it in this CL, but instead of refactoring the same code X
times is you could just put it into a separate tiny test.

TEST_F(AffiliatedInvalidationServiceProviderImplTest,
NoNotifcationIfNoServiceConnected) {
  // Register a consumer. Verify that the consumer is not called back
  // immediately as no connected invalidation service exists yet.
  consumer_.reset(new FakeConsumer(provider_.get()));
  EXPECT_EQ(0, consumer_->GetAndClearInvalidationServiceSetCount());
}

The initial comment and
  EXPECT_EQ(0, consumer_->GetAndClearInvalidationServiceSetCount());
could be removed from all tests.
consumer_.reset(...) is self explanatory.

[bartfab (slow), 2015-02-05 15:15:43]

New patchsets have been uploaded after l-g-t-m from pneubeck@chromium.org

[bartfab (slow), 2015-02-05 15:15:53]

https://codereview.chromium.org/897113003/diff/1/chrome/browser/chromeos/poli...
File
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/897113003/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc:75:
int invalidation_service_set_count_;
On 2015/02/05 12:39:05, pneubeck wrote:
> you can use static initializers here:
>  int invalidation_service_set_count_ = 0;
>  invalidation::InvalidationService* invalidation_service_ = nullptr;

Phew, fancy C++11 :).

Done.

By the way - this would be an "initializer," not a "static initializer."

https://codereview.chromium.org/897113003/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc:372:
// Register a consumer. Verify that the consumer is not called back
On 2015/02/05 12:39:05, pneubeck wrote:
> I know you didn't add it in this CL, but instead of refactoring the same code
X
> times is you could just put it into a separate tiny test.
> 
> TEST_F(AffiliatedInvalidationServiceProviderImplTest,
> NoNotifcationIfNoServiceConnected) {
>   // Register a consumer. Verify that the consumer is not called back
>   // immediately as no connected invalidation service exists yet.
>   consumer_.reset(new FakeConsumer(provider_.get()));
>   EXPECT_EQ(0, consumer_->GetAndClearInvalidationServiceSetCount());
> }
> 
> The initial comment and
>   EXPECT_EQ(0, consumer_->GetAndClearInvalidationServiceSetCount());
> could be removed from all tests.
> consumer_.reset(...) is self explanatory.
>   

Done.

[bartfab (slow), 2015-02-05 15:17:36]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2015-02-05 15:18:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/897113003/20001

[commit-bot: I haz the power, 2015-02-05 15:58:06]

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

[commit-bot: I haz the power, 2015-02-05 15:58:07]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bartfab (slow), 2015-02-05 17:30:01]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2015-02-05 17:30:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/897113003/40001

[commit-bot: I haz the power, 2015-02-05 17:54:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-05 17:55:38]

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