[GCM] Make sure GCM checkout logic is invoked when the profile is signed out

[GCM] Make sure GCM checkout logic is invoked when the profile is signed out

We now always create GCMProfileService that listens to profile
notifications such that GCM checkout logic could be invoked when the
profile is signed out.

We now move the GCM check-in logic from GCMClient::Initialize to GCMClient::CheckIn.

BUG=344031
TEST=new tests added and existing tests updated

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

[jianli, 2014-02-20 20:52:11]

zea and fgorski, please review all.

asvitkine, please review the new histogram being added.

[fgorski, 2014-02-20 22:18:40]

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_client_mock.cc (right):

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_client_mock.cc:38:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
Do you want to add a DCHECK that GCMClientMock is not checkin in yet?

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_client_mock.cc:91: checkin_delay_ =
NO_CHECKIN_DELAY;
do you need to flip the flag here? why do you do it?

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.cc:638: EnsureCheckedIn();
Seems like calling send or receive when will check the user in regardless of the
IsGCMChannelEnabled().

Could you make that check inside of EnsureCheckedIn?

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (right):

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:604:
TEST_F(GCMProfileServiceTest, SignInAndSignOutUnderNegativeChannelSignal) {
Make sure to add a test where you invoke send or register and the pref is set to
false.

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:605: // Negative
channel signal will cause GCMClient not checked in when the
will prevent GCMClient from checking in when

https://codereview.chromium.org/165993005/diff/90001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/165993005/diff/90001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:161: }
Add a INITIALIZED to the State enum in the header and set it at the end of the
method.

https://codereview.chromium.org/165993005/diff/90001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:164: void GCMClientImpl::CheckIn() {
Add a DCHECK_EQ(state_, INITIALIZED);

[jianli, 2014-02-21 18:25:10]

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_client_mock.cc (right):

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_client_mock.cc:38:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
On 2014/02/20 22:18:40, Filip Gorski wrote:
> Do you want to add a DCHECK that GCMClientMock is not checkin in yet?

Done.

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_client_mock.cc:91: checkin_delay_ =
NO_CHECKIN_DELAY;
On 2014/02/20 22:18:40, Filip Gorski wrote:
> do you need to flip the flag here? why do you do it?

We flip the flag such that GCMClientMock::CheckIn can be called to bypass
checkin_delay_ == CHECKIN_DELAY check.

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.cc:638: EnsureCheckedIn();
On 2014/02/20 22:18:40, Filip Gorski wrote:
> Seems like calling send or receive when will check the user in regardless of
the
> IsGCMChannelEnabled().
> 
> Could you make that check inside of EnsureCheckedIn?

When a gcm app is running and register/send is called, we want to initiate GCM
regardless IsGCMChannelEnabled. Otherwise, GCM app will not be running.

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (right):

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:604:
TEST_F(GCMProfileServiceTest, SignInAndSignOutUnderNegativeChannelSignal) {
On 2014/02/20 22:18:40, Filip Gorski wrote:
> Make sure to add a test where you invoke send or register and the pref is set
to
> false.

Done.

https://codereview.chromium.org/165993005/diff/90001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:605: // Negative
channel signal will cause GCMClient not checked in when the
On 2014/02/20 22:18:40, Filip Gorski wrote:
> will prevent GCMClient from checking in when

Done.

https://codereview.chromium.org/165993005/diff/90001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/165993005/diff/90001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:161: }
On 2014/02/20 22:18:40, Filip Gorski wrote:
> Add a INITIALIZED to the State enum in the header and set it at the end of the
> method.

Done.

https://codereview.chromium.org/165993005/diff/90001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:164: void GCMClientImpl::CheckIn() {
On 2014/02/20 22:18:40, Filip Gorski wrote:
> Add a DCHECK_EQ(state_, INITIALIZED);

Done.

[Nicolas Zea, 2014-02-21 18:43:32]

https://codereview.chromium.org/165993005/diff/260001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/165993005/diff/260001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:529: // Create and initialize
the GCMClient. Note that this does not initiate the
Initializing the client will result in connecting to MCS if checkin credentials
were already there. This means the kill switch won't have an effect anymore.

I think we can't perform initialization unless GCMChannel enabled returns true
or it's unset and an app attempts to perform an action.

https://codereview.chromium.org/165993005/diff/260001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/165993005/diff/260001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.h:122: // Returns true if GCM is
enabled for the rollout channel.
nit: mention that this controls whether the GCMPS will initialize not or not.

[Alexei Svitkine (slow), 2014-02-21 18:55:40]

https://codereview.chromium.org/165993005/diff/260001/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/165993005/diff/260001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client_impl.cc:381: LOG_IF(ERROR, !success) << "GCM store
failed to be destroyed!";
I think non-debug build log messages are discouraged in Chromium. There was a
recent chromium-dev thread about this.

Can this be a DLOG_IF?

https://codereview.chromium.org/165993005/diff/260001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/165993005/diff/260001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5594: +    indicates a failure
destroying the persistence store.
Can you expand the description to give more context about when the GCM
persistent store get destroyed?

[Nicolas Zea, 2014-02-21 18:58:59]

https://codereview.chromium.org/165993005/diff/260001/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/165993005/diff/260001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client.h:135: // Begins initialization of the GCM Client.
mention that this will not trigger a connection yet

https://codereview.chromium.org/165993005/diff/260001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client.h:151: virtual void CheckIn() = 0;
Does it make more sense to call this Load() now? And mention that we'll
automatically check in if no store is found?

Although I wonder, should loading be part of initialize now, and we should just
have a separate Connect method (this would handle dealing with apps that
uninstall while we're suppressed).

I find it confusing that CheckIn will always load, but will only sometimes
actually perform a check-in. Especially since we'll have actual checkins
separate from load (the periodic ones).

[jianli, 2014-02-21 20:24:06]

https://codereview.chromium.org/165993005/diff/260001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/165993005/diff/260001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:529: // Create and initialize
the GCMClient. Note that this does not initiate the
On 2014/02/21 18:43:33, Nicolas Zea wrote:
> Initializing the client will result in connecting to MCS if checkin
credentials
> were already there. This means the kill switch won't have an effect anymore.
> 
> I think we can't perform initialization unless GCMChannel enabled returns true
> or it's unset and an app attempts to perform an action.

 I have moved the GCMStore loading logic out of GCMClientImpl::Initialize. Now
when GCMClient is initialized, we only create GCMStore and MCSClient. We do not
trigger store loading at this time.

https://codereview.chromium.org/165993005/diff/260001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/165993005/diff/260001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.h:122: // Returns true if GCM is
enabled for the rollout channel.
On 2014/02/21 18:43:33, Nicolas Zea wrote:
> nit: mention that this controls whether the GCMPS will initialize not or not.

Done.

https://codereview.chromium.org/165993005/diff/260001/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/165993005/diff/260001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client.h:135: // Begins initialization of the GCM Client.
On 2014/02/21 18:58:59, Nicolas Zea wrote:
> mention that this will not trigger a connection yet

Done.

https://codereview.chromium.org/165993005/diff/260001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client.h:151: virtual void CheckIn() = 0;
On 2014/02/21 18:58:59, Nicolas Zea wrote:
> Does it make more sense to call this Load() now? And mention that we'll
> automatically check in if no store is found?

Renamed to Load() added comment.
> 
> Although I wonder, should loading be part of initialize now, and we should
just
> have a separate Connect method (this would handle dealing with apps that
> uninstall while we're suppressed).

If the channel signal is off, we do not really need to load from the store.

We do need to refactor this to make the whole logic clean, but I would like to
do it later since we probably need to merge this patch to M34 branch.

> 
> I find it confusing that CheckIn will always load, but will only sometimes
> actually perform a check-in. Especially since we'll have actual checkins
> separate from load (the periodic ones).

https://codereview.chromium.org/165993005/diff/260001/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/165993005/diff/260001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client_impl.cc:381: LOG_IF(ERROR, !success) << "GCM store
failed to be destroyed!";
On 2014/02/21 18:55:40, Alexei Svitkine wrote:
> I think non-debug build log messages are discouraged in Chromium. There was a
> recent chromium-dev thread about this.
> 
> Can this be a DLOG_IF?

Done.

https://codereview.chromium.org/165993005/diff/260001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/165993005/diff/260001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5594: +    indicates a failure
destroying the persistence store.
On 2014/02/21 18:55:40, Alexei Svitkine wrote:
> Can you expand the description to give more context about when the GCM
> persistent store get destroyed?

Done.

[Alexei Svitkine (slow), 2014-02-21 20:41:41]

histograms lgtm

[Nicolas Zea, 2014-02-21 20:58:58]

mainly nits

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/extensio...
File chrome/browser/extensions/api/gcm/gcm_apitest.cc (right):

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/extensio...
chrome/browser/extensions/api/gcm/gcm_apitest.cc:47: // GCMProfileService
instance as in SetUpOnMainThread. To work around this
nit: fix comment. Not sure what "settings a faked GCMProfileService instance as
in" means

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_client_mock.h (right):

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
chrome/browser/services/gcm/gcm_client_mock.h:23: LOADING_DELAY,
nit: loading_delay -> delay_load (make it an action)

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (left):

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:424: gcm_client_.reset();
why was this removed?

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:263: void CheckIn();
this should be named Load as well

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:750: // Initiate the GCM
check-in.
nit: the GCM load

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.h:123: // GCMProfileService
should make GCMClient checked in or not.
nit: "should make GCMClient checked in or not" ->"should load and connect the
GCMClient or not"

[fgorski, 2014-02-21 22:05:06]

lgtm mod comment

https://codereview.chromium.org/165993005/diff/390001/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/165993005/diff/390001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client_impl_unittest.cc:287: gcm_client_->CheckIn();
Load as well

[jianli, 2014-02-22 00:03:05]

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/extensio...
File chrome/browser/extensions/api/gcm/gcm_apitest.cc (right):

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/extensio...
chrome/browser/extensions/api/gcm/gcm_apitest.cc:47: // GCMProfileService
instance as in SetUpOnMainThread. To work around this
On 2014/02/21 20:58:58, Nicolas Zea wrote:
> nit: fix comment. Not sure what "settings a faked GCMProfileService instance
as
> in" means

Done.

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_client_mock.h (right):

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
chrome/browser/services/gcm/gcm_client_mock.h:23: LOADING_DELAY,
On 2014/02/21 20:58:58, Nicolas Zea wrote:
> nit: loading_delay -> delay_load (make it an action)

Done.

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (left):

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:424: gcm_client_.reset();
On 2014/02/21 20:58:58, Nicolas Zea wrote:
> why was this removed?

We need to keep GCMClient instance alive in case that the profile has been
signed in again. Currently we do not recreate GCMClient.

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:263: void CheckIn();
On 2014/02/21 20:58:58, Nicolas Zea wrote:
> this should be named Load as well

Done.

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:750: // Initiate the GCM
check-in.
On 2014/02/21 20:58:58, Nicolas Zea wrote:
> nit: the GCM load

Done.

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/165993005/diff/390001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.h:123: // GCMProfileService
should make GCMClient checked in or not.
On 2014/02/21 20:58:58, Nicolas Zea wrote:
> nit: "should make GCMClient checked in or not" ->"should load and connect the
> GCMClient or not"

This is removed since I decided to combine this and IsGCMEnabled to a new method
GetGCMEnabledState. Comments are added there.

https://codereview.chromium.org/165993005/diff/390001/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/165993005/diff/390001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client_impl_unittest.cc:287: gcm_client_->CheckIn();
On 2014/02/21 22:05:06, Filip Gorski wrote:
> Load as well

Done.

[Nicolas Zea, 2014-02-22 00:20:46]

lgtm

https://codereview.chromium.org/165993005/diff/410003/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/165993005/diff/410003/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:434: // might have been
signed in again.
nit: might have been -> might be

https://codereview.chromium.org/165993005/diff/410003/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (right):

https://codereview.chromium.org/165993005/diff/410003/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:627: // GCMClient
should not be loaded.
update comment

[Nicolas Zea, 2014-02-26 02:26:36]

Mostly still LG

https://codereview.chromium.org/165993005/diff/890001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (right):

https://codereview.chromium.org/165993005/diff/890001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:101:
content::BrowserThread::PostTask(
Doesn't RunUntilIdle accomplish the same thing?

https://codereview.chromium.org/165993005/diff/970001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/165993005/diff/970001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:239: explicit IOWorker(const
base::WeakPtr<GCMProfileService>& service);
is |service| here still necessary?

[fgorski, 2014-02-26 18:36:30]

still lg.

https://codereview.chromium.org/165993005/diff/970001/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/165993005/diff/970001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client_impl_unittest.cc:288: gcm_client_->Load();
move after the encryptor update

[jianli, 2014-02-26 19:04:24]

https://codereview.chromium.org/165993005/diff/890001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (right):

https://codereview.chromium.org/165993005/diff/890001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:101:
content::BrowserThread::PostTask(
On 2014/02/26 02:26:37, Nicolas Zea wrote:
> Doesn't RunUntilIdle accomplish the same thing?

No. It seems that occasionally some posted tasks are not seen by RunUntilIdle. I
have not figured out why.

Changing to this will make sure all IO tasks posted before this point get
executed. This is indeed what we want.

https://codereview.chromium.org/165993005/diff/970001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/165993005/diff/970001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:239: explicit IOWorker(const
base::WeakPtr<GCMProfileService>& service);
On 2014/02/26 02:26:37, Nicolas Zea wrote:
> is |service| here still necessary?

Yes, it is not needed. Removed.

https://codereview.chromium.org/165993005/diff/970001/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/165993005/diff/970001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client_impl_unittest.cc:288: gcm_client_->Load();
On 2014/02/26 18:36:31, Filip Gorski wrote:
> move after the encryptor update

Done.

[Nicolas Zea, 2014-02-26 20:34:49]

lgtm

[jianli, 2014-02-27 02:09:13]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-02-27 02:15:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/165993005/1000001

[commit-bot: I haz the power, 2014-02-27 13:46:56]

Change committed as 253787

[SeRya, 2014-03-04 08:35:25]

https://codereview.chromium.org/165993005/diff/1000001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/165993005/diff/1000001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service.cc:544:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter =
I tried to use ProfileSyncService on startup and got the following failure:

[10007:10007:0304/122239:FATAL:profile_impl.cc(991)] Check failed:
ssl_config_service_manager_. SSLConfigServiceManager is not initialized yet
 [0x7f0417dda49e] base::debug::StackTrace::StackTrace()
 [0x7f0417e320a5] logging::LogMessage::~LogMessage()
 [0x7f041bc2ae94] ProfileImpl::GetSSLConfigService()
 [0x7f041b6c452e] ProfileIOData::InitializeOnUIThread()
 [0x7f041bc300dc] ProfileImplIOData::Handle::LazyInitialize()
 [0x7f041bc2fefd] ProfileImplIOData::Handle::GetResourceContext()
 [0x7f041bc2acfc] ProfileImpl::GetResourceContext()
 [0x7f041d1da911] content::StoragePartitionImplMap::Get()
 [0x7f041cde3ccf] content::(anonymous
namespace)::GetStoragePartitionFromConfig()
 [0x7f041cde3c3a] content::BrowserContext::GetStoragePartition()
 [0x7f041cde3e1f] content::BrowserContext::GetDefaultStoragePartition()
 [0x7f041bc2a945] ProfileImpl::GetRequestContext()
 [0x7f041c24c375] gcm::GCMProfileService::Initialize()
 [0x7f041bc76236] gcm::GCMProfileServiceFactory::BuildServiceInstanceFor()
 [0x7f041e33dd45]
BrowserContextKeyedServiceFactory::GetServiceForBrowserContext()
 [0x7f041bc760c7] gcm::GCMProfileServiceFactory::GetForProfile()
 [0x7f041b7efd21] ProfileSyncServiceFactory::BuildServiceInstanceFor()
 [0x7f041e33dd45]
BrowserContextKeyedServiceFactory::GetServiceForBrowserContext()
 [0x7f041b7efa8f] ProfileSyncServiceFactory::GetForProfile()

There is a comment at the DCHEK:

  // If ssl_config_service_manager_ is null, this typically means that some
  // BrowserContextKeyedService is trying to create a RequestContext at startup,
  // but SSLConfigServiceManager is not initialized until DoFinalInit() which is
  // invoked after all BrowserContextKeyedServices have been initialized (see
  // http://crbug.com/171406).

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...

Doesn't it make sense to avoid using profile_->GetRequestContext() in service
initialization?

https://codereview.chromium.org/165993005/diff/1000001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/165993005/diff/1000001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_factory.cc:101:
gcm::GCMProfileServiceFactory::GetForProfile(profile);
Shouldn't it be paired with
DependsOn(gcm::GCMProfileServiceFactory::GetInstance())?

[jianli, 2014-03-05 01:37:33]

https://codereview.chromium.org/165993005/diff/1000001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/165993005/diff/1000001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service.cc:544:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter =
On 2014/03/04 08:35:26, SeRya wrote:
> I tried to use ProfileSyncService on startup and got the following failure:
> 
> [10007:10007:0304/122239:FATAL:profile_impl.cc(991)] Check failed:
> ssl_config_service_manager_. SSLConfigServiceManager is not initialized yet
>  [0x7f0417dda49e] base::debug::StackTrace::StackTrace()
>  [0x7f0417e320a5] logging::LogMessage::~LogMessage()
>  [0x7f041bc2ae94] ProfileImpl::GetSSLConfigService()
>  [0x7f041b6c452e] ProfileIOData::InitializeOnUIThread()
>  [0x7f041bc300dc] ProfileImplIOData::Handle::LazyInitialize()
>  [0x7f041bc2fefd] ProfileImplIOData::Handle::GetResourceContext()
>  [0x7f041bc2acfc] ProfileImpl::GetResourceContext()
>  [0x7f041d1da911] content::StoragePartitionImplMap::Get()
>  [0x7f041cde3ccf] content::(anonymous
> namespace)::GetStoragePartitionFromConfig()
>  [0x7f041cde3c3a] content::BrowserContext::GetStoragePartition()
>  [0x7f041cde3e1f] content::BrowserContext::GetDefaultStoragePartition()
>  [0x7f041bc2a945] ProfileImpl::GetRequestContext()
>  [0x7f041c24c375] gcm::GCMProfileService::Initialize()
>  [0x7f041bc76236] gcm::GCMProfileServiceFactory::BuildServiceInstanceFor()
>  [0x7f041e33dd45]
> BrowserContextKeyedServiceFactory::GetServiceForBrowserContext()
>  [0x7f041bc760c7] gcm::GCMProfileServiceFactory::GetForProfile()
>  [0x7f041b7efd21] ProfileSyncServiceFactory::BuildServiceInstanceFor()
>  [0x7f041e33dd45]
> BrowserContextKeyedServiceFactory::GetServiceForBrowserContext()
>  [0x7f041b7efa8f] ProfileSyncServiceFactory::GetForProfile()
> 
> There is a comment at the DCHEK:
> 
>   // If ssl_config_service_manager_ is null, this typically means that some
>   // BrowserContextKeyedService is trying to create a RequestContext at
startup,
>   // but SSLConfigServiceManager is not initialized until DoFinalInit() which
is
>   // invoked after all BrowserContextKeyedServices have been initialized (see
>   // http://crbug.com/171406).
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
> 
> Doesn't it make sense to avoid using profile_->GetRequestContext() in service
> initialization?

I can't repro this. Could you please share how you get this? OS? Version?

[SeRya, 2014-03-05 05:45:52]

On 2014/03/05 01:37:33, jianli wrote:
>
https://codereview.chromium.org/165993005/diff/1000001/chrome/browser/service...
> File chrome/browser/services/gcm/gcm_profile_service.cc (right):
> 
>
https://codereview.chromium.org/165993005/diff/1000001/chrome/browser/service...
> chrome/browser/services/gcm/gcm_profile_service.cc:544:
> scoped_refptr<net::URLRequestContextGetter> url_request_context_getter =
> On 2014/03/04 08:35:26, SeRya wrote:
> > I tried to use ProfileSyncService on startup and got the following failure:
> > 
> > [10007:10007:0304/122239:FATAL:profile_impl.cc(991)] Check failed:
> > ssl_config_service_manager_. SSLConfigServiceManager is not initialized yet
> >  [0x7f0417dda49e] base::debug::StackTrace::StackTrace()
> >  [0x7f0417e320a5] logging::LogMessage::~LogMessage()
> >  [0x7f041bc2ae94] ProfileImpl::GetSSLConfigService()
> >  [0x7f041b6c452e] ProfileIOData::InitializeOnUIThread()
> >  [0x7f041bc300dc] ProfileImplIOData::Handle::LazyInitialize()
> >  [0x7f041bc2fefd] ProfileImplIOData::Handle::GetResourceContext()
> >  [0x7f041bc2acfc] ProfileImpl::GetResourceContext()
> >  [0x7f041d1da911] content::StoragePartitionImplMap::Get()
> >  [0x7f041cde3ccf] content::(anonymous
> > namespace)::GetStoragePartitionFromConfig()
> >  [0x7f041cde3c3a] content::BrowserContext::GetStoragePartition()
> >  [0x7f041cde3e1f] content::BrowserContext::GetDefaultStoragePartition()
> >  [0x7f041bc2a945] ProfileImpl::GetRequestContext()
> >  [0x7f041c24c375] gcm::GCMProfileService::Initialize()
> >  [0x7f041bc76236] gcm::GCMProfileServiceFactory::BuildServiceInstanceFor()
> >  [0x7f041e33dd45]
> > BrowserContextKeyedServiceFactory::GetServiceForBrowserContext()
> >  [0x7f041bc760c7] gcm::GCMProfileServiceFactory::GetForProfile()
> >  [0x7f041b7efd21] ProfileSyncServiceFactory::BuildServiceInstanceFor()
> >  [0x7f041e33dd45]
> > BrowserContextKeyedServiceFactory::GetServiceForBrowserContext()
> >  [0x7f041b7efa8f] ProfileSyncServiceFactory::GetForProfile()
> > 
> > There is a comment at the DCHEK:
> > 
> >   // If ssl_config_service_manager_ is null, this typically means that some
> >   // BrowserContextKeyedService is trying to create a RequestContext at
> startup,
> >   // but SSLConfigServiceManager is not initialized until DoFinalInit()
which
> is
> >   // invoked after all BrowserContextKeyedServices have been initialized
(see
> >   // http://crbug.com/171406).
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
> > 
> > Doesn't it make sense to avoid using profile_->GetRequestContext() in
service
> > initialization?
> 
> I can't repro this. Could you please share how you get this? OS? Version?

My reduced code is like this:

class MyServiceFactory : public BrowserContextKeyedServiceFactory {
 public:
  static MyServiceFactory* GetInstance();

  virtual bool ServiceIsCreatedWithBrowserContext() const { return true; /*
Create on startup */ }

  virtual BrowserContextKeyedService* BuildServiceInstanceFor(
      content::BrowserContext* context) const {
    Profile* const profile = static_cast<Profile*>(context);
    ProfileSyncServiceFactory::GetForProfile(profile);
    ...
  }
}

chrome_browser_main_extra_parts_profiles.cc:

MyServiceFactory::GetInstance();

Linux64, Debug

[jianli, 2014-03-06 07:29:40]

On Tue, Mar 4, 2014 at 9:45 PM, <serya@chromium.org> wrote:

> On 2014/03/05 01:37:33, jianli wrote:
>
> https://codereview.chromium.org/165993005/diff/1000001/
> chrome/browser/services/gcm/gcm_profile_service.cc
>
>> File chrome/browser/services/gcm/gcm_profile_service.cc (right):
>>
>
>
> https://codereview.chromium.org/165993005/diff/1000001/
> chrome/browser/services/gcm/gcm_profile_service.cc#newcode544
>
>> chrome/browser/services/gcm/gcm_profile_service.cc:544:
>> scoped_refptr<net::URLRequestContextGetter> url_request_context_getter =
>> On 2014/03/04 08:35:26, SeRya wrote:
>> > I tried to use ProfileSyncService on startup and got the following
>> failure:
>> >
>> > [10007:10007:0304/122239:FATAL:profile_impl.cc(991)] Check failed:
>> > ssl_config_service_manager_. SSLConfigServiceManager is not initialized
>> yet
>> >  [0x7f0417dda49e] base::debug::StackTrace::StackTrace()
>> >  [0x7f0417e320a5] logging::LogMessage::~LogMessage()
>> >  [0x7f041bc2ae94] ProfileImpl::GetSSLConfigService()
>> >  [0x7f041b6c452e] ProfileIOData::InitializeOnUIThread()
>> >  [0x7f041bc300dc] ProfileImplIOData::Handle::LazyInitialize()
>> >  [0x7f041bc2fefd] ProfileImplIOData::Handle::GetResourceContext()
>> >  [0x7f041bc2acfc] ProfileImpl::GetResourceContext()
>> >  [0x7f041d1da911] content::StoragePartitionImplMap::Get()
>> >  [0x7f041cde3ccf] content::(anonymous
>> > namespace)::GetStoragePartitionFromConfig()
>> >  [0x7f041cde3c3a] content::BrowserContext::GetStoragePartition()
>> >  [0x7f041cde3e1f] content::BrowserContext::GetDefaultStoragePartition()
>> >  [0x7f041bc2a945] ProfileImpl::GetRequestContext()
>> >  [0x7f041c24c375] gcm::GCMProfileService::Initialize()
>> >  [0x7f041bc76236] gcm::GCMProfileServiceFactory:
>> :BuildServiceInstanceFor()
>> >  [0x7f041e33dd45]
>> > BrowserContextKeyedServiceFactory::GetServiceForBrowserContext()
>> >  [0x7f041bc760c7] gcm::GCMProfileServiceFactory::GetForProfile()
>> >  [0x7f041b7efd21] ProfileSyncServiceFactory::BuildServiceInstanceFor()
>> >  [0x7f041e33dd45]
>> > BrowserContextKeyedServiceFactory::GetServiceForBrowserContext()
>> >  [0x7f041b7efa8f] ProfileSyncServiceFactory::GetForProfile()
>> >
>> > There is a comment at the DCHEK:
>> >
>> >   // If ssl_config_service_manager_ is null, this typically means that
>> some
>> >   // BrowserContextKeyedService is trying to create a RequestContext at
>> startup,
>> >   // but SSLConfigServiceManager is not initialized until DoFinalInit()
>>
> which
>
>> is
>> >   // invoked after all BrowserContextKeyedServices have been initialized
>>
> (see
>
>> >   // http://crbug.com/171406).
>> >
>> >
>>
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/chrome/browser/profiles/profile_impl.cc&l=986
>
>> >
>> > Doesn't it make sense to avoid using profile_->GetRequestContext() in
>>
> service
>
>> > initialization?
>>
>
>  I can't repro this. Could you please share how you get this? OS? Version?
>>
>
> My reduced code is like this:
>
> class MyServiceFactory : public BrowserContextKeyedServiceFactory {
>  public:
>   static MyServiceFactory* GetInstance();
>
>   virtual bool ServiceIsCreatedWithBrowserContext() const { return true;
> /*
> Create on startup */ }
>
>   virtual BrowserContextKeyedService* BuildServiceInstanceFor(
>       content::BrowserContext* context) const {
>     Profile* const profile = static_cast<Profile*>(context);
>     ProfileSyncServiceFactory::GetForProfile(profile);
>     ...
>   }
> }
>
> chrome_browser_main_extra_parts_profiles.cc:
>
> MyServiceFactory::GetInstance();
>
> Linux64, Debug
>

My guess is that MyServiceFactory is created a bit too early. When I have
time tomorrow, I will try to follow your code to repro it. Thanks.


> https://codereview.chromium.org/165993005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.