Fix crashes in ExtensionService

Fix crashes when the ExtensionService isn't present (e.g. incognito, chrome os before login).
This is a workaround until http://code.google.com/p/chromium/issues/detail?id=73768 is fixed.

BUG=73658
TEST=with chromeos=1 build, go to any site and check no crash.

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

[piman, 2011-02-18 23:22:15]

r75122 introduces a crash any time one visits any page if no ExtensionService
has started. This fixes it.

[jstritar, 2011-02-22 16:03:03]

On 2011/02/18 23:22:15, piman wrote:
> r75122 introduces a crash any time one visits any page if no ExtensionService
> has started. This fixes it.

LGTM. Could you also associate this with bug 73658? Someone created that this
weekend.

[Erik does not do reviews, 2011-02-22 16:41:03]

Please don't land this.

[Erik does not do reviews, 2011-02-22 16:41:47]

Sorry, hit send too quickly.

Please don't land this without understanding why this is happening.  Adding a
bunch of null checks doesn't get to the core of the bug and may actually mask
future regressions.

[piman, 2011-02-22 17:19:17]

On Tue, Feb 22, 2011 at 8:41 AM, <erikkay@chromium.org> wrote:

> Sorry, hit send too quickly.
>
> Please don't land this without understanding why this is happening.  Adding
> a
> bunch of null checks doesn't get to the core of the bug and may actually
> mask
> future regressions.


What do you suggest ? The alternative is to revert the original change. This
is seriously blocking development.
I've looked at the code that explicitly doesn't start the extension service.
I'm not sure what you're asking... There are many other instances where it
is already checked explicitly.

Antoine


>
>
> http://codereview.chromium.org/6544023/
>

[piman, 2011-02-22 17:19:19]

On Tue, Feb 22, 2011 at 8:41 AM, <erikkay@chromium.org> wrote:

> Sorry, hit send too quickly.
>
> Please don't land this without understanding why this is happening.  Adding
> a
> bunch of null checks doesn't get to the core of the bug and may actually
> mask
> future regressions.


What do you suggest ? The alternative is to revert the original change. This
is seriously blocking development.
I've looked at the code that explicitly doesn't start the extension service.
I'm not sure what you're asking... There are many other instances where it
is already checked explicitly.

Antoine


>
>
> http://codereview.chromium.org/6544023/
>

[Erik does not do reviews, 2011-02-22 17:34:39]

On 2011/02/22 17:19:19, piman wrote:
> On Tue, Feb 22, 2011 at 8:41 AM, <mailto:erikkay@chromium.org> wrote:
> 
> > Sorry, hit send too quickly.
> >
> > Please don't land this without understanding why this is happening.  Adding
> > a
> > bunch of null checks doesn't get to the core of the bug and may actually
> > mask
> > future regressions.
> 
> 
> What do you suggest ? 

The question is why isn't there an extension service in this case.  I understand
that there's a case at the login screen where this can happen, but there should
always be an extension service in other cases.  How is it possible that
launching the browser in development gets into this state?


> The alternative is to revert the original change. This
> is seriously blocking development.

If you have no leads as to how we're getting into this state and it's blocking
development, then another alternative would be to at least label this CL and the
changes associated with it as temporary with TODOs, etc.  The way it's written
is as if this is the permanent solution.


> I've looked at the code that explicitly doesn't start the extension service.

Under what conditions doesn't it start?  ChromeOS supports extensions, so it's
clearly not the normal case.  I know about the login window case, but this is
after the browser has started.


> I'm not sure what you're asking... There are many other instances where it
> is already checked explicitly.

There are probably too many in the code already.  There are definitely a few
cases where it's necessary (where code can run very early before the service has
been created), but I'm sure others have crept in to cover cases that shouldn't
have been.

[piman, 2011-02-22 17:46:05]

On Tue, Feb 22, 2011 at 9:34 AM, <erikkay@chromium.org> wrote:

> On 2011/02/22 17:19:19, piman wrote:
>
>  On Tue, Feb 22, 2011 at 8:41 AM, <mailto:erikkay@chromium.org> wrote:
>>
>
>  > Sorry, hit send too quickly.
>> >
>> > Please don't land this without understanding why this is happening.
>>  Adding
>> > a
>> > bunch of null checks doesn't get to the core of the bug and may actually
>> > mask
>> > future regressions.
>>
>
>
>  What do you suggest ?
>>
>
> The question is why isn't there an extension service in this case.  I
> understand
> that there's a case at the login screen where this can happen, but there
> should
> always be an extension service in other cases.  How is it possible that
> launching the browser in development gets into this state?


When you compile the browser with chromeos=1 and you launch it you get into
the state where it's effectively in "login" mode, including no extension
service.

>
>
>
>  The alternative is to revert the original change. This
>> is seriously blocking development.
>>
>
> If you have no leads as to how we're getting into this state and it's
> blocking
> development, then another alternative would be to at least label this CL
> and the
> changes associated with it as temporary with TODOs, etc.  The way it's
> written
> is as if this is the permanent solution.


The code dereferences a NULL pointer. It's wrong. Worse than that, it
crashes 2 stacks deep because the NULL pointer doesn't get dereferenced
immediately. It only gets dereferenced through a field and that's even more
dangerous because if that field end up being at a sufficiently high offset,
then you'll randomly corrupt memory.
The profiles's extension service can be NULL. If it's not supposed to be
NULL when running these code paths, then it should be explicitly DCHECK-ed,
with some explanation of why in this code path it cannot be NULL.

Truth is, in some of them, it actually can be NULL.



> I've looked at the code that explicitly doesn't start the extension
>> service.
>>
>
> Under what conditions doesn't it start?  ChromeOS supports extensions, so
> it's
> clearly not the normal case.  I know about the login window case, but this
> is
> after the browser has started.


Compile chrome with chromeos=1
Run it.
Go to google.com
Crash.

It's simple.

I'm not sure what you're asking... There are many other instances where it
>> is already checked explicitly.
>>
>
> There are probably too many in the code already.  There are definitely a
> few
> cases where it's necessary (where code can run very early before the
> service has
> been created), but I'm sure others have crept in to cover cases that
> shouldn't
> have been.
>
>
>
>
> http://codereview.chromium.org/6544023/
>

[Erik does not do reviews, 2011-02-22 18:14:01]

On 2011/02/22 17:46:05, piman wrote:
> On Tue, Feb 22, 2011 at 9:34 AM, <mailto:erikkay@chromium.org> wrote:
> 
> > On 2011/02/22 17:19:19, piman wrote:
> >
> >  On Tue, Feb 22, 2011 at 8:41 AM, <mailto:erikkay@chromium.org> wrote:
> >>
> >
> >  > Sorry, hit send too quickly.
> >> >
> >> > Please don't land this without understanding why this is happening.
> >>  Adding
> >> > a
> >> > bunch of null checks doesn't get to the core of the bug and may actually
> >> > mask
> >> > future regressions.
> >>
> >
> >
> >  What do you suggest ?
> >>
> >
> > The question is why isn't there an extension service in this case.  I
> > understand
> > that there's a case at the login screen where this can happen, but there
> > should
> > always be an extension service in other cases.  How is it possible that
> > launching the browser in development gets into this state?
> 
> 
> When you compile the browser with chromeos=1 and you launch it you get into
> the state where it's effectively in "login" mode, including no extension
> service.

This is the bit that seems wrong to me.  This isn't a valid state that a user
could ever be in (visible browser chrome but in the "login" mode), right?

> 
> >
> >
> >
> >  The alternative is to revert the original change. This
> >> is seriously blocking development.
> >>
> >
> > If you have no leads as to how we're getting into this state and it's
> > blocking
> > development, then another alternative would be to at least label this CL
> > and the
> > changes associated with it as temporary with TODOs, etc.  The way it's
> > written
> > is as if this is the permanent solution.
> 
> 
> The code dereferences a NULL pointer. It's wrong. Worse than that, it
> crashes 2 stacks deep because the NULL pointer doesn't get dereferenced
> immediately. It only gets dereferenced through a field and that's even more
> dangerous because if that field end up being at a sufficiently high offset,
> then you'll randomly corrupt memory.

I'm not arguing that there's not a bug here.  Clearly a NULL deref isn't OK.

> The profiles's extension service can be NULL. If it's not supposed to be
> NULL when running these code paths, then it should be explicitly DCHECK-ed,
> with some explanation of why in this code path it cannot be NULL.
> 
> Truth is, in some of them, it actually can be NULL.

Exactly.  This is what I mean by the core problem.  The "login" behavior was
spelled out to me as only happening in the login window, not with any browser
chrome visible. What you've described as happening is different than that. This
means that we're missing an invariant somewhere and I'm trying to avoid it
getting papered over.

I'm not trying to slow you down here.  It sounds like this has made ChromeOS
browser development impossible, so please go ahead and land.  However, please
update the CL description indicating that this is a temporary fix, file a bug
about there not being an extension service at this point (and that this CL
should be reverted when that's fixed) and reference that bug here.


> > I've looked at the code that explicitly doesn't start the extension
> >> service.
> >>
> >
> > Under what conditions doesn't it start?  ChromeOS supports extensions, so
> > it's
> > clearly not the normal case.  I know about the login window case, but this
> > is
> > after the browser has started.
> 
> 
> Compile chrome with chromeos=1
> Run it.
> Go to http://google.com
> Crash.
> 
> It's simple.
> 
> I'm not sure what you're asking... There are many other instances where it
> >> is already checked explicitly.
> >>
> >
> > There are probably too many in the code already.  There are definitely a
> > few
> > cases where it's necessary (where code can run very early before the
> > service has
> > been created), but I'm sure others have crept in to cover cases that
> > shouldn't
> > have been.
> >
> >
> >
> >
> > http://codereview.chromium.org/6544023/
> >

[piman, 2011-02-22 18:26:20]

On Tue, Feb 22, 2011 at 10:14 AM, <erikkay@chromium.org> wrote:

> On 2011/02/22 17:46:05, piman wrote:
>
>  On Tue, Feb 22, 2011 at 9:34 AM, <mailto:erikkay@chromium.org> wrote:
>>
>
>  > On 2011/02/22 17:19:19, piman wrote:
>> >
>> >  On Tue, Feb 22, 2011 at 8:41 AM, <mailto:erikkay@chromium.org> wrote:
>> >>
>> >
>> >  > Sorry, hit send too quickly.
>> >> >
>> >> > Please don't land this without understanding why this is happening.
>> >>  Adding
>> >> > a
>> >> > bunch of null checks doesn't get to the core of the bug and may
>> actually
>> >> > mask
>> >> > future regressions.
>> >>
>> >
>> >
>> >  What do you suggest ?
>> >>
>> >
>> > The question is why isn't there an extension service in this case.  I
>> > understand
>> > that there's a case at the login screen where this can happen, but there
>> > should
>> > always be an extension service in other cases.  How is it possible that
>> > launching the browser in development gets into this state?
>>
>
>
>  When you compile the browser with chromeos=1 and you launch it you get
>> into
>> the state where it's effectively in "login" mode, including no extension
>> service.
>>
>
> This is the bit that seems wrong to me.  This isn't a valid state that a
> user
> could ever be in (visible browser chrome but in the "login" mode), right?


I don't know for sure, but that is clearly the mode developers start in.


> >
>> >
>> >
>> >  The alternative is to revert the original change. This
>> >> is seriously blocking development.
>> >>
>> >
>> > If you have no leads as to how we're getting into this state and it's
>> > blocking
>> > development, then another alternative would be to at least label this CL
>> > and the
>> > changes associated with it as temporary with TODOs, etc.  The way it's
>> > written
>> > is as if this is the permanent solution.
>>
>
>
>  The code dereferences a NULL pointer. It's wrong. Worse than that, it
>> crashes 2 stacks deep because the NULL pointer doesn't get dereferenced
>> immediately. It only gets dereferenced through a field and that's even
>> more
>> dangerous because if that field end up being at a sufficiently high
>> offset,
>> then you'll randomly corrupt memory.
>>
>
> I'm not arguing that there's not a bug here.  Clearly a NULL deref isn't
> OK.
>
>
>  The profiles's extension service can be NULL. If it's not supposed to be
>> NULL when running these code paths, then it should be explicitly
>> DCHECK-ed,
>> with some explanation of why in this code path it cannot be NULL.
>>
>
>  Truth is, in some of them, it actually can be NULL.
>>
>
> Exactly.  This is what I mean by the core problem.  The "login" behavior
> was
> spelled out to me as only happening in the login window, not with any
> browser
> chrome visible. What you've described as happening is different than that.
> This
> means that we're missing an invariant somewhere and I'm trying to avoid it
> getting papered over.
>
> I'm not trying to slow you down here.  It sounds like this has made
> ChromeOS
> browser development impossible, so please go ahead and land.  However,
> please
> update the CL description indicating that this is a temporary fix, file a
> bug
> about there not being an extension service at this point (and that this CL
> should be reverted when that's fixed) and reference that bug here.
>
>
>
>  > I've looked at the code that explicitly doesn't start the extension
>> >> service.
>> >>
>> >
>> > Under what conditions doesn't it start?  ChromeOS supports extensions,
>> so
>> > it's
>> > clearly not the normal case.  I know about the login window case, but
>> this
>> > is
>> > after the browser has started.
>>
>
>
>  Compile chrome with chromeos=1
>> Run it.
>> Go to http://google.com
>> Crash.
>>
>
>  It's simple.
>>
>
>  I'm not sure what you're asking... There are many other instances where it
>> >> is already checked explicitly.
>> >>
>> >
>> > There are probably too many in the code already.  There are definitely a
>> > few
>> > cases where it's necessary (where code can run very early before the
>> > service has
>> > been created), but I'm sure others have crept in to cover cases that
>> > shouldn't
>> > have been.
>> >
>> >
>> >
>> >
>> > http://codereview.chromium.org/6544023/
>> >
>>
>
>
>
> http://codereview.chromium.org/6544023/
>

[Aaron Boodman, 2011-02-22 19:53:02]

The design of this part of the system was that there would *always* be a valid
ES instance, and many, many things in the system depend on that being true.

This simplifies implementation of all the clients. They just iterate extensions
the ES has loaded. They don't need to know why there are zero.

Instead of not creating an extension service, it seems like it would be better
to tell it to not load anything.

[piman, 2011-02-22 22:08:07]

PTAL, I filed bug 73768, I added a comment everywhere that links to the bug.

[Erik does not do reviews, 2011-02-22 22:32:24]

thanks