Move UserPolicySigninServiceBase to observe SigninManager.

Move UserPolicySigninServiceBase to observe SigninManager.

This is part of the effort to eliminate Signin-related notifications.

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

[Joao da Silva, 2014-03-05 17:09:54]

lgtm

+Drew: FYI

[blundell, 2014-03-05 17:12:03]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-05 17:13:55]

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

[commit-bot: I haz the power, 2014-03-05 17:16:48]

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

[commit-bot: I haz the power, 2014-03-05 17:16:48]

Try jobs failed on following builders: linux_chromium_rel, mac_chromium_rel

[blundell, 2014-03-05 20:17:16]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-05 20:23:04]

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

[commit-bot: I haz the power, 2014-03-05 20:38:25]

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

[commit-bot: I haz the power, 2014-03-05 20:38:26]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[blundell, 2014-03-05 21:47:57]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-05 21:51:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/187693002/20001

[commit-bot: I haz the power, 2014-03-05 22:42:20]

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

[commit-bot: I haz the power, 2014-03-05 22:42:21]

Try jobs failed on following builders: mac_chromium_compile_dbg

[blundell, 2014-03-06 06:11:22]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-06 06:12:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/187693002/20001

[commit-bot: I haz the power, 2014-03-06 07:32:05]

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

[commit-bot: I haz the power, 2014-03-06 07:32:06]

Try jobs failed on following builders: linux_chromium_rel

[blundell, 2014-03-06 12:09:09]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-06 12:09:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/187693002/40001

[commit-bot: I haz the power, 2014-03-06 12:22:21]

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

[commit-bot: I haz the power, 2014-03-06 12:22:22]

Try jobs failed on following builders: linux_chromium_rel

[blundell, 2014-03-07 08:05:51]

Joao,

Can you throw an eye over the issue I'm having? I'm guarding the call to
SigninManagerBase::RemoveObserver() by a check that the signin manager isn't
null, but unit tests on Linux seem to be crashing as if that check isn't there,
e.g.:

http://build.chromium.org/p/tryserver.chromium/builders/linux_gtk/builds/3194...

Is there something obvious I'm missing?

Thanks for your help,

Colin

[Joao da Silva, 2014-03-07 20:30:15]

On 2014/03/07 08:05:51, blundell wrote:
> Joao,
> 
> Can you throw an eye over the issue I'm having? I'm guarding the call to
> SigninManagerBase::RemoveObserver() by a check that the signin manager isn't
> null, but unit tests on Linux seem to be crashing as if that check isn't
there,
> e.g.:
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_gtk/builds/3194...
> 
> Is there something obvious I'm missing?
> 
> Thanks for your help,
> 
> Colin

It's a dead pointer. There's a deeper issue here that isn't obvious at first,
see https://codereview.chromium.org/189443008 for a fix. This CL can be landed
as-is after that fix goes in.

[blundell, 2014-03-10 10:28:59]

Joao,

Thanks so much for the investigation and uncovering the issue!

Now that I know the issue, I can easily make a fix in this CL: The UPSS can
observe the SigninManagerFactory, remove itself as an Observer when a
SigninManager is getting shut down, and add itself as an Observer when a
SigninManager is created. This will take care of the dead pointer problem
without requiring https://codereview.chromium.org/189443008/. How does that
sound?

[Joao da Silva, 2014-03-11 16:23:41]

On 2014/03/10 10:28:59, blundell wrote:
> Joao,
> 
> Thanks so much for the investigation and uncovering the issue!
> 
> Now that I know the issue, I can easily make a fix in this CL: The UPSS can
> observe the SigninManagerFactory, remove itself as an Observer when a
> SigninManager is getting shut down, and add itself as an Observer when a
> SigninManager is created. This will take care of the dead pointer problem
> without requiring https://codereview.chromium.org/189443008/. How does that
> sound?

I'm assuming that the UPSS would observe the SigninManagers of a specific
Profile only, and not of other Profile's.

That would work but seems like a hack around the underlying issue; PKSes of a
specific Profile are not meant to come and go at runtime. We should really get
the test setup fixed.

[blundell, 2014-03-11 16:24:54]

On 2014/03/11 16:23:41, Joao da Silva wrote:
> On 2014/03/10 10:28:59, blundell wrote:
> > Joao,
> > 
> > Thanks so much for the investigation and uncovering the issue!
> > 
> > Now that I know the issue, I can easily make a fix in this CL: The UPSS can
> > observe the SigninManagerFactory, remove itself as an Observer when a
> > SigninManager is getting shut down, and add itself as an Observer when a
> > SigninManager is created. This will take care of the dead pointer problem
> > without requiring https://codereview.chromium.org/189443008/. How does that
> > sound?
> 
> I'm assuming that the UPSS would observe the SigninManagers of a specific
> Profile only, and not of other Profile's.
> 
> That would work but seems like a hack around the underlying issue; PKSes of a
> specific Profile are not meant to come and go at runtime. We should really get
> the test setup fixed.

Yeah Drew's suggestion on the other CL seems perfect to me (and yes it would
filter by the Profile in the suggestion I was making).

[blundell, 2014-03-12 06:24:33]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-12 06:25:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/187693002/40001

[commit-bot: I haz the power, 2014-03-12 08:34:21]

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

[commit-bot: I haz the power, 2014-03-12 08:34:22]

Try jobs failed on following builders: mac_chromium_rel

[blundell, 2014-03-12 11:46:01]

Hi Joao,

So it turns out that there are a bunch of places in the codebase (~8) that
follow the pattern of creating a TestingProfile followed by setting a testing
factory for SigninManager on it. All of these places have the same problem. Do
you think it's better to change all of them to use the Builder or to do the
SigninManagerFactory::Observer approach? The advantage to the latter that I see
is that it doesn't leave this as a latent problem in the codebase. OTOH, maybe
Chromium should be deprecating direct usage of BCKS::SetTestingFactory in favor
of using a TestingProfileBuilder in order to head off problems like this one in
the first place.

[Joao da Silva, 2014-03-12 16:06:37]

On 2014/03/12 11:46:01, blundell wrote:
> Hi Joao,
> 
> So it turns out that there are a bunch of places in the codebase (~8) that
> follow the pattern of creating a TestingProfile followed by setting a testing
> factory for SigninManager on it. All of these places have the same problem. Do
> you think it's better to change all of them to use the Builder or to do the
> SigninManagerFactory::Observer approach? The advantage to the latter that I
see
> is that it doesn't leave this as a latent problem in the codebase. OTOH, maybe
> Chromium should be deprecating direct usage of BCKS::SetTestingFactory in
favor
> of using a TestingProfileBuilder in order to head off problems like this one
in
> the first place.

I think it's better to fix those 8 places and use a Builder.

The Builder was introduced to solve these sort of problems, and the production
code assumes that BCKSes for a Profile are "stable" and don't change at runtime;
I don't think we should make code more complicated to accommodate for this
(broken) testing pattern.

BCKS::SetTestingFactory is not a problem if the service hasn't been created yet.
I think it'd be a good idea to CHECK() at SetTestingFactory when the service has
already been created, and log a message hinting at TestingProfile::Builder as
the appropriate fix.

[blundell, 2014-03-12 16:26:51]

On 2014/03/12 16:06:37, Joao da Silva wrote:
> On 2014/03/12 11:46:01, blundell wrote:
> > Hi Joao,
> > 
> > So it turns out that there are a bunch of places in the codebase (~8) that
> > follow the pattern of creating a TestingProfile followed by setting a
testing
> > factory for SigninManager on it. All of these places have the same problem.
Do
> > you think it's better to change all of them to use the Builder or to do the
> > SigninManagerFactory::Observer approach? The advantage to the latter that I
> see
> > is that it doesn't leave this as a latent problem in the codebase. OTOH,
maybe
> > Chromium should be deprecating direct usage of BCKS::SetTestingFactory in
> favor
> > of using a TestingProfileBuilder in order to head off problems like this one
> in
> > the first place.
> 
> I think it's better to fix those 8 places and use a Builder.
> 
> The Builder was introduced to solve these sort of problems, and the production
> code assumes that BCKSes for a Profile are "stable" and don't change at
runtime;
> I don't think we should make code more complicated to accommodate for this
> (broken) testing pattern.
> 
> BCKS::SetTestingFactory is not a problem if the service hasn't been created
yet.
> I think it'd be a good idea to CHECK() at SetTestingFactory when the service
has
> already been created, and log a message hinting at TestingProfile::Builder as
> the appropriate fix.

The problem with "if the service hasn't been created yet" is that service
creation time can change over time. By the time that that CHECK() goes off there
might be 10+ unit tests that have to be fixed, which sucks. I'm starting to
regard direct usage of BCKS::SetTestingFactory() after the TestingProfile has
been created as a ticking time bomb.

The reason I suggest changing the code is not to accommodate the broken testing
pattern as such. It's the fact that the code as it is can go off like this again
whenever anyone creates a new test using this pattern, and it's difficult to
debug. A CHECK() would be good there, but I think it would break a lot of
existing code (there's a bunch of code in BCKSFactory to deal specifically with
the case where there's already a service present), and I don't personally have
the bandwidth to take on an effort like that :).

I'm fine changing the existing unittests (clearly a good thing in any case) and
landing the CL as-is, just wanted to give my reasoning.

[blundell, 2014-03-27 20:55:19]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-27 20:56:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/187693002/100001

[blundell, 2014-03-28 17:59:21]

The CQ bit was unchecked by blundell@chromium.org

[blundell, 2014-03-28 17:59:31]

The CQ bit was checked by blundell@chromium.org

[blundell, 2014-03-29 07:25:17]

The CQ bit was unchecked by blundell@chromium.org

[blundell, 2014-03-29 07:25:56]

The CQ bit was checked by blundell@chromium.org

[blundell, 2014-03-29 18:38:42]

The CQ bit was unchecked by blundell@chromium.org

[blundell, 2014-03-29 18:39:59]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-29 18:40:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/187693002/120001

[commit-bot: I haz the power, 2014-03-29 19:30:32]

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

[commit-bot: I haz the power, 2014-03-29 19:30:33]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[blundell, 2014-03-30 16:53:44]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-30 16:54:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/187693002/120001

[commit-bot: I haz the power, 2014-03-30 18:22:58]

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

[commit-bot: I haz the power, 2014-03-30 18:23:01]

Failed to apply patch for
chrome/browser/policy/cloud/user_policy_signin_service_base.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/policy/cloud/user_policy_signin_service_base.cc
  Hunk #1 FAILED at 9.
  Hunk #2 succeeded at 72 (offset 1 line).
  Hunk #3 succeeded at 133 (offset 1 line).
  Hunk #4 succeeded at 183 (offset 1 line).
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/policy/cloud/user_policy_signin_service_base.cc.rej

Patch:       chrome/browser/policy/cloud/user_policy_signin_service_base.cc
Index: chrome/browser/policy/cloud/user_policy_signin_service_base.cc
diff --git a/chrome/browser/policy/cloud/user_policy_signin_service_base.cc
b/chrome/browser/policy/cloud/user_policy_signin_service_base.cc
index
db69d5a25a08c8ae6675f0dc700fc42be2d91ee4..e2100b8babc86b24fe97eb97b203dd970dd32f5b
100644
--- a/chrome/browser/policy/cloud/user_policy_signin_service_base.cc
+++ b/chrome/browser/policy/cloud/user_policy_signin_service_base.cc
@@ -9,7 +9,6 @@
 #include "base/message_loop/message_loop.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/policy/cloud/user_cloud_policy_manager_factory.h"
-#include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/common/chrome_content_client.h"
 #include "chrome/common/chrome_switches.h"
@@ -71,14 +70,15 @@ void
UserPolicySigninServiceBase::FetchPolicyForSignedInUser(
   manager->core()->service()->RefreshPolicy(callback);
 }
 
+void UserPolicySigninServiceBase::GoogleSignedOut(const std::string& username)
{
+  ShutdownUserCloudPolicyManager();
+}
+
 void UserPolicySigninServiceBase::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   switch (type) {
-    case chrome::NOTIFICATION_GOOGLE_SIGNED_OUT:
-      ShutdownUserCloudPolicyManager();
-      break;
     case chrome::NOTIFICATION_PROFILE_ADDED:
       // A new profile has been loaded - if it's signed in, then initialize the
       // UCPM, otherwise shut down the UCPM (which deletes any cached policy
@@ -131,6 +131,8 @@ void
UserPolicySigninServiceBase::OnClientError(CloudPolicyClient* client) {
 }
 
 void UserPolicySigninServiceBase::Shutdown() {
+  if (signin_manager())
+    signin_manager()->RemoveObserver(this);
   PrepareForUserCloudPolicyManagerShutdown();
 }
 
@@ -179,12 +181,11 @@ void
UserPolicySigninServiceBase::InitializeOnProfileReady(Profile* profile) {
     return;
   }
 
-  // Shutdown the UserCloudPolicyManager when the user signs out. We do
-  // this here because we don't want to get SIGNED_OUT notifications until
-  // after the profile has started initializing (http://crbug.com/316229).
-  registrar_.Add(this,
-                 chrome::NOTIFICATION_GOOGLE_SIGNED_OUT,
-                 content::Source<Profile>(profile));
+  // Shutdown the UserCloudPolicyManager when the user signs out. We start
+  // observing the SigninManager here because we don't want to get signout
+  // notifications until after the profile has started initializing
+  // (http://crbug.com/316229).
+  signin_manager()->AddObserver(this);
 
   std::string username = signin_manager()->GetAuthenticatedUsername();
   if (username.empty())

[blundell, 2014-03-30 18:29:01]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-30 18:29:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/187693002/140001

[commit-bot: I haz the power, 2014-03-30 20:57:17]

Change committed as 260468