Fix crashers in GetPrimaryDisplay

Fix crashers in GetPrimaryDisplay
- Store primary display info in global object as this will be accessed after Shell is deleted during shutdown. 
- Don't touch UI/aura/ash in HandleAppExitingForPlatform if Shell is not initialized yet. This can happen when XIOError occurs during startup.

BUG=150633, 150781
TEST=no crash report


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

[oshima, 2012-09-19 19:17:07]

Dan, can you review this?

[Daniel Erat, 2012-09-19 20:47:33]

lgtm

http://codereview.chromium.org/10952028/diff/2001/ash/display/display_control...
File ash/display/display_controller.h (right):

http://codereview.chromium.org/10952028/diff/2001/ash/display/display_control...
ash/display/display_controller.h:88: static const gfx::Display&
GetPrimaryDisplay();
nit: returning a reference to an internal pointer feels a bit strange.  could
you make this return const gfx::Display* or just a gfx::Display (since it looks
like this gets copied downstream) instead?

http://codereview.chromium.org/10952028/diff/2001/chrome/browser/lifetime/app...
File chrome/browser/lifetime/application_lifetime_aura.cc (right):

http://codereview.chromium.org/10952028/diff/2001/chrome/browser/lifetime/app...
chrome/browser/lifetime/application_lifetime_aura.cc:23: // This may be called
before ASH is initialized when XIOError is reported.
nit: s/ASH/Ash/ or just "ash"

[sky, 2012-09-19 20:49:28]

LGTM

[oshima, 2012-09-19 21:21:28]

http://codereview.chromium.org/10952028/diff/2001/ash/display/display_control...
File ash/display/display_controller.h (right):

http://codereview.chromium.org/10952028/diff/2001/ash/display/display_control...
ash/display/display_controller.h:88: static const gfx::Display&
GetPrimaryDisplay();
On 2012/09/19 20:47:33, Daniel Erat wrote:
> nit: returning a reference to an internal pointer feels a bit strange.  could
> you make this return const gfx::Display* or just a gfx::Display (since it
looks
> like this gets copied downstream) instead?


This pattern isn't uncommon in chrome (quick search of "  const\s[^\s]*&.*(" in
header shows a lot, although it doesn't necessarily mean it's ok just because
it's common :) 
and this won't cause real issue as this is global object which should outlive
UI.
Or are you worrying about because it may be deleted DisplayController's
constructor?

I'm happy to make this change as gfx::Display is treated as POD, but want to
understand
your concern before making the change.

http://codereview.chromium.org/10952028/diff/2001/chrome/browser/lifetime/app...
File chrome/browser/lifetime/application_lifetime_aura.cc (right):

http://codereview.chromium.org/10952028/diff/2001/chrome/browser/lifetime/app...
chrome/browser/lifetime/application_lifetime_aura.cc:23: // This may be called
before ASH is initialized when XIOError is reported.
On 2012/09/19 20:47:33, Daniel Erat wrote:
> nit: s/ASH/Ash/ or just "ash"

changed to |ash::Shell|

[Daniel Erat, 2012-09-19 21:31:47]

http://codereview.chromium.org/10952028/diff/2001/ash/display/display_control...
File ash/display/display_controller.h (right):

http://codereview.chromium.org/10952028/diff/2001/ash/display/display_control...
ash/display/display_controller.h:88: static const gfx::Display&
GetPrimaryDisplay();
On 2012/09/19 21:21:28, oshima wrote:
> On 2012/09/19 20:47:33, Daniel Erat wrote:
> > nit: returning a reference to an internal pointer feels a bit strange. 
could
> > you make this return const gfx::Display* or just a gfx::Display (since it
> looks
> > like this gets copied downstream) instead?
> 
> 
> This pattern isn't uncommon in chrome (quick search of "  const\s[^\s]*&.*("
in
> header shows a lot, although it doesn't necessarily mean it's ok just because
> it's common :) 
> and this won't cause real issue as this is global object which should outlive
> UI.
> Or are you worrying about because it may be deleted DisplayController's
> constructor?
> 
> I'm happy to make this change as gfx::Display is treated as POD, but want to
> understand
> your concern before making the change.

Accessors that return const references are super-common for members that are
embedded within the class (since you know that the reference will be valid for
the lifetime of the object), but seem very rare for pointer members -- I only
see six files when I grep for 'const\s[^\s]*&.*(.*return \*'.  It makes me
nervous since it's possible for the reference to become invalid while the object
is still alive, if the object's pointer gets updated to point somewhere else.

[oshima, 2012-09-19 21:43:11]

On Wed, Sep 19, 2012 at 2:31 PM, <derat@chromium.org> wrote:

>
> http://codereview.chromium.**org/10952028/diff/2001/ash/**
>
display/display_controller.h<http://codereview.chromium.org/10952028/diff/2001/ash/display/display_controller.h>
> File ash/display/display_**controller.h (right):
>
> http://codereview.chromium.**org/10952028/diff/2001/ash/**
>
display/display_controller.h#**newcode88<http://codereview.chromium.org/10952028/diff/2001/ash/display/display_controller.h#newcode88>
> ash/display/display_**controller.h:88: static const gfx::Display&
> GetPrimaryDisplay();
> On 2012/09/19 21:21:28, oshima wrote:
>
>> On 2012/09/19 20:47:33, Daniel Erat wrote:
>> > nit: returning a reference to an internal pointer feels a bit
>>
> strange.  could
>
>> > you make this return const gfx::Display* or just a gfx::Display
>>
> (since it
>
>> looks
>> > like this gets copied downstream) instead?
>>
>
>
>  This pattern isn't uncommon in chrome (quick search of "
>>
> const\s[^\s]*&.*(" in
>
>> header shows a lot, although it doesn't necessarily mean it's ok just
>>
> because
>
>> it's common :)
>> and this won't cause real issue as this is global object which should
>>
> outlive
>
>> UI.
>> Or are you worrying about because it may be deleted
>>
> DisplayController's
>
>> constructor?
>>
>
>  I'm happy to make this change as gfx::Display is treated as POD, but
>>
> want to
>
>> understand
>> your concern before making the change.
>>
>
> Accessors that return const references are super-common for members that
> are embedded within the class (since you know that the reference will be
> valid for the lifetime of the object), but seem very rare for pointer
> members -- I only see six files when I grep for
> 'const\s[^\s]*&.*(.*return \*'.  It makes me nervous since it's possible
> for the reference to become invalid while the object is still alive, if
> the object's pointer gets updated to point somewhere else.
>

I made it pointer because it's global but it's lifetime is global (except
in test)
but I see your point. Will change.

- oshima



>
>
http://codereview.chromium.**org/10952028/<http://codereview.chromium.org/109...
>

[commit-bot: I haz the power, 2012-09-19 22:00:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/10952028/3006

[commit-bot: I haz the power, 2012-09-20 00:48:57]

Change committed as 157671