Start session fail causes restart chrome

Start session fail causes restart chrome

BUG=244628
TEST=manual check on Acer C720

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

[ygorshenin1, 2014-04-10 15:39:21]

https://codereview.chromium.org/228703004/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/228703004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:203: void onSessionStarted(const
UserContext& user_context,
Functions should start with a capital letter
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)

https://codereview.chromium.org/228703004/diff/1/chromeos/dbus/session_manage...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/228703004/diff/1/chromeos/dbus/session_manage...
chromeos/dbus/session_manager_client.cc:340: LOG(ERROR)
nit: move '<< "Failed to call "' to the previous line.

https://codereview.chromium.org/228703004/diff/1/chromeos/dbus/session_manage...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/228703004/diff/1/chromeos/dbus/session_manage...
chromeos/dbus/session_manager_client.h:73: typedef base::Callback<void(bool
success)> StartSessionCallback;
nit: add an empty line between typedef and following comment.

[Nikita (slow), 2014-04-10 15:41:12]

https://codereview.chromium.org/228703004/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/228703004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:424: const std::string&
display_email,
nit: Alignment should be fixed.

[Roman Sorokin (ftl), 2014-04-11 07:38:12]

PTAL

https://codereview.chromium.org/228703004/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/228703004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:203: void onSessionStarted(const
UserContext& user_context,
On 2014/04/10 15:39:22, ygorshenin1 wrote:
> Functions should start with a capital letter
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)

Done.

https://codereview.chromium.org/228703004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:424: const std::string&
display_email,
On 2014/04/10 15:41:13, Nikita Kostylev wrote:
> nit: Alignment should be fixed.

Done.

https://codereview.chromium.org/228703004/diff/1/chromeos/dbus/session_manage...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/228703004/diff/1/chromeos/dbus/session_manage...
chromeos/dbus/session_manager_client.cc:340: LOG(ERROR)
On 2014/04/10 15:39:22, ygorshenin1 wrote:
> nit: move '<< "Failed to call "' to the previous line.

Done.

https://codereview.chromium.org/228703004/diff/1/chromeos/dbus/session_manage...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/228703004/diff/1/chromeos/dbus/session_manage...
chromeos/dbus/session_manager_client.h:73: typedef base::Callback<void(bool
success)> StartSessionCallback;
On 2014/04/10 15:39:22, ygorshenin1 wrote:
> nit: add an empty line between typedef and following comment.

Done.

[ygorshenin1, 2014-04-11 07:46:03]

Looks like you also need to fix FakeSessionManagerClient (from linux_chromeos
trybot:
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...).

[Roman Sorokin (ftl), 2014-04-16 15:31:06]

PTAL

[ygorshenin1, 2014-04-16 16:50:04]

lgtm with nit.

https://codereview.chromium.org/228703004/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/crash_restore_browsertest.cc (right):

https://codereview.chromium.org/228703004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/crash_restore_browsertest.cc:72: size_t
session_started_cnt_;
nit: no need to use size_t here, use int instead. Also, use "count" instead of
"cnt".

[Nikita (slow), 2014-04-18 05:22:58]

lgtm when these comments are addressed

https://codereview.chromium.org/228703004/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/crash_restore_browsertest.cc (right):

https://codereview.chromium.org/228703004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/crash_restore_browsertest.cc:72: size_t
session_started_cnt_;
nit: Please rename

either num_session_started_ or
session_started_count_

https://codereview.chromium.org/228703004/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/228703004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.cc:202: // Callback to resume profile
preparing after start session
nit: Add dot at the end, otherwise comment looks like it is unfinished.

https://codereview.chromium.org/228703004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.cc:209: // Complete profile
preparation with having active session
nit: same here.

https://codereview.chromium.org/228703004/diff/80001/chromeos/dbus/fake_sessi...
File chromeos/dbus/fake_session_manager_client.cc (right):

https://codereview.chromium.org/228703004/diff/80001/chromeos/dbus/fake_sessi...
chromeos/dbus/fake_session_manager_client.cc:57: if (loop)
nit: Add comment why such check is needed.

https://codereview.chromium.org/228703004/diff/80001/chromeos/dbus/session_ma...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/228703004/diff/80001/chromeos/dbus/session_ma...
chromeos/dbus/session_manager_client.cc:345: if (!reader.PopBool(&success))
I think response also has error string, please put it to the logs as well.

[Roman Sorokin (ftl), 2014-04-22 08:28:46]

+oshima for chromeos/*

https://codereview.chromium.org/228703004/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/crash_restore_browsertest.cc (right):

https://codereview.chromium.org/228703004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/crash_restore_browsertest.cc:72: size_t
session_started_cnt_;
On 2014/04/16 16:50:04, ygorshenin1 wrote:
> nit: no need to use size_t here, use int instead. Also, use "count" instead of
> "cnt".

Done.

https://codereview.chromium.org/228703004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/crash_restore_browsertest.cc:72: size_t
session_started_cnt_;
On 2014/04/16 16:50:04, ygorshenin1 wrote:
> nit: no need to use size_t here, use int instead. Also, use "count" instead of
> "cnt".

Done.

https://codereview.chromium.org/228703004/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/228703004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.cc:202: // Callback to resume profile
preparing after start session
On 2014/04/18 05:22:59, Nikita Kostylev wrote:
> nit: Add dot at the end, otherwise comment looks like it is unfinished.

Done.

https://codereview.chromium.org/228703004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.cc:209: // Complete profile
preparation with having active session
On 2014/04/18 05:22:59, Nikita Kostylev wrote:
> nit: same here.

Done.

https://codereview.chromium.org/228703004/diff/80001/chromeos/dbus/fake_sessi...
File chromeos/dbus/fake_session_manager_client.cc (right):

https://codereview.chromium.org/228703004/diff/80001/chromeos/dbus/fake_sessi...
chromeos/dbus/fake_session_manager_client.cc:57: if (loop)
On 2014/04/18 05:22:59, Nikita Kostylev wrote:
> nit: Add comment why such check is needed.

Done.

https://codereview.chromium.org/228703004/diff/80001/chromeos/dbus/session_ma...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/228703004/diff/80001/chromeos/dbus/session_ma...
chromeos/dbus/session_manager_client.cc:345: if (!reader.PopBool(&success))
On 2014/04/18 05:22:59, Nikita Kostylev wrote:
> I think response also has error string, please put it to the logs as well.

Error string from dbus was logged in object_proxy

[oshima, 2014-04-22 10:03:32]

https://codereview.chromium.org/228703004/diff/100001/chromeos/dbus/fake_sess...
File chromeos/dbus/fake_session_manager_client.cc (right):

https://codereview.chromium.org/228703004/diff/100001/chromeos/dbus/fake_sess...
chromeos/dbus/fake_session_manager_client.cc:58: // MessageLoop::current()
returns NULL.
Isn't it possible to create a message lop in such test instead of adding a code
for test?

[Roman Sorokin (ftl), 2014-04-24 11:49:25]

PTAL

https://codereview.chromium.org/228703004/diff/100001/chromeos/dbus/fake_sess...
File chromeos/dbus/fake_session_manager_client.cc (right):

https://codereview.chromium.org/228703004/diff/100001/chromeos/dbus/fake_sess...
chromeos/dbus/fake_session_manager_client.cc:58: // MessageLoop::current()
returns NULL.
On 2014/04/22 10:03:32, oshima wrote:
> Isn't it possible to create a message lop in such test instead of adding a
code
> for test?

Yes, it is. In this browser test we need to start sessions before browser start.
So I would create message loop for processing callbacks from session manager

[oshima, 2014-04-24 13:59:30]

lgtm with nits

https://codereview.chromium.org/228703004/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/crash_restore_browsertest.cc (right):

https://codereview.chromium.org/228703004/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/login/crash_restore_browsertest.cc:57: //session_manager
// We need to ...
// session_manager
(space after //)

https://codereview.chromium.org/228703004/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/login/crash_restore_browsertest.cc:58:
msg_loop_.reset(new base::MessageLoop);
you can just create message look on stack.

[Roman Sorokin (ftl), 2014-04-28 12:26:25]

The CQ bit was checked by rsorokin@chromium.org

[Roman Sorokin (ftl), 2014-04-28 12:26:29]

The CQ bit was unchecked by rsorokin@chromium.org

[Roman Sorokin (ftl), 2014-04-28 15:31:14]

to CQ

https://codereview.chromium.org/228703004/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/crash_restore_browsertest.cc (right):

https://codereview.chromium.org/228703004/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/login/crash_restore_browsertest.cc:57: //session_manager
On 2014/04/24 13:59:31, oshima wrote:
> // We need to ...
> // session_manager
> (space after //)

Done.

https://codereview.chromium.org/228703004/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/login/crash_restore_browsertest.cc:58:
msg_loop_.reset(new base::MessageLoop);
On 2014/04/24 13:59:31, oshima wrote:
> you can just create message look on stack.

Done.

[Roman Sorokin (ftl), 2014-04-28 15:31:18]

The CQ bit was checked by rsorokin@chromium.org

[commit-bot: I haz the power, 2014-04-28 15:31:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsorokin@chromium.org/228703004/180001

[commit-bot: I haz the power, 2014-04-28 16:45:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 16:45:55]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[Roman Sorokin (ftl), 2014-04-29 07:57:27]

The CQ bit was checked by rsorokin@chromium.org

[commit-bot: I haz the power, 2014-04-29 07:59:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsorokin@chromium.org/228703004/180001

[commit-bot: I haz the power, 2014-04-29 09:01:37]

Change committed as 266791

[Nikita (slow), 2014-05-06 12:51:22]

A revert of this CL has been created in
https://codereview.chromium.org/268293002/ by nkostylev@chromium.org.

The reason for reverting is: Seems to be breaking user sign in.
See also http://crbug.com/369040.