chromeos: Stop calling ProfileManager::GetDefaultProfile() from extension event dispatch functions

chromeos: Stop calling ProfileManager::GetDefaultProfile() from extension event dispatch functions

During login, GetDefaultProfile() changes the profile which it returns from the one for the login screen to the one which is used for the user browsing session.
The problem is that GetDefaultProfile() starts to return the new profile before it gets ready to be used.
It means using GetDefaultProfile() for events which can happen during login may result in crash.
To avoid this, observers remember the profile which they should use instead of calling GetDefaultProfile() every time.  By doing this, they never access the not-ready profile and the profile switching is safely performed by ChromeBrowserMainPartsChromeos::PostProfileInit which is called for the new profile.

BUG=148552
TEST=Boot a Chromebook, type the password for a user, start pressing brightness up/down key randomly, press Enter to login while keeping pressing brightness keys, ensure no crash report is sent during login.

[hashimoto, 2012-09-12 19:53:45]

satorux,
You might be somehow familiar with this code.
Could you review this change?

[satorux1, 2012-09-12 20:33:23]

http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
File chrome/browser/extensions/system/system_api.h (right):

http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
chrome/browser/extensions/system/system_api.h:45: void
DispatchWokeUpEvent(Profile* profile);
While you are at it. Could you add function comments? In particular, please
describe why Profile* is needed.

[hashimoto, 2012-09-12 20:42:50]

http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
File chrome/browser/extensions/system/system_api.h (right):

http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
chrome/browser/extensions/system/system_api.h:45: void
DispatchWokeUpEvent(Profile* profile);
On 2012/09/12 20:33:23, satorux1 wrote:
> While you are at it. Could you add function comments? In particular, please
> describe why Profile* is needed.

Done.

[hashimoto, 2012-09-12 23:38:04]

ping?
On 2012/09/12 20:42:50, hashimoto wrote:
>
http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
> File chrome/browser/extensions/system/system_api.h (right):
> 
>
http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
> chrome/browser/extensions/system/system_api.h:45: void
> DispatchWokeUpEvent(Profile* profile);
> On 2012/09/12 20:33:23, satorux1 wrote:
> > While you are at it. Could you add function comments? In particular, please
> > describe why Profile* is needed.
> 
> Done.

[satorux1, 2012-09-12 23:51:00]

I'm confused about "GetDefaultProfile() may return a profile before it gets
ready to be used."

Did you mean this function might be called before it's ready hence it could
return NULL?


On 2012/09/12 23:38:04, hashimoto wrote:
> ping?
> On 2012/09/12 20:42:50, hashimoto wrote:
> >
>
http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
> > File chrome/browser/extensions/system/system_api.h (right):
> > 
> >
>
http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
> > chrome/browser/extensions/system/system_api.h:45: void
> > DispatchWokeUpEvent(Profile* profile);
> > On 2012/09/12 20:33:23, satorux1 wrote:
> > > While you are at it. Could you add function comments? In particular,
please
> > > describe why Profile* is needed.
> > 
> > Done.

[hashimoto, 2012-09-13 00:08:21]

The following events happen during login.

(1) Before login, ProfileManager::GetDefaultProfile() returns a profile which is
not associated with any user.
(2) User initiates login.
(1) Setup of new profile for the logged-in user starts.
(2) ProfileManager::GetDefaultProfile() begins to return the new profile. (but,
the profile is not ready)
(3) The new profile gets ready to be used.

The crash in the bug happens because DispatchEvent() is called at the timing
between (2) and (3).
By remembering the profile instead of calling GetDefaultProfile() every time, we
don't have this kind of crash.

On 2012/09/12 23:51:00, satorux1 wrote:
> I'm confused about "GetDefaultProfile() may return a profile before it gets
> ready to be used."
> 
> Did you mean this function might be called before it's ready hence it could
> return NULL?
> 
> 
> On 2012/09/12 23:38:04, hashimoto wrote:
> > ping?
> > On 2012/09/12 20:42:50, hashimoto wrote:
> > >
> >
>
http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
> > > File chrome/browser/extensions/system/system_api.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
> > > chrome/browser/extensions/system/system_api.h:45: void
> > > DispatchWokeUpEvent(Profile* profile);
> > > On 2012/09/12 20:33:23, satorux1 wrote:
> > > > While you are at it. Could you add function comments? In particular,
> please
> > > > describe why Profile* is needed.
> > > 
> > > Done.

[satorux1, 2012-09-13 00:18:17]

On 2012/09/13 00:08:21, hashimoto wrote:
> The following events happen during login.
> 
> (1) Before login, ProfileManager::GetDefaultProfile() returns a profile which
is
> not associated with any user.
> (2) User initiates login.
> (1) Setup of new profile for the logged-in user starts.
> (2) ProfileManager::GetDefaultProfile() begins to return the new profile.
(but,
> the profile is not ready)
> (3) The new profile gets ready to be used.
> 
> The crash in the bug happens because DispatchEvent() is called at the timing
> between (2) and (3).
> By remembering the profile instead of calling GetDefaultProfile() every time,
we
> don't have this kind of crash.

Is it OK to use the profile not associated with any user? Looks a bit scary.

> 
> On 2012/09/12 23:51:00, satorux1 wrote:
> > I'm confused about "GetDefaultProfile() may return a profile before it gets
> > ready to be used."
> > 
> > Did you mean this function might be called before it's ready hence it could
> > return NULL?
> > 
> > 
> > On 2012/09/12 23:38:04, hashimoto wrote:
> > > ping?
> > > On 2012/09/12 20:42:50, hashimoto wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
> > > > File chrome/browser/extensions/system/system_api.h (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
> > > > chrome/browser/extensions/system/system_api.h:45: void
> > > > DispatchWokeUpEvent(Profile* profile);
> > > > On 2012/09/12 20:33:23, satorux1 wrote:
> > > > > While you are at it. Could you add function comments? In particular,
> > please
> > > > > describe why Profile* is needed.
> > > > 
> > > > Done.

[hashimoto, 2012-09-13 00:24:43]

On 2012/09/13 00:18:17, satorux1 wrote:
> On 2012/09/13 00:08:21, hashimoto wrote:
> > The following events happen during login.
> > 
> > (1) Before login, ProfileManager::GetDefaultProfile() returns a profile
which
> is
> > not associated with any user.
> > (2) User initiates login.
> > (1) Setup of new profile for the logged-in user starts.
> > (2) ProfileManager::GetDefaultProfile() begins to return the new profile.
> (but,
> > the profile is not ready)
> > (3) The new profile gets ready to be used.
> > 
> > The crash in the bug happens because DispatchEvent() is called at the timing
> > between (2) and (3).
> > By remembering the profile instead of calling GetDefaultProfile() every
time,
> we
> > don't have this kind of crash.
> 
> Is it OK to use the profile not associated with any user? Looks a bit scary.
By " profile which is not associated with any user.", I mean the profile which
is used on the login screen.
Of course it's safe to use.
> 
> > 
> > On 2012/09/12 23:51:00, satorux1 wrote:
> > > I'm confused about "GetDefaultProfile() may return a profile before it
gets
> > > ready to be used."
> > > 
> > > Did you mean this function might be called before it's ready hence it
could
> > > return NULL?
> > > 
> > > 
> > > On 2012/09/12 23:38:04, hashimoto wrote:
> > > > ping?
> > > > On 2012/09/12 20:42:50, hashimoto wrote:
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
> > > > > File chrome/browser/extensions/system/system_api.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
> > > > > chrome/browser/extensions/system/system_api.h:45: void
> > > > > DispatchWokeUpEvent(Profile* profile);
> > > > > On 2012/09/12 20:33:23, satorux1 wrote:
> > > > > > While you are at it. Could you add function comments? In particular,
> > > please
> > > > > > describe why Profile* is needed.
> > > > > 
> > > > > Done.

[satorux1, 2012-09-13 00:40:21]

On 2012/09/13 00:24:43, hashimoto wrote:
> On 2012/09/13 00:18:17, satorux1 wrote:
> > On 2012/09/13 00:08:21, hashimoto wrote:
> > > The following events happen during login.
> > > 
> > > (1) Before login, ProfileManager::GetDefaultProfile() returns a profile
> which
> > is
> > > not associated with any user.
> > > (2) User initiates login.
> > > (1) Setup of new profile for the logged-in user starts.
> > > (2) ProfileManager::GetDefaultProfile() begins to return the new profile.
> > (but,
> > > the profile is not ready)
> > > (3) The new profile gets ready to be used.
> > > 
> > > The crash in the bug happens because DispatchEvent() is called at the
timing
> > > between (2) and (3).
> > > By remembering the profile instead of calling GetDefaultProfile() every
> time,
> > we
> > > don't have this kind of crash.
> > 
> > Is it OK to use the profile not associated with any user? Looks a bit scary.
> By " profile which is not associated with any user.", I mean the profile which
> is used on the login screen.
> Of course it's safe to use.

I see. Could you revise the patch description based on the conversation? I think
this part is confusing: "GetDefaultProfile() may return a profile before it gets
ready to be used."

The issue is complicated, so it's nice to have detailed background on the
change.


> > 
> > > 
> > > On 2012/09/12 23:51:00, satorux1 wrote:
> > > > I'm confused about "GetDefaultProfile() may return a profile before it
> gets
> > > > ready to be used."
> > > > 
> > > > Did you mean this function might be called before it's ready hence it
> could
> > > > return NULL?
> > > > 
> > > > 
> > > > On 2012/09/12 23:38:04, hashimoto wrote:
> > > > > ping?
> > > > > On 2012/09/12 20:42:50, hashimoto wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
> > > > > > File chrome/browser/extensions/system/system_api.h (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/10911261/diff/1/chrome/browser/extensions/syst...
> > > > > > chrome/browser/extensions/system/system_api.h:45: void
> > > > > > DispatchWokeUpEvent(Profile* profile);
> > > > > > On 2012/09/12 20:33:23, satorux1 wrote:
> > > > > > > While you are at it. Could you add function comments? In
particular,
> > > > please
> > > > > > > describe why Profile* is needed.
> > > > > > 
> > > > > > Done.

[satorux1, 2012-09-13 00:42:56]

http://codereview.chromium.org/10911261/diff/4002/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/10911261/diff/4002/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:460:
brightness_observer_.reset(new chromeos::BrightnessObserver(profile()));
Please write some comment about why these observers need to take profile() and
cache the pointer

[hashimoto, 2012-09-13 01:01:39]

Updated the change description.

http://codereview.chromium.org/10911261/diff/4002/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/10911261/diff/4002/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:460:
brightness_observer_.reset(new chromeos::BrightnessObserver(profile()));
On 2012/09/13 00:42:56, satorux1 wrote:
> Please write some comment about why these observers need to take profile() and
> cache the pointer

There is already a comment "they use the profile to dispatch extension events"
above.
And I don't think we should add some comment about why we are not using
GetDefaultProfile() since ProfileManager::GetDefaultProfile() is marked as
DEPRECATED in profile_manager.h (even though they allow Chrome OS code to use
it).

Thoughts?

[satorux1, 2012-09-13 01:13:46]

LGTM for fixing this bug now.

However, per the following description:

"The problem is that GetDefaultProfile() starts to return the new profile before
it gets ready to be used." 

this seems to be a bug in GetDefaultProfile(). Would it be difficult to fix this
in GetDefaultProfile()? Otherwise, this problem can cause crashes elsewhere.
Could you file a bug on this? Or am I misunderstanding something?

http://codereview.chromium.org/10911261/diff/4002/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/10911261/diff/4002/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:460:
brightness_observer_.reset(new chromeos::BrightnessObserver(profile()));
On 2012/09/13 01:01:40, hashimoto wrote:
> On 2012/09/13 00:42:56, satorux1 wrote:
> > Please write some comment about why these observers need to take profile()
and
> > cache the pointer
> 
> There is already a comment "they use the profile to dispatch extension events"
> above.
> And I don't think we should add some comment about why we are not using
> GetDefaultProfile() since ProfileManager::GetDefaultProfile() is marked as
> DEPRECATED in profile_manager.h (even though they allow Chrome OS code to use
> it).
> 
> Thoughts?

I thought it'd be nice to explain why these observers need to cache the pointer
to the Profile used at this moment (Profile for login), as it's solving a subtle
problem.

[hashimoto, 2012-09-13 04:06:19]

On 2012/09/13 01:13:46, satorux1 wrote:
> LGTM for fixing this bug now.
> 
> However, per the following description:
> 
> "The problem is that GetDefaultProfile() starts to return the new profile
before
> it gets ready to be used." 
> 
> this seems to be a bug in GetDefaultProfile(). Would it be difficult to fix
this
> in GetDefaultProfile()? Otherwise, this problem can cause crashes elsewhere.
> Could you file a bug on this? Or am I misunderstanding something?
Filed crbug.com/148878
Since this does not fix the root cause, I would like to submit a small
alternative fix for this bug instead of this change.
http://codereview.chromium.org/10905250/
Does this look good for you?
> 
>
http://codereview.chromium.org/10911261/diff/4002/chrome/browser/chromeos/chr...
> File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):
> 
>
http://codereview.chromium.org/10911261/diff/4002/chrome/browser/chromeos/chr...
> chrome/browser/chromeos/chrome_browser_main_chromeos.cc:460:
> brightness_observer_.reset(new chromeos::BrightnessObserver(profile()));
> On 2012/09/13 01:01:40, hashimoto wrote:
> > On 2012/09/13 00:42:56, satorux1 wrote:
> > > Please write some comment about why these observers need to take profile()
> and
> > > cache the pointer
> > 
> > There is already a comment "they use the profile to dispatch extension
events"
> > above.
> > And I don't think we should add some comment about why we are not using
> > GetDefaultProfile() since ProfileManager::GetDefaultProfile() is marked as
> > DEPRECATED in profile_manager.h (even though they allow Chrome OS code to
use
> > it).
> > 
> > Thoughts?
> 
> I thought it'd be nice to explain why these observers need to cache the
pointer
> to the Profile used at this moment (Profile for login), as it's solving a
subtle
> problem.

[Mihai Parparita -not on Chrome, 2012-09-13 04:15:38]

I'm pretty sure the most correct fix is to use EventRouterForwarder, which
doesn't require a Profile instance (and instead dispatches the event to all
loaded profiles that have an extension with a listener for that event). See
https://chromiumcodereview.appspot.com/10836341/diff/28011/chrome/browser/ext...
example.


Mihai

On Wed, Sep 12, 2012 at 9:06 PM, <hashimoto@chromium.org> wrote:

> On 2012/09/13 01:13:46, satorux1 wrote:
>
>> LGTM for fixing this bug now.
>>
>
>  However, per the following description:
>>
>
>  "The problem is that GetDefaultProfile() starts to return the new profile
>>
> before
>
>> it gets ready to be used."
>>
>
>  this seems to be a bug in GetDefaultProfile(). Would it be difficult to
>> fix
>>
> this
>
>> in GetDefaultProfile()? Otherwise, this problem can cause crashes
>> elsewhere.
>> Could you file a bug on this? Or am I misunderstanding something?
>>
> Filed crbug.com/148878
> Since this does not fix the root cause, I would like to submit a small
> alternative fix for this bug instead of this change.
>
http://codereview.chromium.**org/10905250/<http://codereview.chromium.org/109...
> Does this look good for you?
>
>
>
> http://codereview.chromium.**org/10911261/diff/4002/chrome/**
>
browser/chromeos/chrome_**browser_main_chromeos.cc<http://codereview.chromium.org/10911261/diff/4002/chrome/browser/chromeos/chrome_browser_main_chromeos.cc>
>
>> File chrome/browser/chromeos/**chrome_browser_main_chromeos.**cc (right):
>>
>
>
> http://codereview.chromium.**org/10911261/diff/4002/chrome/**
>
browser/chromeos/chrome_**browser_main_chromeos.cc#**newcode460<http://codereview.chromium.org/10911261/diff/4002/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode460>
>
>> chrome/browser/chromeos/**chrome_browser_main_chromeos.**cc:460:
>> brightness_observer_.reset(new chromeos::BrightnessObserver(**
>> profile()));
>> On 2012/09/13 01:01:40, hashimoto wrote:
>> > On 2012/09/13 00:42:56, satorux1 wrote:
>> > > Please write some comment about why these observers need to take
>> profile()
>> and
>> > > cache the pointer
>> >
>> > There is already a comment "they use the profile to dispatch extension
>>
> events"
>
>> > above.
>> > And I don't think we should add some comment about why we are not using
>> > GetDefaultProfile() since ProfileManager::**GetDefaultProfile() is
>> marked as
>> > DEPRECATED in profile_manager.h (even though they allow Chrome OS code
>> to
>>
> use
>
>> > it).
>> >
>> > Thoughts?
>>
>
>  I thought it'd be nice to explain why these observers need to cache the
>>
> pointer
>
>> to the Profile used at this moment (Profile for login), as it's solving a
>>
> subtle
>
>> problem.
>>
>
>
>
http://codereview.chromium.**org/10911261/<http://codereview.chromium.org/109...
>

[hashimoto, 2012-09-13 19:50:32]

On 2012/09/13 04:15:38, Mihai Parparita wrote:
> I'm pretty sure the most correct fix is to use EventRouterForwarder, which
> doesn't require a Profile instance (and instead dispatches the event to all
> loaded profiles that have an extension with a listener for that event). See
>
https://chromiumcodereview.appspot.com/10836341/diff/28011/chrome/browser/ext...
> example.
> 
> 
> Mihai
Oh, EventRouterForwarder seems to be the exact thing which I want.
Reworked http://codereview.chromium.org/10905250/ with it.
Huge thanks!
> 
> On Wed, Sep 12, 2012 at 9:06 PM, <mailto:hashimoto@chromium.org> wrote:
> 
> > On 2012/09/13 01:13:46, satorux1 wrote:
> >
> >> LGTM for fixing this bug now.
> >>
> >
> >  However, per the following description:
> >>
> >
> >  "The problem is that GetDefaultProfile() starts to return the new profile
> >>
> > before
> >
> >> it gets ready to be used."
> >>
> >
> >  this seems to be a bug in GetDefaultProfile(). Would it be difficult to
> >> fix
> >>
> > this
> >
> >> in GetDefaultProfile()? Otherwise, this problem can cause crashes
> >> elsewhere.
> >> Could you file a bug on this? Or am I misunderstanding something?
> >>
> > Filed crbug.com/148878
> > Since this does not fix the root cause, I would like to submit a small
> > alternative fix for this bug instead of this change.
> >
>
http://codereview.chromium.**org/10905250/%3Chttp://codereview.chromium.org/1...>
> > Does this look good for you?
> >
> >
> >
> > http://codereview.chromium.**org/10911261/diff/4002/chrome/**
> >
>
browser/chromeos/chrome_**browser_main_chromeos.cc<http://codereview.chromium.org/10911261/diff/4002/chrome/browser/chromeos/chrome_browser_main_chromeos.cc>
> >
> >> File chrome/browser/chromeos/**chrome_browser_main_chromeos.**cc (right):
> >>
> >
> >
> > http://codereview.chromium.**org/10911261/diff/4002/chrome/**
> >
>
browser/chromeos/chrome_**browser_main_chromeos.cc#**newcode460<http://codereview.chromium.org/10911261/diff/4002/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode460>
> >
> >> chrome/browser/chromeos/**chrome_browser_main_chromeos.**cc:460:
> >> brightness_observer_.reset(new chromeos::BrightnessObserver(**
> >> profile()));
> >> On 2012/09/13 01:01:40, hashimoto wrote:
> >> > On 2012/09/13 00:42:56, satorux1 wrote:
> >> > > Please write some comment about why these observers need to take
> >> profile()
> >> and
> >> > > cache the pointer
> >> >
> >> > There is already a comment "they use the profile to dispatch extension
> >>
> > events"
> >
> >> > above.
> >> > And I don't think we should add some comment about why we are not using
> >> > GetDefaultProfile() since ProfileManager::**GetDefaultProfile() is
> >> marked as
> >> > DEPRECATED in profile_manager.h (even though they allow Chrome OS code
> >> to
> >>
> > use
> >
> >> > it).
> >> >
> >> > Thoughts?
> >>
> >
> >  I thought it'd be nice to explain why these observers need to cache the
> >>
> > pointer
> >
> >> to the Profile used at this moment (Profile for login), as it's solving a
> >>
> > subtle
> >
> >> problem.
> >>
> >
> >
> >
>
http://codereview.chromium.**org/10911261/%3Chttp://codereview.chromium.org/1...>
> >

[satorux1, 2012-09-13 19:53:21]

Yes, that was a great suggestion. I'm just wondering how you magically appeared
to the code review. :)

[Mihai Parparita -not on Chrome, 2012-09-13 19:54:02]

On Thu, Sep 13, 2012 at 12:53 PM, <satorux@chromium.org> wrote:

> Yes, that was a great suggestion. I'm just wondering how you magically
> appeared
> to the code review. :)
>

I'm auto-cced for all extensions-related code reviews.

Mihai