Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
828953004:
Add AffiliatedInvalidationServiceProvider (Closed)
|
Created:
5 years, 11 months ago by bartfab (slow) Modified:
5 years, 10 months ago CC:
chromium-reviews, davemoore+watch_chromium.org, nkostylev+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd AffiliatedInvalidationServiceProvider
Device policy pushing requires a connected invalidation service which
belongs to an account that is affiliated with the device's enrollment
domain. If an affiliated user is logged in and has a connected per-profile
invalidation service, that service is used. Otherwise, a device-global
invalidation service is spun up.
This CL extracts the mechanism from DeviceCloudPolicyInvalidator and
generalizes it so that it can be used by any number of consumers. This
will allow the same invalidation service to be resued by e.g. device
policy pushing, device-local account policy pushing and device remote
commands.
The CL adds a new AffiliatedInvalidationServiceProvider with tests but
does not switch the DeviceCloudPolicyInvalidator to this new
infrastructure yet. That will be done in a follow-up CL.
BUG=442800
TEST=New unit tests
Committed: https://crrev.com/465d8a5a1709bce5780c697407f7a7c4146dfea8
Cr-Commit-Position: refs/heads/master@{#313488}
Patch Set 1 #Patch Set 2 : Added test accessors; made unit test no longer be a friend #
Total comments: 26
Patch Set 3 : Addressed comments. #Patch Set 4 : Removed obosolete code accidentally left in previous revision. #Patch Set 5 : Trimmed unit tests to only check externally visible behavior only. #
Total comments: 16
Patch Set 6 : Simplified tests. Made code reentrant. #
Total comments: 5
Patch Set 7 : Fix corner case in InvalidationServiceObserver constuctor. #Patch Set 8 : More protection against premature callbacks. #Patch Set 9 : Added AffiliatedInvalidationServiceProvider::Shutdown() method. #
Total comments: 2
Patch Set 10 : Rebased. #Patch Set 11 : Style fixes. #Messages
Total messages: 35 (5 generated)
bartfab@chromium.org changed reviewers: + dcheng@chromium.org, mnissler@chromium.org
Hi Mattias, Could you please review: chrome/browser/chromeos/policy/* Hi Daniel, Could you please review: chrome/browser/invalidation/profile_invalidation_provider_factory.h
lgtm for c/b/invalidation changes. However, it seems unfortunate to have to keep friending tests. If it's just a few things needed by the test, maybe we should expose some foo_for_test() accessors? Obviously this can wait for another patch (or doesn't have to happen at all if it's non-trivial)
On 2015/01/09 08:00:43, dcheng wrote: > lgtm for c/b/invalidation changes. > > However, it seems unfortunate to have to keep friending tests. If it's just a > few things needed by the test, maybe we should expose some foo_for_test() > accessors? Obviously this can wait for another patch (or doesn't have to happen > at all if it's non-trivial) Done, although I actually preferred the way it was before, with a friend declaration. The four new accessors are clearly named *ForTest() but they still clutter the interface with access to things that are really off limits to anyone else.
Code looks sane in general, but the unit tests aren't really focusing on the public interface of the service provider but check lots of implementation detail. This (1) makes maintenance costly and (2) makes tests very verbose and thus harder to follow. https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc (right): https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:164: syncer::INVALIDATIONS_ENABLED) { nit: I guess a more consistent way of writing this would be profile_invalidation_service_observers_.back().IsServiceConnected() Or should this even be done by the InvalidationServiceObserver ctor? https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:193: consumer->OnInvalidationServiceReset(); Uh, this is surprising to me - as a consumer I wouldn't expect any callbacks on removing myself. https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:323: FOR_EACH_OBSERVER(Consumer, nit: indentation https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h (right): https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 :) https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h:50: virtual void OnInvalidationServiceReset() = 0; Could as well just call OnInvalidationSeviceSet(nullptr)? Would that make things simpler or not? https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc (right): https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:164: return provider_.get(); Is there a good reason to have everything go through the GetProvider() helper instead of just having |provider_| as a protected member (as most other code I know does)? https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:210: EXPECT_EQ(0, GetProfileInvalidationServiceObserverCount()); These tests are very strongly tied to the implementation details of AffiliatedInvalidationServiceProvider, in particular regarding service observers and their count. This makes code maintenance costly, as one will have to re-write all the tests should anything within the implementation of AffiliatedInvalidationServiceProvider change. Can we instead focus in the tests to merely check that Consumers see what they're supposed to see? That's the public interface of the service provider after all that you want to behave as intended. https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:246: // not being made available to consumers. does it matter to assert this? https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:264: // being made available to consumers. Didn't you test this per the consumer callback expectations above already? https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:279: // being made available to consumers. ditto here https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:305: // not being made available to consumers. ditto https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:316: // been created. ditto - do we care in the test? The consumer should receive a callback eventually, isn't that enough? I'll stop reviewing the tests here pending resolution of the question how finicky the tests should be. https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right): https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:150: affiliated_invalidation_service_provider_; This is not really something that belongs to policy, I'd prefer we find a better place for it to live.
https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc (right): https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:164: syncer::INVALIDATIONS_ENABLED) { On 2015/01/13 09:50:15, Mattias Nissler wrote: > nit: I guess a more consistent way of writing this would be > > profile_invalidation_service_observers_.back().IsServiceConnected() > > Or should this even be done by the InvalidationServiceObserver ctor? Moved to the observer constructor. https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:193: consumer->OnInvalidationServiceReset(); On 2015/01/13 09:50:15, Mattias Nissler wrote: > Uh, this is surprising to me - as a consumer I wouldn't expect any callbacks on > removing myself. Removed. https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:323: FOR_EACH_OBSERVER(Consumer, On 2015/01/13 09:50:15, Mattias Nissler wrote: > nit: indentation Done. https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h (right): https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/13 09:50:15, Mattias Nissler wrote: > 2015 :) This is what happens when you write code just before Christmas but do not manage to send it out for review until the next year... :). https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h:50: virtual void OnInvalidationServiceReset() = 0; On 2015/01/13 09:50:15, Mattias Nissler wrote: > Could as well just call OnInvalidationSeviceSet(nullptr)? Would that make things > simpler or not? It shortens the unit test file a bit. Done. https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc (right): https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:164: return provider_.get(); On 2015/01/13 09:50:16, Mattias Nissler wrote: > Is there a good reason to have everything go through the GetProvider() helper > instead of just having |provider_| as a protected member (as most other code I > know does)? Just practicing good OOD encapsulation. I can make |provider_| directly accessible if you prefer. https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:210: EXPECT_EQ(0, GetProfileInvalidationServiceObserverCount()); On 2015/01/13 09:50:15, Mattias Nissler wrote: > These tests are very strongly tied to the implementation details of > AffiliatedInvalidationServiceProvider, in particular regarding service observers > and their count. This makes code maintenance costly, as one will have to > re-write all the tests should anything within the implementation of > AffiliatedInvalidationServiceProvider change. > > Can we instead focus in the tests to merely check that Consumers see what > they're supposed to see? That's the public interface of the service provider > after all that you want to behave as intended. Ensuring that consumers see the right thing is one aspect of these tests. But there are three other important aspects: 1) Ensure that the number of cloud connections is minimal. The straightforward way to verify this is to reach into the provider's bowels and verify that it is not running a device-global invalidation service when not required. 2) Ensure that the provider is monitoring affiliated connections. The straightforward way to verify this is to reach into the provider's bowels once again and verify that it is observing connections. 3) Ensure that the provider is no monitoring unaffiliated connections. As above. A nicer way to verify these properties would be to make all of the connections actually succeed against a mock invalidation server. We could then count connections on the server and send invalidations, checking which ones get honored and which ones get ignored. But AFAICT, there is no mock invalidation server in existence anywhere. Writing one would be a major undertaking. https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:246: // not being made available to consumers. On 2015/01/13 09:50:15, Mattias Nissler wrote: > does it matter to assert this? Yes, because the call in line 260 will try to call a method on the invalidation service. I went through this whole file before sending it for review and made sure I ASSERT when needed and EXPECT otherwise (modulo any mistakes I might have made, of course). https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:264: // being made available to consumers. On 2015/01/13 09:50:15, Mattias Nissler wrote: > Didn't you test this per the consumer callback expectations above already? I tested that the service is *not* made available above. I now test that the service still exists and *is* made available. https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:279: // being made available to consumers. On 2015/01/13 09:50:15, Mattias Nissler wrote: > ditto here As above, in reverse - we switched back from *is* available to is *not* available. https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:305: // not being made available to consumers. On 2015/01/13 09:50:16, Mattias Nissler wrote: > ditto This is a new test, so we check the starting state again. https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:316: // been created. On 2015/01/13 09:50:15, Mattias Nissler wrote: > ditto - do we care in the test? The consumer should receive a callback > eventually, isn't that enough? > > I'll stop reviewing the tests here pending resolution of the question how > finicky the tests should be. We can discuss this offline if you want. The thing I want to ensure are the three properties listed above. I am open to how exactly this is done. https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right): https://codereview.chromium.org/828953004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:150: affiliated_invalidation_service_provider_; On 2015/01/13 09:50:16, Mattias Nissler wrote: > This is not really something that belongs to policy, I'd prefer we find a better > place for it to live. I can move it to the platform part but I cannot think of any consumers outside policy right now. And we have three policy-related consumers: * Device policy pushing * Device-local account policy pushing * Device remote commands
Hi Mattias, Please take another look. Your persistence paid off: I significantly trimmed the unit tests so that they check externally visible state only. I agree that the tests are much better now.
Tests look much better indeed, thanks for that. Two major points of feedback: 1. Re-entrancy isn't really considered in the AffiliatedInvalidationServiceProvider code. It might not be relevant here, but experience tells me that if it's not too hard to get right from the start that may significantly reduce future pain. 2. There's still significant duplication in test driving code for the different test cases, please take a look and create helper functions to reduce the duplication and reduce noise for readers of the code. https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc (right): https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:190: if (invalidation_service_ &&consumer_count_ == 0) { nit: space after && https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:192: DestroyDeviceInvalidationService(); This pattern may be dangerous as ResetInvalidationService calls observers, which may call back into AffiliatedInvalidationServiceProvider, and then DestroyDeviceInvalidationService() does the wrong thing. Might want to have a dedicated helper for this pattern (there's another instance below) that is coded with the appropriate level of defense? https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:225: SetInvalidationService(invalidation_service); Two callbacks to consumers in this function - not good for re-entrancy... ;) https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:309: FOR_EACH_OBSERVER(Consumer, consumers_, OnInvalidationServiceSet(nullptr)); nit: It's probably more robust to reset |invalidation_service_| *before* calling out to consumer, because they may call back into the provider and cause invalidation_service_ to be recreated. I might be a bit paranoid though; I've seen too many observer re-entrancy problems in my chromium time. https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc (right): https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:202: EXPECT_CALL(consumer, OnInvalidationServiceSet(_)).Times(0); Are these expectations actually necessary, i.e. doesn't gmock also fail if it gets a call that doesn't match any expectation? I don't remember, just checking. https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:222: provider_->UnregisterConsumer(&consumer); Let's put a Mock::VerifyAndClearExpectations here for good measure - I know it's not necessary if we return directly, but who knows what code gets added to this test in the future. It's easy to miss adding the verification then. Also below. https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:386: // to the consumer. Verifies that when a per-profile invalidation service This and all tests below have a common setup sequence. Can you break that out into a helper function? It might also make sense to make a helper functions LoginAndCheckServiceSwitch() and LoginAndCheckIgnored() for other commonly used test code patterns. https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:635: Mock::VerifyAndClearExpectations(&consumer_2); nit: Put a blank line here, the expectation in the line below belongs to the verification below. Also, can you add an explicit Mock::VerifyAndClearExpectations(&consumer_2) at the end?
https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc (right): https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:190: if (invalidation_service_ &&consumer_count_ == 0) { On 2015/01/16 13:24:29, Mattias Nissler wrote: > nit: space after && Done. https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:192: DestroyDeviceInvalidationService(); On 2015/01/16 13:24:29, Mattias Nissler wrote: > This pattern may be dangerous as ResetInvalidationService calls observers, which > may call back into AffiliatedInvalidationServiceProvider, and then > DestroyDeviceInvalidationService() does the wrong thing. Might want to have a > dedicated helper for this pattern (there's another instance below) that is coded > with the appropriate level of defense? This is actually not dangerous, because we are guaranteed to have precisely zero observers. I made it more explicit by replacing |ResetInvalidationService()| with |invalidation_service_ = nullptr|. https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:225: SetInvalidationService(invalidation_service); On 2015/01/16 13:24:29, Mattias Nissler wrote: > Two callbacks to consumers in this function - not good for re-entrancy... ;) Changed to a single callback. https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:309: FOR_EACH_OBSERVER(Consumer, consumers_, OnInvalidationServiceSet(nullptr)); On 2015/01/16 13:24:29, Mattias Nissler wrote: > nit: It's probably more robust to reset |invalidation_service_| *before* calling > out to consumer, because they may call back into the provider and cause > invalidation_service_ to be recreated. I might be a bit paranoid though; I've > seen too many observer re-entrancy problems in my chromium time. Done. https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc (right): https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:202: EXPECT_CALL(consumer, OnInvalidationServiceSet(_)).Times(0); On 2015/01/16 13:24:29, Mattias Nissler wrote: > Are these expectations actually necessary, i.e. doesn't gmock also fail if it > gets a call that doesn't match any expectation? I don't remember, just checking. Regular Mock just prints a warning that there was an "uninteresting call" and does not fail. But I had forgotten that there is StrictMock as well. I switched to that now. https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:222: provider_->UnregisterConsumer(&consumer); On 2015/01/16 13:24:29, Mattias Nissler wrote: > Let's put a Mock::VerifyAndClearExpectations here for good measure - I know it's > not necessary if we return directly, but who knows what code gets added to this > test in the future. It's easy to miss adding the verification then. Also below. Done. https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:386: // to the consumer. Verifies that when a per-profile invalidation service On 2015/01/16 13:24:29, Mattias Nissler wrote: > This and all tests below have a common setup sequence. Can you break that out > into a helper function? It might also make sense to make a helper functions > LoginAndCheckServiceSwitch() and LoginAndCheckIgnored() for other commonly used > test code patterns. Done. https://codereview.chromium.org/828953004/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_unittest.cc:635: Mock::VerifyAndClearExpectations(&consumer_2); On 2015/01/16 13:24:29, Mattias Nissler wrote: > nit: Put a blank line here, the expectation in the line below belongs to the > verification below. Also, can you add an explicit > Mock::VerifyAndClearExpectations(&consumer_2) at the end? Done.
Looks good mostly, just one more quick suggestion for an improvement. Please consider, I might be missing details about whether this helps things or not - so let me know in case it doesn't make sense. https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc (right): https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:247: if (!invalidation_service_) Still doing some work here after a call to the observer returns, which could have changed internal state. Maybe it's better (and more readable?) to have FindConnectedInvalidationService return the service it picked and then fire the update here?
https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc (right): https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:247: if (!invalidation_service_) On 2015/01/21 12:57:28, Mattias Nissler wrote: > Still doing some work here after a call to the observer returns, which could > have changed internal state. Maybe it's better (and more readable?) to have > FindConnectedInvalidationService return the service it picked and then fire the > update here? I had considered this. However, there is one code path in which FindConnectedInvalidationService() does not explicitly set the |invalidation_service_| itself: If there is no |device_invalidation_service_| yet, FindConnectedInvalidationService() creates one. If this service reports that it is connected immediately (unlikely as a client-server handshake needs to happen first but I need to be compatible with all possible future implementations, including e.g. one that just latches onto a cached connection immediately), its observer will trigger SetInvalidationService(). Thus, at the end of FindConnectedInvalidationService(), there are three options: 1) SetInvalidationService() was called by FindConnectedInvalidationService() explicitly. 2) SetInvalidationService() was called by other code that FindConnectedInvalidationService() triggered. 3) SetInvalidationService() was never called. |invalidation_service_| is a nullptr. FindConnectedInvalidationService() can report (1) reliably, but it cannot distinguish between (2) and (3). Thus, you have to look at |invalidation_service_| in some cases. I could move this into FindConnectedInvalidationService() and only look at |invalidation_service_| in the code path that needs to distinguish between (2) and (3). But I find it cleaner, simpler and less error-prone to unconditionally look at |invalidation_service_| here instead, allowing us to always distinguish between (1)/(2) and (3).
https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc (right): https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:247: if (!invalidation_service_) On 2015/01/21 13:38:48, bartfab wrote: > On 2015/01/21 12:57:28, Mattias Nissler wrote: > > Still doing some work here after a call to the observer returns, which could > > have changed internal state. Maybe it's better (and more readable?) to have > > FindConnectedInvalidationService return the service it picked and then fire > the > > update here? > > I had considered this. However, there is one code path in which > FindConnectedInvalidationService() does not explicitly set the > |invalidation_service_| itself: > > If there is no |device_invalidation_service_| yet, > FindConnectedInvalidationService() creates one. If this service reports that it > is connected immediately (unlikely as a client-server handshake needs to happen > first but I need to be compatible with all possible future implementations, > including e.g. one that just latches onto a cached connection immediately), its > observer will trigger SetInvalidationService(). > > Thus, at the end of FindConnectedInvalidationService(), there are three options: > 1) SetInvalidationService() was called by FindConnectedInvalidationService() > explicitly. > 2) SetInvalidationService() was called by other code that > FindConnectedInvalidationService() triggered. > 3) SetInvalidationService() was never called. |invalidation_service_| is a > nullptr. > > FindConnectedInvalidationService() can report (1) reliably, but it cannot > distinguish between (2) and (3). Thus, you have to look at > |invalidation_service_| in some cases. I could move this into > FindConnectedInvalidationService() and only look at |invalidation_service_| in > the code path that needs to distinguish between (2) and (3). But I find it > cleaner, simpler and less error-prone to unconditionally look at > |invalidation_service_| here instead, allowing us to always distinguish between > (1)/(2) and (3). A simple way to get to the point where SetInvalidationService doesn't get called for cases #2 and #3 is to remove the IsServiceConnected check in the InvalidationServiceObserver ctor and perform the IsServiceConnected check in the caller. That makes the block starting in lines 289 go away, and a check would have to be added in line 166 (I think you had that initially). Do you think that'd be clearer?
On 2015/01/21 14:19:26, Mattias Nissler wrote: > https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... > File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc > (right): > > https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... > chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:247: > if (!invalidation_service_) > On 2015/01/21 13:38:48, bartfab wrote: > > On 2015/01/21 12:57:28, Mattias Nissler wrote: > > > Still doing some work here after a call to the observer returns, which could > > > have changed internal state. Maybe it's better (and more readable?) to have > > > FindConnectedInvalidationService return the service it picked and then fire > > the > > > update here? > > > > I had considered this. However, there is one code path in which > > FindConnectedInvalidationService() does not explicitly set the > > |invalidation_service_| itself: > > > > If there is no |device_invalidation_service_| yet, > > FindConnectedInvalidationService() creates one. If this service reports that > it > > is connected immediately (unlikely as a client-server handshake needs to > happen > > first but I need to be compatible with all possible future implementations, > > including e.g. one that just latches onto a cached connection immediately), > its > > observer will trigger SetInvalidationService(). > > > > Thus, at the end of FindConnectedInvalidationService(), there are three > options: > > 1) SetInvalidationService() was called by FindConnectedInvalidationService() > > explicitly. > > 2) SetInvalidationService() was called by other code that > > FindConnectedInvalidationService() triggered. > > 3) SetInvalidationService() was never called. |invalidation_service_| is a > > nullptr. > > > > FindConnectedInvalidationService() can report (1) reliably, but it cannot > > distinguish between (2) and (3). Thus, you have to look at > > |invalidation_service_| in some cases. I could move this into > > FindConnectedInvalidationService() and only look at |invalidation_service_| in > > the code path that needs to distinguish between (2) and (3). But I find it > > cleaner, simpler and less error-prone to unconditionally look at > > |invalidation_service_| here instead, allowing us to always distinguish > between > > (1)/(2) and (3). > > A simple way to get to the point where SetInvalidationService doesn't get called > for cases #2 and #3 is to remove the IsServiceConnected check in the > InvalidationServiceObserver ctor and perform the IsServiceConnected check in the > caller. That makes the block starting in lines 289 go away, and a check would > have to be added in line 166 (I think you had that initially). Do you think > that'd be clearer? Moving the IsServiceConnected() check to the constructor was your suggestion :), see: https://codereview.chromium.org/828953004/diff2/20001:40001/chrome/browser/ch... While thinking about this, I discovered a reentrancy problem in the observer constructor: 1) Initially, the invalidation service is not connected. 2) While the observer is registering itself with the service (line 75), the service connects and fires an OnInvalidatorStateChange(), which triggers OnInvalidationServiceConnected(). 3) The observer determines that the service is connected (line 76), triggering OnInvalidationServiceConnected(). We now have two calls to OnInvalidationServiceConnected(), both of which call SetInvalidationService(), running into DCHECK(!invalidation_service_) on the second call. I uploaded a patch set that fixes this. If we move the IsServiceConnected() out of the constructor, we will still have the problem that the invalidation service can trigger an OnInvalidatorStateChange() during observer construction. And the only way to deal with this that I can see is to explicitly look at |invalidation_service_| once again.
https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc (right): https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:247: if (!invalidation_service_) On 2015/01/21 14:19:26, Mattias Nissler wrote: > On 2015/01/21 13:38:48, bartfab wrote: > > On 2015/01/21 12:57:28, Mattias Nissler wrote: > > > Still doing some work here after a call to the observer returns, which could > > > have changed internal state. Maybe it's better (and more readable?) to have > > > FindConnectedInvalidationService return the service it picked and then fire > > the > > > update here? > > > > I had considered this. However, there is one code path in which > > FindConnectedInvalidationService() does not explicitly set the > > |invalidation_service_| itself: > > > > If there is no |device_invalidation_service_| yet, > > FindConnectedInvalidationService() creates one. If this service reports that > it > > is connected immediately (unlikely as a client-server handshake needs to > happen > > first but I need to be compatible with all possible future implementations, > > including e.g. one that just latches onto a cached connection immediately), > its > > observer will trigger SetInvalidationService(). > > > > Thus, at the end of FindConnectedInvalidationService(), there are three > options: > > 1) SetInvalidationService() was called by FindConnectedInvalidationService() > > explicitly. > > 2) SetInvalidationService() was called by other code that > > FindConnectedInvalidationService() triggered. > > 3) SetInvalidationService() was never called. |invalidation_service_| is a > > nullptr. > > > > FindConnectedInvalidationService() can report (1) reliably, but it cannot > > distinguish between (2) and (3). Thus, you have to look at > > |invalidation_service_| in some cases. I could move this into > > FindConnectedInvalidationService() and only look at |invalidation_service_| in > > the code path that needs to distinguish between (2) and (3). But I find it > > cleaner, simpler and less error-prone to unconditionally look at > > |invalidation_service_| here instead, allowing us to always distinguish > between > > (1)/(2) and (3). > > A simple way to get to the point where SetInvalidationService doesn't get called > for cases #2 and #3 is to remove the IsServiceConnected check in the > InvalidationServiceObserver ctor and perform the IsServiceConnected check in the > caller. That makes the block starting in lines 289 go away, and a check would > have to be added in line 166 (I think you had that initially). Do you think > that'd be clearer? *Argh*, Rietveld ate my homework and deleted the entire comment I had typed :(. I will start again: I had moved IsServiceConnected() to the observer constructor at your explicit request :), see: https://codereview.chromium.org/828953004/diff2/20001:40001/chrome/browser/ch... While thinking about this, I noticed another corner case in the observer constructor that the newest patch set fixes: 1) The invalidaiton service is not connected at first. 2) The service connects while the observer is registering as an observer (line 75), calling OnInvalidatorStateChange(), which triggers OnInvalidationServiceConnected() 3) The observer notices that the service is connected (line 76), triggering OnInvalidationServiceConnected(). We now have two calls to OnInvalidationServiceConnected(), both of which call SetInvalidationService(). The second call runs into DCHECK(!invalidation_service_); (line 299). If we move the IsServiceConnected() check out of the observer constructor, we will still have to check somehow whether the service happens to have triggered an SetInvalidationService() before we got to the IsServiceConnected() check. The only way to do this that I can see is to look at |invalidation_service_| once again. So nothing is gained.
https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc (right): https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:247: if (!invalidation_service_) On 2015/01/21 14:45:23, bartfab wrote: > On 2015/01/21 14:19:26, Mattias Nissler wrote: > > On 2015/01/21 13:38:48, bartfab wrote: > > > On 2015/01/21 12:57:28, Mattias Nissler wrote: > > > > Still doing some work here after a call to the observer returns, which > could > > > > have changed internal state. Maybe it's better (and more readable?) to > have > > > > FindConnectedInvalidationService return the service it picked and then > fire > > > the > > > > update here? > > > > > > I had considered this. However, there is one code path in which > > > FindConnectedInvalidationService() does not explicitly set the > > > |invalidation_service_| itself: > > > > > > If there is no |device_invalidation_service_| yet, > > > FindConnectedInvalidationService() creates one. If this service reports that > > it > > > is connected immediately (unlikely as a client-server handshake needs to > > happen > > > first but I need to be compatible with all possible future implementations, > > > including e.g. one that just latches onto a cached connection immediately), > > its > > > observer will trigger SetInvalidationService(). > > > > > > Thus, at the end of FindConnectedInvalidationService(), there are three > > options: > > > 1) SetInvalidationService() was called by FindConnectedInvalidationService() > > > explicitly. > > > 2) SetInvalidationService() was called by other code that > > > FindConnectedInvalidationService() triggered. > > > 3) SetInvalidationService() was never called. |invalidation_service_| is a > > > nullptr. > > > > > > FindConnectedInvalidationService() can report (1) reliably, but it cannot > > > distinguish between (2) and (3). Thus, you have to look at > > > |invalidation_service_| in some cases. I could move this into > > > FindConnectedInvalidationService() and only look at |invalidation_service_| > in > > > the code path that needs to distinguish between (2) and (3). But I find it > > > cleaner, simpler and less error-prone to unconditionally look at > > > |invalidation_service_| here instead, allowing us to always distinguish > > between > > > (1)/(2) and (3). > > > > A simple way to get to the point where SetInvalidationService doesn't get > called > > for cases #2 and #3 is to remove the IsServiceConnected check in the > > InvalidationServiceObserver ctor and perform the IsServiceConnected check in > the > > caller. That makes the block starting in lines 289 go away, and a check would > > have to be added in line 166 (I think you had that initially). Do you think > > that'd be clearer? > > *Argh*, Rietveld ate my homework and deleted the entire comment I had typed :(. > I will start again: > > I had moved IsServiceConnected() to the observer constructor at your explicit > request :), see: > > https://codereview.chromium.org/828953004/diff2/20001:40001/chrome/browser/ch... > > While thinking about this, I noticed another corner case in the observer > constructor that the newest patch set fixes: > > 1) The invalidaiton service is not connected at first. > 2) The service connects while the observer is registering as an observer (line > 75), calling OnInvalidatorStateChange(), which triggers > OnInvalidationServiceConnected() > 3) The observer notices that the service is connected (line 76), triggering > OnInvalidationServiceConnected(). > > We now have two calls to OnInvalidationServiceConnected(), both of which call > SetInvalidationService(). The second call runs into > DCHECK(!invalidation_service_); (line 299). > > If we move the IsServiceConnected() check out of the observer constructor, we > will still have to check somehow whether the service happens to have triggered > an SetInvalidationService() before we got to the IsServiceConnected() check. The > only way to do this that I can see is to look at |invalidation_service_| once > again. So nothing is gained. The problem you've found with RegisterInvalidationHandler() triggering re-entrancy is even worse - what if the profile and with it the profile's invalidation service go away in the mean time? Tangential question: Are you handling the case of profiles going away properly at all? I can't find a place where observers are removed fro |profile_invalidation_service_observers_|. Anyhow, an additional problem will be that while you call RegisterInvalidationHandler(), the OnValidationServiceConnected() callback will fire at a time where the newly-created observer isn't present in profile_invalidation_service_observers_, which can only lead to trouble. Maybe it's best to ignore callbacks until you've wired everything up correctly? Once you arm the observer, you would then also perform the IsConnected() check.
On 2015/01/21 14:59:58, Mattias Nissler wrote: > https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... > File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc > (right): > > https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... > chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:247: > if (!invalidation_service_) > On 2015/01/21 14:45:23, bartfab wrote: > > On 2015/01/21 14:19:26, Mattias Nissler wrote: > > > On 2015/01/21 13:38:48, bartfab wrote: > > > > On 2015/01/21 12:57:28, Mattias Nissler wrote: > > > > > Still doing some work here after a call to the observer returns, which > > could > > > > > have changed internal state. Maybe it's better (and more readable?) to > > have > > > > > FindConnectedInvalidationService return the service it picked and then > > fire > > > > the > > > > > update here? > > > > > > > > I had considered this. However, there is one code path in which > > > > FindConnectedInvalidationService() does not explicitly set the > > > > |invalidation_service_| itself: > > > > > > > > If there is no |device_invalidation_service_| yet, > > > > FindConnectedInvalidationService() creates one. If this service reports > that > > > it > > > > is connected immediately (unlikely as a client-server handshake needs to > > > happen > > > > first but I need to be compatible with all possible future > implementations, > > > > including e.g. one that just latches onto a cached connection > immediately), > > > its > > > > observer will trigger SetInvalidationService(). > > > > > > > > Thus, at the end of FindConnectedInvalidationService(), there are three > > > options: > > > > 1) SetInvalidationService() was called by > FindConnectedInvalidationService() > > > > explicitly. > > > > 2) SetInvalidationService() was called by other code that > > > > FindConnectedInvalidationService() triggered. > > > > 3) SetInvalidationService() was never called. |invalidation_service_| is a > > > > nullptr. > > > > > > > > FindConnectedInvalidationService() can report (1) reliably, but it cannot > > > > distinguish between (2) and (3). Thus, you have to look at > > > > |invalidation_service_| in some cases. I could move this into > > > > FindConnectedInvalidationService() and only look at > |invalidation_service_| > > in > > > > the code path that needs to distinguish between (2) and (3). But I find it > > > > cleaner, simpler and less error-prone to unconditionally look at > > > > |invalidation_service_| here instead, allowing us to always distinguish > > > between > > > > (1)/(2) and (3). > > > > > > A simple way to get to the point where SetInvalidationService doesn't get > > called > > > for cases #2 and #3 is to remove the IsServiceConnected check in the > > > InvalidationServiceObserver ctor and perform the IsServiceConnected check in > > the > > > caller. That makes the block starting in lines 289 go away, and a check > would > > > have to be added in line 166 (I think you had that initially). Do you think > > > that'd be clearer? > > > > *Argh*, Rietveld ate my homework and deleted the entire comment I had typed > :(. > > I will start again: > > > > I had moved IsServiceConnected() to the observer constructor at your explicit > > request :), see: > > > > > https://codereview.chromium.org/828953004/diff2/20001:40001/chrome/browser/ch... > > > > While thinking about this, I noticed another corner case in the observer > > constructor that the newest patch set fixes: > > > > 1) The invalidaiton service is not connected at first. > > 2) The service connects while the observer is registering as an observer (line > > 75), calling OnInvalidatorStateChange(), which triggers > > OnInvalidationServiceConnected() > > 3) The observer notices that the service is connected (line 76), triggering > > OnInvalidationServiceConnected(). > > > > We now have two calls to OnInvalidationServiceConnected(), both of which call > > SetInvalidationService(). The second call runs into > > DCHECK(!invalidation_service_); (line 299). > > > > If we move the IsServiceConnected() check out of the observer constructor, we > > will still have to check somehow whether the service happens to have triggered > > an SetInvalidationService() before we got to the IsServiceConnected() check. > The > > only way to do this that I can see is to look at |invalidation_service_| once > > again. So nothing is gained. > > The problem you've found with RegisterInvalidationHandler() triggering > re-entrancy is even worse - what if the profile and with it the profile's > invalidation service go away in the mean time? > > Tangential question: Are you handling the case of profiles going away properly > at all? I can't find a place where observers are removed fro > |profile_invalidation_service_observers_|. AFAIK, we only destroy Profiles during shutdown, after the AffiliatedInvalidationServiceProvider has been destroyed. Thus, there should be no need to handle Profiles going away. There is lots of code in Chrome that would go up in flames if Profiles just went away mid-flight. > > Anyhow, an additional problem will be that while you call > RegisterInvalidationHandler(), the OnValidationServiceConnected() callback will > fire at a time where the newly-created observer isn't present in > profile_invalidation_service_observers_, which can only lead to trouble. Maybe > it's best to ignore callbacks until you've wired everything up correctly? Once > you arm the observer, you would then also perform the IsConnected() check. Makes sense. Done.
On 2015/01/21 15:49:38, bartfab wrote: > On 2015/01/21 14:59:58, Mattias Nissler wrote: > > > https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... > > File > chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc > > (right): > > > > > https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... > > > chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:247: > > if (!invalidation_service_) > > On 2015/01/21 14:45:23, bartfab wrote: > > > On 2015/01/21 14:19:26, Mattias Nissler wrote: > > > > On 2015/01/21 13:38:48, bartfab wrote: > > > > > On 2015/01/21 12:57:28, Mattias Nissler wrote: > > > > > > Still doing some work here after a call to the observer returns, which > > > could > > > > > > have changed internal state. Maybe it's better (and more readable?) to > > > have > > > > > > FindConnectedInvalidationService return the service it picked and then > > > fire > > > > > the > > > > > > update here? > > > > > > > > > > I had considered this. However, there is one code path in which > > > > > FindConnectedInvalidationService() does not explicitly set the > > > > > |invalidation_service_| itself: > > > > > > > > > > If there is no |device_invalidation_service_| yet, > > > > > FindConnectedInvalidationService() creates one. If this service reports > > that > > > > it > > > > > is connected immediately (unlikely as a client-server handshake needs to > > > > happen > > > > > first but I need to be compatible with all possible future > > implementations, > > > > > including e.g. one that just latches onto a cached connection > > immediately), > > > > its > > > > > observer will trigger SetInvalidationService(). > > > > > > > > > > Thus, at the end of FindConnectedInvalidationService(), there are three > > > > options: > > > > > 1) SetInvalidationService() was called by > > FindConnectedInvalidationService() > > > > > explicitly. > > > > > 2) SetInvalidationService() was called by other code that > > > > > FindConnectedInvalidationService() triggered. > > > > > 3) SetInvalidationService() was never called. |invalidation_service_| is > a > > > > > nullptr. > > > > > > > > > > FindConnectedInvalidationService() can report (1) reliably, but it > cannot > > > > > distinguish between (2) and (3). Thus, you have to look at > > > > > |invalidation_service_| in some cases. I could move this into > > > > > FindConnectedInvalidationService() and only look at > > |invalidation_service_| > > > in > > > > > the code path that needs to distinguish between (2) and (3). But I find > it > > > > > cleaner, simpler and less error-prone to unconditionally look at > > > > > |invalidation_service_| here instead, allowing us to always distinguish > > > > between > > > > > (1)/(2) and (3). > > > > > > > > A simple way to get to the point where SetInvalidationService doesn't get > > > called > > > > for cases #2 and #3 is to remove the IsServiceConnected check in the > > > > InvalidationServiceObserver ctor and perform the IsServiceConnected check > in > > > the > > > > caller. That makes the block starting in lines 289 go away, and a check > > would > > > > have to be added in line 166 (I think you had that initially). Do you > think > > > > that'd be clearer? > > > > > > *Argh*, Rietveld ate my homework and deleted the entire comment I had typed > > :(. > > > I will start again: > > > > > > I had moved IsServiceConnected() to the observer constructor at your > explicit > > > request :), see: > > > > > > > > > https://codereview.chromium.org/828953004/diff2/20001:40001/chrome/browser/ch... > > > > > > While thinking about this, I noticed another corner case in the observer > > > constructor that the newest patch set fixes: > > > > > > 1) The invalidaiton service is not connected at first. > > > 2) The service connects while the observer is registering as an observer > (line > > > 75), calling OnInvalidatorStateChange(), which triggers > > > OnInvalidationServiceConnected() > > > 3) The observer notices that the service is connected (line 76), triggering > > > OnInvalidationServiceConnected(). > > > > > > We now have two calls to OnInvalidationServiceConnected(), both of which > call > > > SetInvalidationService(). The second call runs into > > > DCHECK(!invalidation_service_); (line 299). > > > > > > If we move the IsServiceConnected() check out of the observer constructor, > we > > > will still have to check somehow whether the service happens to have > triggered > > > an SetInvalidationService() before we got to the IsServiceConnected() check. > > The > > > only way to do this that I can see is to look at |invalidation_service_| > once > > > again. So nothing is gained. > > > > The problem you've found with RegisterInvalidationHandler() triggering > > re-entrancy is even worse - what if the profile and with it the profile's > > invalidation service go away in the mean time? > > > > Tangential question: Are you handling the case of profiles going away properly > > at all? I can't find a place where observers are removed fro > > |profile_invalidation_service_observers_|. > > AFAIK, we only destroy Profiles during shutdown, after the > AffiliatedInvalidationServiceProvider has been destroyed. Thus, there should be > no need to handle Profiles going away. There is lots of code in Chrome that > would go up in flames if Profiles just went away mid-flight. Can you please double-check whether that assumption holds for secondary profiles in multi-profile sessions? At least for desktop Chrome, Profiles do definitely get destroyed (this was one motivation to do the profile-keyed-service stuff IIRC). > > > > > Anyhow, an additional problem will be that while you call > > RegisterInvalidationHandler(), the OnValidationServiceConnected() callback > will > > fire at a time where the newly-created observer isn't present in > > profile_invalidation_service_observers_, which can only lead to trouble. Maybe > > it's best to ignore callbacks until you've wired everything up correctly? Once > > you arm the observer, you would then also perform the IsConnected() check. > > Makes sense. Done.
On 2015/01/21 15:56:38, Mattias Nissler wrote: > On 2015/01/21 15:49:38, bartfab wrote: > > On 2015/01/21 14:59:58, Mattias Nissler wrote: > > > > > > https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... > > > File > > chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... > > > > > > chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:247: > > > if (!invalidation_service_) > > > On 2015/01/21 14:45:23, bartfab wrote: > > > > On 2015/01/21 14:19:26, Mattias Nissler wrote: > > > > > On 2015/01/21 13:38:48, bartfab wrote: > > > > > > On 2015/01/21 12:57:28, Mattias Nissler wrote: > > > > > > > Still doing some work here after a call to the observer returns, > which > > > > could > > > > > > > have changed internal state. Maybe it's better (and more readable?) > to > > > > have > > > > > > > FindConnectedInvalidationService return the service it picked and > then > > > > fire > > > > > > the > > > > > > > update here? > > > > > > > > > > > > I had considered this. However, there is one code path in which > > > > > > FindConnectedInvalidationService() does not explicitly set the > > > > > > |invalidation_service_| itself: > > > > > > > > > > > > If there is no |device_invalidation_service_| yet, > > > > > > FindConnectedInvalidationService() creates one. If this service > reports > > > that > > > > > it > > > > > > is connected immediately (unlikely as a client-server handshake needs > to > > > > > happen > > > > > > first but I need to be compatible with all possible future > > > implementations, > > > > > > including e.g. one that just latches onto a cached connection > > > immediately), > > > > > its > > > > > > observer will trigger SetInvalidationService(). > > > > > > > > > > > > Thus, at the end of FindConnectedInvalidationService(), there are > three > > > > > options: > > > > > > 1) SetInvalidationService() was called by > > > FindConnectedInvalidationService() > > > > > > explicitly. > > > > > > 2) SetInvalidationService() was called by other code that > > > > > > FindConnectedInvalidationService() triggered. > > > > > > 3) SetInvalidationService() was never called. |invalidation_service_| > is > > a > > > > > > nullptr. > > > > > > > > > > > > FindConnectedInvalidationService() can report (1) reliably, but it > > cannot > > > > > > distinguish between (2) and (3). Thus, you have to look at > > > > > > |invalidation_service_| in some cases. I could move this into > > > > > > FindConnectedInvalidationService() and only look at > > > |invalidation_service_| > > > > in > > > > > > the code path that needs to distinguish between (2) and (3). But I > find > > it > > > > > > cleaner, simpler and less error-prone to unconditionally look at > > > > > > |invalidation_service_| here instead, allowing us to always > distinguish > > > > > between > > > > > > (1)/(2) and (3). > > > > > > > > > > A simple way to get to the point where SetInvalidationService doesn't > get > > > > called > > > > > for cases #2 and #3 is to remove the IsServiceConnected check in the > > > > > InvalidationServiceObserver ctor and perform the IsServiceConnected > check > > in > > > > the > > > > > caller. That makes the block starting in lines 289 go away, and a check > > > would > > > > > have to be added in line 166 (I think you had that initially). Do you > > think > > > > > that'd be clearer? > > > > > > > > *Argh*, Rietveld ate my homework and deleted the entire comment I had > typed > > > :(. > > > > I will start again: > > > > > > > > I had moved IsServiceConnected() to the observer constructor at your > > explicit > > > > request :), see: > > > > > > > > > > > > > > https://codereview.chromium.org/828953004/diff2/20001:40001/chrome/browser/ch... > > > > > > > > While thinking about this, I noticed another corner case in the observer > > > > constructor that the newest patch set fixes: > > > > > > > > 1) The invalidaiton service is not connected at first. > > > > 2) The service connects while the observer is registering as an observer > > (line > > > > 75), calling OnInvalidatorStateChange(), which triggers > > > > OnInvalidationServiceConnected() > > > > 3) The observer notices that the service is connected (line 76), > triggering > > > > OnInvalidationServiceConnected(). > > > > > > > > We now have two calls to OnInvalidationServiceConnected(), both of which > > call > > > > SetInvalidationService(). The second call runs into > > > > DCHECK(!invalidation_service_); (line 299). > > > > > > > > If we move the IsServiceConnected() check out of the observer constructor, > > we > > > > will still have to check somehow whether the service happens to have > > triggered > > > > an SetInvalidationService() before we got to the IsServiceConnected() > check. > > > The > > > > only way to do this that I can see is to look at |invalidation_service_| > > once > > > > again. So nothing is gained. > > > > > > The problem you've found with RegisterInvalidationHandler() triggering > > > re-entrancy is even worse - what if the profile and with it the profile's > > > invalidation service go away in the mean time? > > > > > > Tangential question: Are you handling the case of profiles going away > properly > > > at all? I can't find a place where observers are removed fro > > > |profile_invalidation_service_observers_|. > > > > AFAIK, we only destroy Profiles during shutdown, after the > > AffiliatedInvalidationServiceProvider has been destroyed. Thus, there should > be > > no need to handle Profiles going away. There is lots of code in Chrome that > > would go up in flames if Profiles just went away mid-flight. > > Can you please double-check whether that assumption holds for secondary profiles > in multi-profile sessions? At least for desktop Chrome, Profiles do definitely > get destroyed (this was one motivation to do the profile-keyed-service stuff > IIRC). Yes, it definitely holds for Chrome OS multi-profile sessions. You can add more users to a session but you cannot remove them. Signing out restarts the browser and signs you out of all profiles. > > > > > > > > > Anyhow, an additional problem will be that while you call > > > RegisterInvalidationHandler(), the OnValidationServiceConnected() callback > > will > > > fire at a time where the newly-created observer isn't present in > > > profile_invalidation_service_observers_, which can only lead to trouble. > Maybe > > > it's best to ignore callbacks until you've wired everything up correctly? > Once > > > you arm the observer, you would then also perform the IsConnected() check. > > > > Makes sense. Done.
On 2015/01/21 15:58:09, bartfab wrote: > On 2015/01/21 15:56:38, Mattias Nissler wrote: > > On 2015/01/21 15:49:38, bartfab wrote: > > > On 2015/01/21 14:59:58, Mattias Nissler wrote: > > > > > > > > > > https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... > > > > File > > > chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... > > > > > > > > > > chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:247: > > > > if (!invalidation_service_) > > > > On 2015/01/21 14:45:23, bartfab wrote: > > > > > On 2015/01/21 14:19:26, Mattias Nissler wrote: > > > > > > On 2015/01/21 13:38:48, bartfab wrote: > > > > > > > On 2015/01/21 12:57:28, Mattias Nissler wrote: > > > > > > > > Still doing some work here after a call to the observer returns, > > which > > > > > could > > > > > > > > have changed internal state. Maybe it's better (and more > readable?) > > to > > > > > have > > > > > > > > FindConnectedInvalidationService return the service it picked and > > then > > > > > fire > > > > > > > the > > > > > > > > update here? > > > > > > > > > > > > > > I had considered this. However, there is one code path in which > > > > > > > FindConnectedInvalidationService() does not explicitly set the > > > > > > > |invalidation_service_| itself: > > > > > > > > > > > > > > If there is no |device_invalidation_service_| yet, > > > > > > > FindConnectedInvalidationService() creates one. If this service > > reports > > > > that > > > > > > it > > > > > > > is connected immediately (unlikely as a client-server handshake > needs > > to > > > > > > happen > > > > > > > first but I need to be compatible with all possible future > > > > implementations, > > > > > > > including e.g. one that just latches onto a cached connection > > > > immediately), > > > > > > its > > > > > > > observer will trigger SetInvalidationService(). > > > > > > > > > > > > > > Thus, at the end of FindConnectedInvalidationService(), there are > > three > > > > > > options: > > > > > > > 1) SetInvalidationService() was called by > > > > FindConnectedInvalidationService() > > > > > > > explicitly. > > > > > > > 2) SetInvalidationService() was called by other code that > > > > > > > FindConnectedInvalidationService() triggered. > > > > > > > 3) SetInvalidationService() was never called. > |invalidation_service_| > > is > > > a > > > > > > > nullptr. > > > > > > > > > > > > > > FindConnectedInvalidationService() can report (1) reliably, but it > > > cannot > > > > > > > distinguish between (2) and (3). Thus, you have to look at > > > > > > > |invalidation_service_| in some cases. I could move this into > > > > > > > FindConnectedInvalidationService() and only look at > > > > |invalidation_service_| > > > > > in > > > > > > > the code path that needs to distinguish between (2) and (3). But I > > find > > > it > > > > > > > cleaner, simpler and less error-prone to unconditionally look at > > > > > > > |invalidation_service_| here instead, allowing us to always > > distinguish > > > > > > between > > > > > > > (1)/(2) and (3). > > > > > > > > > > > > A simple way to get to the point where SetInvalidationService doesn't > > get > > > > > called > > > > > > for cases #2 and #3 is to remove the IsServiceConnected check in the > > > > > > InvalidationServiceObserver ctor and perform the IsServiceConnected > > check > > > in > > > > > the > > > > > > caller. That makes the block starting in lines 289 go away, and a > check > > > > would > > > > > > have to be added in line 166 (I think you had that initially). Do you > > > think > > > > > > that'd be clearer? > > > > > > > > > > *Argh*, Rietveld ate my homework and deleted the entire comment I had > > typed > > > > :(. > > > > > I will start again: > > > > > > > > > > I had moved IsServiceConnected() to the observer constructor at your > > > explicit > > > > > request :), see: > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/828953004/diff2/20001:40001/chrome/browser/ch... > > > > > > > > > > While thinking about this, I noticed another corner case in the observer > > > > > constructor that the newest patch set fixes: > > > > > > > > > > 1) The invalidaiton service is not connected at first. > > > > > 2) The service connects while the observer is registering as an observer > > > (line > > > > > 75), calling OnInvalidatorStateChange(), which triggers > > > > > OnInvalidationServiceConnected() > > > > > 3) The observer notices that the service is connected (line 76), > > triggering > > > > > OnInvalidationServiceConnected(). > > > > > > > > > > We now have two calls to OnInvalidationServiceConnected(), both of which > > > call > > > > > SetInvalidationService(). The second call runs into > > > > > DCHECK(!invalidation_service_); (line 299). > > > > > > > > > > If we move the IsServiceConnected() check out of the observer > constructor, > > > we > > > > > will still have to check somehow whether the service happens to have > > > triggered > > > > > an SetInvalidationService() before we got to the IsServiceConnected() > > check. > > > > The > > > > > only way to do this that I can see is to look at |invalidation_service_| > > > once > > > > > again. So nothing is gained. > > > > > > > > The problem you've found with RegisterInvalidationHandler() triggering > > > > re-entrancy is even worse - what if the profile and with it the profile's > > > > invalidation service go away in the mean time? > > > > > > > > Tangential question: Are you handling the case of profiles going away > > properly > > > > at all? I can't find a place where observers are removed fro > > > > |profile_invalidation_service_observers_|. > > > > > > AFAIK, we only destroy Profiles during shutdown, after the > > > AffiliatedInvalidationServiceProvider has been destroyed. Thus, there should > > be > > > no need to handle Profiles going away. There is lots of code in Chrome that > > > would go up in flames if Profiles just went away mid-flight. > > > > Can you please double-check whether that assumption holds for secondary > profiles > > in multi-profile sessions? At least for desktop Chrome, Profiles do definitely > > get destroyed (this was one motivation to do the profile-keyed-service stuff > > IIRC). > > Yes, it definitely holds for Chrome OS multi-profile sessions. You can add more > users to a session but you cannot remove them. Signing out restarts the browser > and signs you out of all profiles. OK, so after this comment I was wondering why you don't crash and burn when the Profiles get destroyed and the ProfileInvalidationService instances send disconnects in arbitrary order, forcing attempts to locate alternate connected ProfileInvalidationService instances and hitting objects that have already been deleted. I now realized that you conveniently sidestep this problem by creative service lifetime management in BrowserPolicyConnectorChromeOS::PreShutdown(): AffiliatedInvalidationServiceProvider must be created *before* any Profile comes up, but needs to be destroyed *before* any Profile gets destroyed again. Eeeek. That'll also mean that any consumers of AffiliatedInvalidationServiceProvider are now forced to use early shutdown or destruction, which only proliferates the lifetime nesting violation. I'm not comfortable with this design TBH. The proper and more intuitive way is to create AffiliatedInvalidationServiceProvider *before* all Profiles and consumers and then destroy it *after* all Profiles and consumers are gone. > > > > > > > > > > > > > > Anyhow, an additional problem will be that while you call > > > > RegisterInvalidationHandler(), the OnValidationServiceConnected() callback > > > will > > > > fire at a time where the newly-created observer isn't present in > > > > profile_invalidation_service_observers_, which can only lead to trouble. > > Maybe > > > > it's best to ignore callbacks until you've wired everything up correctly? > > Once > > > > you arm the observer, you would then also perform the IsConnected() check. > > > > > > Makes sense. Done.
On 2015/01/22 11:27:28, Mattias Nissler wrote: > On 2015/01/21 15:58:09, bartfab wrote: > > On 2015/01/21 15:56:38, Mattias Nissler wrote: > > > On 2015/01/21 15:49:38, bartfab wrote: > > > > On 2015/01/21 14:59:58, Mattias Nissler wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... > > > > > File > > > > chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/828953004/diff/100001/chrome/browser/chromeos... > > > > > > > > > > > > > > > chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:247: > > > > > if (!invalidation_service_) > > > > > On 2015/01/21 14:45:23, bartfab wrote: > > > > > > On 2015/01/21 14:19:26, Mattias Nissler wrote: > > > > > > > On 2015/01/21 13:38:48, bartfab wrote: > > > > > > > > On 2015/01/21 12:57:28, Mattias Nissler wrote: > > > > > > > > > Still doing some work here after a call to the observer returns, > > > which > > > > > > could > > > > > > > > > have changed internal state. Maybe it's better (and more > > readable?) > > > to > > > > > > have > > > > > > > > > FindConnectedInvalidationService return the service it picked > and > > > then > > > > > > fire > > > > > > > > the > > > > > > > > > update here? > > > > > > > > > > > > > > > > I had considered this. However, there is one code path in which > > > > > > > > FindConnectedInvalidationService() does not explicitly set the > > > > > > > > |invalidation_service_| itself: > > > > > > > > > > > > > > > > If there is no |device_invalidation_service_| yet, > > > > > > > > FindConnectedInvalidationService() creates one. If this service > > > reports > > > > > that > > > > > > > it > > > > > > > > is connected immediately (unlikely as a client-server handshake > > needs > > > to > > > > > > > happen > > > > > > > > first but I need to be compatible with all possible future > > > > > implementations, > > > > > > > > including e.g. one that just latches onto a cached connection > > > > > immediately), > > > > > > > its > > > > > > > > observer will trigger SetInvalidationService(). > > > > > > > > > > > > > > > > Thus, at the end of FindConnectedInvalidationService(), there are > > > three > > > > > > > options: > > > > > > > > 1) SetInvalidationService() was called by > > > > > FindConnectedInvalidationService() > > > > > > > > explicitly. > > > > > > > > 2) SetInvalidationService() was called by other code that > > > > > > > > FindConnectedInvalidationService() triggered. > > > > > > > > 3) SetInvalidationService() was never called. > > |invalidation_service_| > > > is > > > > a > > > > > > > > nullptr. > > > > > > > > > > > > > > > > FindConnectedInvalidationService() can report (1) reliably, but it > > > > cannot > > > > > > > > distinguish between (2) and (3). Thus, you have to look at > > > > > > > > |invalidation_service_| in some cases. I could move this into > > > > > > > > FindConnectedInvalidationService() and only look at > > > > > |invalidation_service_| > > > > > > in > > > > > > > > the code path that needs to distinguish between (2) and (3). But I > > > find > > > > it > > > > > > > > cleaner, simpler and less error-prone to unconditionally look at > > > > > > > > |invalidation_service_| here instead, allowing us to always > > > distinguish > > > > > > > between > > > > > > > > (1)/(2) and (3). > > > > > > > > > > > > > > A simple way to get to the point where SetInvalidationService > doesn't > > > get > > > > > > called > > > > > > > for cases #2 and #3 is to remove the IsServiceConnected check in the > > > > > > > InvalidationServiceObserver ctor and perform the IsServiceConnected > > > check > > > > in > > > > > > the > > > > > > > caller. That makes the block starting in lines 289 go away, and a > > check > > > > > would > > > > > > > have to be added in line 166 (I think you had that initially). Do > you > > > > think > > > > > > > that'd be clearer? > > > > > > > > > > > > *Argh*, Rietveld ate my homework and deleted the entire comment I had > > > typed > > > > > :(. > > > > > > I will start again: > > > > > > > > > > > > I had moved IsServiceConnected() to the observer constructor at your > > > > explicit > > > > > > request :), see: > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/828953004/diff2/20001:40001/chrome/browser/ch... > > > > > > > > > > > > While thinking about this, I noticed another corner case in the > observer > > > > > > constructor that the newest patch set fixes: > > > > > > > > > > > > 1) The invalidaiton service is not connected at first. > > > > > > 2) The service connects while the observer is registering as an > observer > > > > (line > > > > > > 75), calling OnInvalidatorStateChange(), which triggers > > > > > > OnInvalidationServiceConnected() > > > > > > 3) The observer notices that the service is connected (line 76), > > > triggering > > > > > > OnInvalidationServiceConnected(). > > > > > > > > > > > > We now have two calls to OnInvalidationServiceConnected(), both of > which > > > > call > > > > > > SetInvalidationService(). The second call runs into > > > > > > DCHECK(!invalidation_service_); (line 299). > > > > > > > > > > > > If we move the IsServiceConnected() check out of the observer > > constructor, > > > > we > > > > > > will still have to check somehow whether the service happens to have > > > > triggered > > > > > > an SetInvalidationService() before we got to the IsServiceConnected() > > > check. > > > > > The > > > > > > only way to do this that I can see is to look at > |invalidation_service_| > > > > once > > > > > > again. So nothing is gained. > > > > > > > > > > The problem you've found with RegisterInvalidationHandler() triggering > > > > > re-entrancy is even worse - what if the profile and with it the > profile's > > > > > invalidation service go away in the mean time? > > > > > > > > > > Tangential question: Are you handling the case of profiles going away > > > properly > > > > > at all? I can't find a place where observers are removed fro > > > > > |profile_invalidation_service_observers_|. > > > > > > > > AFAIK, we only destroy Profiles during shutdown, after the > > > > AffiliatedInvalidationServiceProvider has been destroyed. Thus, there > should > > > be > > > > no need to handle Profiles going away. There is lots of code in Chrome > that > > > > would go up in flames if Profiles just went away mid-flight. > > > > > > Can you please double-check whether that assumption holds for secondary > > profiles > > > in multi-profile sessions? At least for desktop Chrome, Profiles do > definitely > > > get destroyed (this was one motivation to do the profile-keyed-service stuff > > > IIRC). > > > > Yes, it definitely holds for Chrome OS multi-profile sessions. You can add > more > > users to a session but you cannot remove them. Signing out restarts the > browser > > and signs you out of all profiles. > > OK, so after this comment I was wondering why you don't crash and burn when the > Profiles get destroyed and the ProfileInvalidationService instances send > disconnects in arbitrary order, forcing attempts to locate alternate connected > ProfileInvalidationService instances and hitting objects that have already been > deleted. I now realized that you conveniently sidestep this problem by creative > service lifetime management in BrowserPolicyConnectorChromeOS::PreShutdown(): > AffiliatedInvalidationServiceProvider must be created *before* any Profile comes > up, but needs to be destroyed *before* any Profile gets destroyed again. Eeeek. > > That'll also mean that any consumers of AffiliatedInvalidationServiceProvider > are now forced to use early shutdown or destruction, which only proliferates the > lifetime nesting violation. I'm not comfortable with this design TBH. The proper > and more intuitive way is to create AffiliatedInvalidationServiceProvider > *before* all Profiles and consumers and then destroy it *after* all Profiles and > consumers are gone. I can change the code of course but this is a common pattern in Chrome OS. Have a look at ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun(). All the major subsystems are shut down before Profiles go away, including essentials like the UserManager and DeviceOAuth2TokenServiceFactory. I could make the AffiliatedInvalidationServiceProvider shut down later but realistically, anyone using Chrome OS subsystems will have other dependencies that force them to shut down early anyway. So we add code complexity with associated potential bugs for no real gain. > > > > > > > > > > > > > > > > > > > > Anyhow, an additional problem will be that while you call > > > > > RegisterInvalidationHandler(), the OnValidationServiceConnected() > callback > > > > will > > > > > fire at a time where the newly-created observer isn't present in > > > > > profile_invalidation_service_observers_, which can only lead to trouble. > > > Maybe > > > > > it's best to ignore callbacks until you've wired everything up > correctly? > > > Once > > > > > you arm the observer, you would then also perform the IsConnected() > check. > > > > > > > > Makes sense. Done.
mnissler@chromium.org changed reviewers: + nkostylev@chromium.org
[Dropping history as we're hitting rietveld limits] Nikita, looping you in here for a cros multi-profile question: Is it OK for new code that gets written to assume that all Profiles will stay alive until the session gets terminated? That's how things are right now, but I'm unsure what the plans are. On 2015/01/22 12:35:35, bartfab wrote: > I can change the code of course but this is a common pattern in Chrome OS. Have > a look at ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun(). All the > major subsystems are shut down before Profiles go away, including essentials > like the UserManager and DeviceOAuth2TokenServiceFactory. I could make the > AffiliatedInvalidationServiceProvider shut down later but realistically, anyone > using Chrome OS subsystems will have other dependencies that force them to shut > down early anyway. So we add code complexity with associated potential bugs for > no real gain. Note that ChromeBrowserMainPartsChromeOS::PostMainMessageLoopRun() sends shutdown notifications to other components, it doesn't destroy the instances. That way, life time management is still properly nested, and downstream dependencies can continue to make the usual life time nesting assumptions. I think it's mostly fine to have Shutdown() invoked in general as part of PostMainMessageLoopRun(), but the object should life on and not blow up on access.
On 2015/01/26 10:44:20, Mattias Nissler wrote: > [Dropping history as we're hitting rietveld limits] > > Nikita, looping you in here for a cros multi-profile question: Is it OK for new > code that gets written to assume that all Profiles will stay alive until the > session gets terminated? That's how things are right now, but I'm unsure what > the plans are. Yes, we should always assume that. > > On 2015/01/22 12:35:35, bartfab wrote: > > I can change the code of course but this is a common pattern in Chrome OS. > Have > > a look at ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun(). All the > > major subsystems are shut down before Profiles go away, including essentials > > like the UserManager and DeviceOAuth2TokenServiceFactory. I could make the > > AffiliatedInvalidationServiceProvider shut down later but realistically, > anyone > > using Chrome OS subsystems will have other dependencies that force them to > shut > > down early anyway. So we add code complexity with associated potential bugs > for > > no real gain. > > Note that ChromeBrowserMainPartsChromeOS::PostMainMessageLoopRun() sends > shutdown notifications to other components, it doesn't destroy the instances. > That way, life time management is still properly nested, and downstream > dependencies can continue to make the usual life time nesting assumptions. I > think it's mostly fine to have Shutdown() invoked in general as part of > PostMainMessageLoopRun(), but the object should life on and not blow up on > access.
Hi Mattias, Please take another look.
This LGTM now after you've verified that the DCHECK is correct (or removed it if it's not). https://codereview.chromium.org/828953004/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc (right): https://codereview.chromium.org/828953004/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:142: DCHECK(is_shut_down_); I'm not sure this DCHECK is a good idea. What if the browser startup logic decides to bail and terminate immediately after creating AffiliatedInvalidationServiceProvider?
https://codereview.chromium.org/828953004/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc (right): https://codereview.chromium.org/828953004/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.cc:142: DCHECK(is_shut_down_); On 2015/01/28 09:02:23, Mattias Nissler wrote: > I'm not sure this DCHECK is a good idea. What if the browser startup logic > decides to bail and terminate immediately after creating > AffiliatedInvalidationServiceProvider? The provider is created when BrowserPolicyConnectorChromeOS::Init() is called by BrowserProcessImpl::PreMainMessageLoopRun(). The provider is shut down when BrowserPolicyConnectorChromeOS:PreShutdown() is called by ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun(). If PreMainMessageLoopRun() ran, PostMainMessageLoopRun() will run as well, no matter how early we bail out.
The CQ bit was checked by bartfab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/828953004/180001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by bartfab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/828953004/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/465d8a5a1709bce5780c697407f7a7c4146dfea8 Cr-Commit-Position: refs/heads/master@{#313488} |
