Open Ash shell after creating Profile

Open Ash shell after creating Profile


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

[stevenjb, 2013-05-23 18:07:13]

We are seeing tcmalloc corruption sometimes in interactive_ui_tests from
GetDefaultProfile().

It turns out that GetDefaultProfile() is getting called from a few places during
Ash Shell initialization. A couple of these would be difficult to fix, and it
seems more correct to initialize the Ash Shell after creating the default
profile anyway. I moved the OpenAsh call and fixed a couple of dependent
initializers. Inspection suggested that should all be OK, and so far running
Chrome seems fine. Hopefully the tests will confirm that.

[sail, 2013-05-23 18:20:55]

https://codereview.chromium.org/15702004/diff/4001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/15702004/diff/4001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_manager.cc:234: // Assume "allowed" for tests;
SetAllowGetDefaultProfile(false) should be called
new line before

https://codereview.chromium.org/15702004/diff/4001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_manager.cc:237: }
new line before

https://codereview.chromium.org/15702004/diff/4001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_manager.cc:247: LOG_IF(FATAL,
!s_allow_get_default_profile)
can we make this a CHECK() instead?

https://codereview.chromium.org/15702004/diff/4001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/15702004/diff/4001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_manager.h:57: // DEPRECATED: Temporary measure
to ensure that GetDefaultProfile() is not
can you add a bug # that tracks removing this?

[stevenjb, 2013-05-23 18:28:15]

https://codereview.chromium.org/15702004/diff/4001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/15702004/diff/4001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_manager.cc:234: // Assume "allowed" for tests;
SetAllowGetDefaultProfile(false) should be called
On 2013/05/23 18:20:56, sail wrote:
> new line before

Done.

https://codereview.chromium.org/15702004/diff/4001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_manager.cc:237: }
On 2013/05/23 18:20:56, sail wrote:
> new line before

Done.

https://codereview.chromium.org/15702004/diff/4001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_manager.cc:247: LOG_IF(FATAL,
!s_allow_get_default_profile)
On 2013/05/23 18:20:56, sail wrote:
> can we make this a CHECK() instead?

Done.

https://codereview.chromium.org/15702004/diff/4001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/15702004/diff/4001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_manager.h:57: // DEPRECATED: Temporary measure
to ensure that GetDefaultProfile() is not
On 2013/05/23 18:20:56, sail wrote:
> can you add a bug # that tracks removing this?

Done. (Added above the first DEPRECATED function)

[sail, 2013-05-23 18:59:30]

profiles/* LGTM

[Paweł Hajdan Jr., 2013-05-23 20:33:24]

Feel free to split the patch into parts:

One addressing the immediate issue, and the other addressing catching this issue
and my comment below.

https://codereview.chromium.org/15702004/diff/11001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/15702004/diff/11001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:235: // Assume "allowed" for tests;
SetAllowGetDefaultProfile(false) should be called
Let's make it stronger: false should be the default.

Feel free to make some test utility for allowing this.

[stevenjb, 2013-05-23 20:42:28]

+davemoore@ for ChromeBrowserMainExtraPartsAsh (wrt TabScrubber)
sky@ for OWNER

[sky, 2013-05-23 21:34:12]

LGTM. Ordering dependencies are easy to change and introduce crashes like this
without knowing it. If possible, I encourage you to write a test.

[stevenjb, 2013-05-23 21:48:59]

On 2013/05/23 21:34:12, sky wrote:
> LGTM. Ordering dependencies are easy to change and introduce crashes like this
> without knowing it. If possible, I encourage you to write a test.

Heh, we have lots of tests that fail when order dependencies change, alas few of
them are explicit tests for that.

I have a follow-up CL that adds a CHECK to GetDefaultProfile():
https://codereview.chromium.org/15695010/ which will at least prevent this
particular dependency from poking up its ugly head again.

[commit-bot: I haz the power, 2013-05-24 03:15:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/15702004/31001

[commit-bot: I haz the power, 2013-05-24 05:29:49]

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

[commit-bot: I haz the power, 2013-05-24 09:41:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/15702004/31001

[commit-bot: I haz the power, 2013-05-24 12:25:14]

Change committed as 202066

[Nikita (slow), 2013-05-27 12:51:03]

lgtm