Fix chromium-os:10777 and other sync related crashes.

Fix chromium-os:10777 and other sync related crashes.
Return NULL when  GetProfileSyncService() is called without a cros_user 
argument (if not already initialized) and protect the settings code when 
sync service is NULL.
A side effect of this is that sync will be disabled if chrome is not started
from the login screen in ChromeOS (e.g. when debugging), instead of being 
initialized without a valid backend, so e.g. settings will not have a sync
section, and about:sync will show 'sync disabled'.

BUG=chromium-os:10777, chromium-os:10893
TEST=Test proxy settings from login screen, sync settings page, and about:sync.

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

[stevenjb, 2011-01-13 00:59:59]

After trying a few approaches, I like this best, but am open to alternatives.

[akalin, 2011-01-13 04:14:56]

http://codereview.chromium.org/6260002/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (left):

http://codereview.chromium.org/6260002/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:1251: // If kLoginManager is specified,
we shouldn't call this unless login has
Hmm.  I'd rather callers explicitly check HasProfileSyncService(), since it
looks like there are only three places in the domui handler that needs that
change.  What do you think?

[akalin, 2011-01-13 04:38:42]

http://codereview.chromium.org/6260002/diff/1/chrome/browser/dom_ui/dom_ui.h
File chrome/browser/dom_ui/dom_ui.h (right):

http://codereview.chromium.org/6260002/diff/1/chrome/browser/dom_ui/dom_ui.h#...
chrome/browser/dom_ui/dom_ui.h:133: // Convenience function for UI that requires
access to the original profile.
I think there's no need for this -- GetProfileSyncService() already drills down
to the original profile IIRC

[stevenjb, 2011-01-13 18:00:48]

http://codereview.chromium.org/6260002/diff/1/chrome/browser/dom_ui/dom_ui.h
File chrome/browser/dom_ui/dom_ui.h (right):

http://codereview.chromium.org/6260002/diff/1/chrome/browser/dom_ui/dom_ui.h#...
chrome/browser/dom_ui/dom_ui.h:133: // Convenience function for UI that requires
access to the original profile.
On 2011/01/13 04:38:42, akalin wrote:
> I think there's no need for this -- GetProfileSyncService() already drills
down
> to the original profile IIRC
OK, I wasn't sure if this mattered. Definitely better if it does not - we drill
down to GetOriginalProfile() in about 4 places right now, and not in the other
couple of dozen. I will remove this and if we encounter any problems we can fix
GetProfileSyncService() instead.

http://codereview.chromium.org/6260002/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (left):

http://codereview.chromium.org/6260002/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:1251: // If kLoginManager is specified,
we shouldn't call this unless login has
On 2011/01/13 04:15:02, akalin wrote:
> Hmm.  I'd rather callers explicitly check HasProfileSyncService(), since it
> looks like there are only three places in the domui handler that needs that
> change.  What do you think?

Originally I added checks in the settings code, but that does not address issue
10893 (we would need to remove the DCHECK there), and still leaves open the
possibility of other similar bugs, or more subtle ones where sync appears to be
enabled but the backend isn't initialized yet. Granted, these would only show up
for developers not running with kLoginManager, but even so I think it is better
not to set up sync in this place.

That said, I am fine with fixing the settings code and removing the DCHECK for
10893 if you and Tim prefer that.

[akalin, 2011-01-13 19:35:08]

On 2011/01/13 18:00:48, Steven Bennetts wrote:
> Originally I added checks in the settings code, but that does not address
issue
> 10893 (we would need to remove the DCHECK there), and still leaves open the
> possibility of other similar bugs, or more subtle ones where sync appears to
be
> enabled but the backend isn't initialized yet. Granted, these would only show
up
> for developers not running with kLoginManager, but even so I think it is
better
> not to set up sync in this place.
> 
> That said, I am fine with fixing the settings code and removing the DCHECK for
> 10893 if you and Tim prefer that.

Ah, so one can be running ChromeOS without kLoginManager?  How about:

#if defined(OS_CHROMEOS)
  if (!sync_service_.get()) {
      if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kLoginManager))
{
        LOG(FATAL) << "GetProfileSyncService() called before login complete.";
      } else {
        return NULL;
      }
  }

?

[stevenjb (google-dont-use), 2011-01-13 20:01:31]

I can leave in the FATAL check for kLoginManager and add
HasProfileSyncService()
to the settings code, but my thinking was that if the code already handles
NULL correctly (like the settings code now does), then we are good, and if
it does not, it will crash anyway and we will still catch it. In other
words, if we return NULL and check on that, we don't have to add the
additional HasProfileSyncService() everywhere.

On Thu, Jan 13, 2011 at 11:35 AM, <akalin@chromium.org> wrote:

> On 2011/01/13 18:00:48, Steven Bennetts wrote:
>
>> Originally I added checks in the settings code, but that does not address
>>
> issue
>
>> 10893 (we would need to remove the DCHECK there), and still leaves open
>> the
>> possibility of other similar bugs, or more subtle ones where sync appears
>> to
>>
> be
>
>> enabled but the backend isn't initialized yet. Granted, these would only
>> show
>>
> up
>
>> for developers not running with kLoginManager, but even so I think it is
>>
> better
>
>> not to set up sync in this place.
>>
>
>  That said, I am fine with fixing the settings code and removing the DCHECK
>> for
>> 10893 if you and Tim prefer that.
>>
>
> Ah, so one can be running ChromeOS without kLoginManager?  How about:
>
> #if defined(OS_CHROMEOS)
>  if (!sync_service_.get()) {
>      if
> (CommandLine::ForCurrentProcess()->HasSwitch(switches::kLoginManager))
>
> {
>        LOG(FATAL) << "GetProfileSyncService() called before login
> complete.";
>      } else {
>        return NULL;
>      }
>  }
>
> ?
>
>
> http://codereview.chromium.org/6260002/
>

[akalin, 2011-01-13 20:01:42]

Sure, I think there's no need for HasPSS() if the calling code handles
NULL correctly.  The important part is that GetPSS() avoids
constructing the PSS on ChromeOS if it doesn't have a user.  I like
the FATAL check because it gives a better stack trace in the case when
login manager is used.

On Thu, Jan 13, 2011 at 11:55 AM, Steven Bennetts <stevenjb@google.com> wrote:
> I can leave in the FATAL check for kLoginManager and
> add HasProfileSyncService() to the settings code, but my thinking was that
> if the code already handles NULL correctly (like the settings code now
> does), then we are good, and if it does not, it will crash anyway and we
> will still catch it. In other words, if we return NULL and check on that, we
> don't have to add the additional HasProfileSyncService() everywhere.
>
> On Thu, Jan 13, 2011 at 11:35 AM, <akalin@chromium.org> wrote:
>>
>> On 2011/01/13 18:00:48, Steven Bennetts wrote:
>>>
>>> Originally I added checks in the settings code, but that does not address
>>
>> issue
>>>
>>> 10893 (we would need to remove the DCHECK there), and still leaves open
>>> the
>>> possibility of other similar bugs, or more subtle ones where sync appears
>>> to
>>
>> be
>>>
>>> enabled but the backend isn't initialized yet. Granted, these would only
>>> show
>>
>> up
>>>
>>> for developers not running with kLoginManager, but even so I think it is
>>
>> better
>>>
>>> not to set up sync in this place.
>>
>>> That said, I am fine with fixing the settings code and removing the
>>> DCHECK for
>>> 10893 if you and Tim prefer that.
>>
>> Ah, so one can be running ChromeOS without kLoginManager?  How about:
>>
>> #if defined(OS_CHROMEOS)
>>  if (!sync_service_.get()) {
>>      if
>> (CommandLine::ForCurrentProcess()->HasSwitch(switches::kLoginManager))
>> {
>>        LOG(FATAL) << "GetProfileSyncService() called before login
>> complete.";
>>      } else {
>>        return NULL;
>>      }
>>  }
>>
>> ?
>>
>> http://codereview.chromium.org/6260002/
>
>

[stevenjb (google-dont-use), 2011-01-13 21:56:12]

But if I leave the FATAL check there, I will have to add the
HasProfileSyncService()
check to the settings code since it does get called when kLoginManager is
true and will trigger the FATAL call. Same with the DCHECK for 10893.

On Thu, Jan 13, 2011 at 12:01 PM, Fred Akalin <akalin@chromium.org> wrote:

> Sure, I think there's no need for HasPSS() if the calling code handles
> NULL correctly.  The important part is that GetPSS() avoids
> constructing the PSS on ChromeOS if it doesn't have a user.  I like
> the FATAL check because it gives a better stack trace in the case when
> login manager is used.
>
> On Thu, Jan 13, 2011 at 11:55 AM, Steven Bennetts <stevenjb@google.com>
> wrote:
> > I can leave in the FATAL check for kLoginManager and
> > add HasProfileSyncService() to the settings code, but my thinking was
> that
> > if the code already handles NULL correctly (like the settings code now
> > does), then we are good, and if it does not, it will crash anyway and we
> > will still catch it. In other words, if we return NULL and check on that,
> we
> > don't have to add the additional HasProfileSyncService() everywhere.
> >
> > On Thu, Jan 13, 2011 at 11:35 AM, <akalin@chromium.org> wrote:
> >>
> >> On 2011/01/13 18:00:48, Steven Bennetts wrote:
> >>>
> >>> Originally I added checks in the settings code, but that does not
> address
> >>
> >> issue
> >>>
> >>> 10893 (we would need to remove the DCHECK there), and still leaves open
> >>> the
> >>> possibility of other similar bugs, or more subtle ones where sync
> appears
> >>> to
> >>
> >> be
> >>>
> >>> enabled but the backend isn't initialized yet. Granted, these would
> only
> >>> show
> >>
> >> up
> >>>
> >>> for developers not running with kLoginManager, but even so I think it
> is
> >>
> >> better
> >>>
> >>> not to set up sync in this place.
> >>
> >>> That said, I am fine with fixing the settings code and removing the
> >>> DCHECK for
> >>> 10893 if you and Tim prefer that.
> >>
> >> Ah, so one can be running ChromeOS without kLoginManager?  How about:
> >>
> >> #if defined(OS_CHROMEOS)
> >>  if (!sync_service_.get()) {
> >>      if
> >> (CommandLine::ForCurrentProcess()->HasSwitch(switches::kLoginManager))
> >> {
> >>        LOG(FATAL) << "GetProfileSyncService() called before login
> >> complete.";
> >>      } else {
> >>        return NULL;
> >>      }
> >>  }
> >>
> >> ?
> >>
> >> http://codereview.chromium.org/6260002/
> >
> >
>

[akalin, 2011-01-13 22:02:42]

Okay, I think I managed to retrace your reasoning. :) I'm now convinced that
returning NULL from GetPSS() is the best thing to do.

On 2011/01/13 21:56:12, stevenjb wrote:
> But if I leave the FATAL check there, I will have to add the
> HasProfileSyncService()
> check to the settings code since it does get called when kLoginManager is
> true and will trigger the FATAL call. Same with the DCHECK for 10893.
> 
> On Thu, Jan 13, 2011 at 12:01 PM, Fred Akalin <mailto:akalin@chromium.org>
wrote:
> 
> > Sure, I think there's no need for HasPSS() if the calling code handles
> > NULL correctly.  The important part is that GetPSS() avoids
> > constructing the PSS on ChromeOS if it doesn't have a user.  I like
> > the FATAL check because it gives a better stack trace in the case when
> > login manager is used.
> >
> > On Thu, Jan 13, 2011 at 11:55 AM, Steven Bennetts
<mailto:stevenjb@google.com>
> > wrote:
> > > I can leave in the FATAL check for kLoginManager and
> > > add HasProfileSyncService() to the settings code, but my thinking was
> > that
> > > if the code already handles NULL correctly (like the settings code now
> > > does), then we are good, and if it does not, it will crash anyway and we
> > > will still catch it. In other words, if we return NULL and check on that,
> > we
> > > don't have to add the additional HasProfileSyncService() everywhere.
> > >
> > > On Thu, Jan 13, 2011 at 11:35 AM, <mailto:akalin@chromium.org> wrote:
> > >>
> > >> On 2011/01/13 18:00:48, Steven Bennetts wrote:
> > >>>
> > >>> Originally I added checks in the settings code, but that does not
> > address
> > >>
> > >> issue
> > >>>
> > >>> 10893 (we would need to remove the DCHECK there), and still leaves open
> > >>> the
> > >>> possibility of other similar bugs, or more subtle ones where sync
> > appears
> > >>> to
> > >>
> > >> be
> > >>>
> > >>> enabled but the backend isn't initialized yet. Granted, these would
> > only
> > >>> show
> > >>
> > >> up
> > >>>
> > >>> for developers not running with kLoginManager, but even so I think it
> > is
> > >>
> > >> better
> > >>>
> > >>> not to set up sync in this place.
> > >>
> > >>> That said, I am fine with fixing the settings code and removing the
> > >>> DCHECK for
> > >>> 10893 if you and Tim prefer that.
> > >>
> > >> Ah, so one can be running ChromeOS without kLoginManager?  How about:
> > >>
> > >> #if defined(OS_CHROMEOS)
> > >>  if (!sync_service_.get()) {
> > >>      if
> > >> (CommandLine::ForCurrentProcess()->HasSwitch(switches::kLoginManager))
> > >> {
> > >>        LOG(FATAL) << "GetProfileSyncService() called before login
> > >> complete.";
> > >>      } else {
> > >>        return NULL;
> > >>      }
> > >>  }
> > >>
> > >> ?
> > >>
> > >> http://codereview.chromium.org/6260002/
> > >
> > >
> >

[akalin, 2011-01-13 22:03:49]

http://codereview.chromium.org/6260002/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6260002/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:1253: // the login code. Use
HasProfileSyncService() to guard against this.
Mention that we may be called on chromeos without kLoginManager and if so,
callers must check for NULL.  Since this returns NULL, then there's no need for
HasPSS() is there?

[stevenjb, 2011-01-13 22:46:58]

http://codereview.chromium.org/6260002/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6260002/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:1253: // the login code. Use
HasProfileSyncService() to guard against this.
On 2011/01/13 22:03:49, akalin wrote:
> Mention that we may be called on chromeos without kLoginManager and if so,
> callers must check for NULL.  Since this returns NULL, then there's no need
for
> HasPSS() is there?

Done.

[akalin, 2011-01-14 00:26:37]

LGTM

http://codereview.chromium.org/6260002/diff/20001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/personal_options_handler.cc (right):

http://codereview.chromium.org/6260002/diff/20001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/personal_options_handler.cc:157: DCHECK(service);
I prefer these to be CHECK()s since you're dereferencing them immediately after,
but I leave it to you