Fix crash when locking screen while adding a user to an existing session.

Fix crash when locking screen while adding a user to an existing session.

Also added a test to prevent a regression.

Essentially, ScreenLocker::HandleLockScreenRequest calls UserAddingScreen::Cancel and uses a callback to be notified for when the user-adding screen is completed. The crash happened because this callback was invoked too early - before the adding screen has been fully deinitialized.

BUG=467111
Committed: https://crrev.com/87e397cc8cce3e680b9a54340949dcda5d3e08eb
Cr-Commit-Position: refs/heads/master@{#351154}

[jdufault, 2015-09-21 20:39:48]

Patchset #2 (id:20001) has been deleted

[jdufault, 2015-09-21 20:42:11]

jdufault@chromium.org changed reviewers:
+ achuith@chromium.org, dzhioev@chromium.org

[jdufault, 2015-09-21 20:42:12]

Pavel, Achuith, would you please take a look? Thanks!

[achuithb, 2015-09-21 21:25:43]

lgtm

[dzhioev (left Google), 2015-09-24 23:51:17]

https://codereview.chromium.org/1356713004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/1356713004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:919:
base::MessageLoop::current()->PostTask(FROM_HERE, completion_callback_);
Can you please explain, how this helps to prevent the crash? (and at which point
of locking process ShutdownDisplayHost() is invoked?)

[jdufault, 2015-09-26 00:35:11]

On 2015/09/24 23:51:17, dzhioev wrote:
>
https://codereview.chromium.org/1356713004/diff/40001/chrome/browser/chromeos...
> File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1356713004/diff/40001/chrome/browser/chromeos...
> chrome/browser/chromeos/login/ui/login_display_host_impl.cc:919:
> base::MessageLoop::current()->PostTask(FROM_HERE, completion_callback_);
> Can you please explain, how this helps to prevent the crash? (and at which
point
> of locking process ShutdownDisplayHost() is invoked?)

Ultimately, the crash is happening because of some initialization order issues.
A DCHECK is getting triggered in SignInScreenController - two instances of it
are getting created at the same time; the contract for the object requires that
there only ever be one object.

In the lock screen, when we press ctrl-L, we end up calling
ScreenLocker::HandleLockScreenRequest. This notices that the UserAddingScreen is
active so it attempts to cancel it. HandleLockScreenRequest registers an
observer (which simply reinvokes HandleLockScreenRequest) on UserAddingScreen
that gets called when the adding screen is dismissed.

UserAddingScreen takes the callback and forwards it to LoginDisplayHostImpl,
which actually invokes it. The issue is that the callback is invoked before
SignInScreenController is deinitialized; HandleLockScreenRequest will cause an
instance of SignInScreenController to be initialized.

So the SignInScreenController flow looks like:

- Allocate from UserAddScreen
- Allocate from ScreenLocker
- crash

We want it to look like:

- Allocate from UserAddScreen
- Deallocate
- Allocate from ScreenLocker
- ...
- Deallocate

Another option is to Post a new callback from UserAddScreen instead of from
LoginDisplayHostImpl, ie, call the following line in a callback instead of
directly:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

[dzhioev (left Google), 2015-09-28 20:18:58]

On 2015/09/26 00:35:11, jdufault wrote:
> On 2015/09/24 23:51:17, dzhioev wrote:
> >
>
https://codereview.chromium.org/1356713004/diff/40001/chrome/browser/chromeos...
> > File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1356713004/diff/40001/chrome/browser/chromeos...
> > chrome/browser/chromeos/login/ui/login_display_host_impl.cc:919:
> > base::MessageLoop::current()->PostTask(FROM_HERE, completion_callback_);
> > Can you please explain, how this helps to prevent the crash? (and at which
> point
> > of locking process ShutdownDisplayHost() is invoked?)
> 
> Ultimately, the crash is happening because of some initialization order
issues.
> A DCHECK is getting triggered in SignInScreenController - two instances of it
> are getting created at the same time; the contract for the object requires
that
> there only ever be one object.
> 
> In the lock screen, when we press ctrl-L, we end up calling
> ScreenLocker::HandleLockScreenRequest. This notices that the UserAddingScreen
is
> active so it attempts to cancel it. HandleLockScreenRequest registers an
> observer (which simply reinvokes HandleLockScreenRequest) on UserAddingScreen
> that gets called when the adding screen is dismissed.
> 
> UserAddingScreen takes the callback and forwards it to LoginDisplayHostImpl,
> which actually invokes it. The issue is that the callback is invoked before
> SignInScreenController is deinitialized; HandleLockScreenRequest will cause an
> instance of SignInScreenController to be initialized.
> 
> So the SignInScreenController flow looks like:
> 
> - Allocate from UserAddScreen
> - Allocate from ScreenLocker
> - crash
> 
> We want it to look like:
> 
> - Allocate from UserAddScreen
> - Deallocate
> - Allocate from ScreenLocker
> - ...
> - Deallocate
> 
> Another option is to Post a new callback from UserAddScreen instead of from
> LoginDisplayHostImpl, ie, call the following line in a callback instead of
> directly:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

Thank you for the explanation!
LGTM

[dzhioev (left Google), 2015-09-28 20:18:59]

On 2015/09/26 00:35:11, jdufault wrote:
> On 2015/09/24 23:51:17, dzhioev wrote:
> >
>
https://codereview.chromium.org/1356713004/diff/40001/chrome/browser/chromeos...
> > File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1356713004/diff/40001/chrome/browser/chromeos...
> > chrome/browser/chromeos/login/ui/login_display_host_impl.cc:919:
> > base::MessageLoop::current()->PostTask(FROM_HERE, completion_callback_);
> > Can you please explain, how this helps to prevent the crash? (and at which
> point
> > of locking process ShutdownDisplayHost() is invoked?)
> 
> Ultimately, the crash is happening because of some initialization order
issues.
> A DCHECK is getting triggered in SignInScreenController - two instances of it
> are getting created at the same time; the contract for the object requires
that
> there only ever be one object.
> 
> In the lock screen, when we press ctrl-L, we end up calling
> ScreenLocker::HandleLockScreenRequest. This notices that the UserAddingScreen
is
> active so it attempts to cancel it. HandleLockScreenRequest registers an
> observer (which simply reinvokes HandleLockScreenRequest) on UserAddingScreen
> that gets called when the adding screen is dismissed.
> 
> UserAddingScreen takes the callback and forwards it to LoginDisplayHostImpl,
> which actually invokes it. The issue is that the callback is invoked before
> SignInScreenController is deinitialized; HandleLockScreenRequest will cause an
> instance of SignInScreenController to be initialized.
> 
> So the SignInScreenController flow looks like:
> 
> - Allocate from UserAddScreen
> - Allocate from ScreenLocker
> - crash
> 
> We want it to look like:
> 
> - Allocate from UserAddScreen
> - Deallocate
> - Allocate from ScreenLocker
> - ...
> - Deallocate
> 
> Another option is to Post a new callback from UserAddScreen instead of from
> LoginDisplayHostImpl, ie, call the following line in a callback instead of
> directly:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

Thank you for the explanation!
LGTM

[jdufault, 2015-09-28 20:39:46]

The CQ bit was checked by jdufault@chromium.org

[commit-bot: I haz the power, 2015-09-28 20:42:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1356713004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1356713004/40001

[commit-bot: I haz the power, 2015-09-28 21:14:54]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-09-28 21:16:38]

Patchset 2 (id:??) landed as
https://crrev.com/87e397cc8cce3e680b9a54340949dcda5d3e08eb
Cr-Commit-Position: refs/heads/master@{#351154}