Fixes smart deploy causing crash on shutdown.

Fixes smart deploy causing crash on shutdown.

Moves the destruction of the Virtual Keyboard Controller before the
Maximized Mode Controller so that re-enabling the internal keyboard
when leaving touchview due to shutdown does not cause the on-screen
keyboard to change state.

BUG=446204
TEST=VirtualKeyboardControllerTest.RestoreKeyboardDevices

Committed: https://crrev.com/bbaf2c83b69fe1e0881809f2c0b7a66606d6a15a
Cr-Commit-Position: refs/heads/master@{#310316}

[rsadam, 2015-01-05 20:44:18]

rsadam@chromium.org changed reviewers:
+ sky@chromium.org

[rsadam, 2015-01-05 20:44:19]

Hi Sky, PTAL!

[sky, 2015-01-06 00:32:23]

How about some test coverage?

[rsadam, 2015-01-06 16:15:43]

On 2015/01/06 00:32:23, sky wrote:
> How about some test coverage?

Hi Sky, it's not obvious to me how to approach testing this, I was wondering if
you had any suggestions.

The crash is caused by the following chain of events:
When rotating the hinge on an actual device, the screen rotation causes the
device to enter maximized mode and creates an event blocker
(ScopedDisableInternalMouseAndKeyboard) which disables the internal keyboard.
When the device is powered off, the MaximizedModeController destructor destroys
the event blocker which re-enables the keyboard, which in turn causes the
VirtualKeyboardCOntroller to be notified of the device list changing. The
VirtualKeyboardController depends on the MaximizedModeController and tries to
access a resource that's already been deleted causing the device to crash. This
patch causes the virtual keyboard controller to be destroyed first, so it's no
longer listening when the event blocker re-enables the internal keyboard.

We only create the event blocker on hinge rotations, and there don't seem to be
any test methods that emulate this behavior. If my understanding of AshTestBase
is correct, the shell will be destroyed and ash's destructor called when the
test ends, so getting the test to the same state as above should be sufficient
(i.e the test would crash without this patch, and pass with it), the part I'm
confused about is how to get it in to that state.

Would it make sense to create a MockScopedDisableInternalMouseAndKeyboard which
can we manipulate as we want, and a InstallEventBlockerForTesting method in
MaximizedModeController? Or is there a simpler approach I'm missing?

[sky, 2015-01-06 16:54:19]

I'm not familiar with the rotation code. I would try Oshima or Stefan.
Hopefully they can point you in the right direction for triggering the
code in a test.

  -Scott

On Tue, Jan 6, 2015 at 8:15 AM,  <rsadam@chromium.org> wrote:
> On 2015/01/06 00:32:23, sky wrote:
>>
>> How about some test coverage?
>
>
> Hi Sky, it's not obvious to me how to approach testing this, I was wondering
> if
> you had any suggestions.
>
> The crash is caused by the following chain of events:
> When rotating the hinge on an actual device, the screen rotation causes the
> device to enter maximized mode and creates an event blocker
> (ScopedDisableInternalMouseAndKeyboard) which disables the internal
> keyboard.
> When the device is powered off, the MaximizedModeController destructor
> destroys
> the event blocker which re-enables the keyboard, which in turn causes the
> VirtualKeyboardCOntroller to be notified of the device list changing. The
> VirtualKeyboardController depends on the MaximizedModeController and tries
> to
> access a resource that's already been deleted causing the device to crash.
> This
> patch causes the virtual keyboard controller to be destroyed first, so it's
> no
> longer listening when the event blocker re-enables the internal keyboard.
>
> We only create the event blocker on hinge rotations, and there don't seem to
> be
> any test methods that emulate this behavior. If my understanding of
> AshTestBase
> is correct, the shell will be destroyed and ash's destructor called when the
> test ends, so getting the test to the same state as above should be
> sufficient
> (i.e the test would crash without this patch, and pass with it), the part
> I'm
> confused about is how to get it in to that state.
>
> Would it make sense to create a MockScopedDisableInternalMouseAndKeyboard
> which
> can we manipulate as we want, and a InstallEventBlockerForTesting method in
> MaximizedModeController? Or is there a simpler approach I'm missing?
>
> https://codereview.chromium.org/834163002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rsadam, 2015-01-06 16:57:20]

rsadam@chromium.org changed reviewers:
+ skuhne@chromium.org

[rsadam, 2015-01-06 16:57:20]

+skuhne

[rsadam, 2015-01-06 18:12:02]

I coded up the approach I initially described, and tested that it fails without
my patch, and passes with it. 

@Sky, how does this look?

[sky, 2015-01-06 22:34:04]

https://codereview.chromium.org/834163002/diff/40001/ash/virtual_keyboard_con...
File ash/virtual_keyboard_controller_unittest.cc (right):

https://codereview.chromium.org/834163002/diff/40001/ash/virtual_keyboard_con...
ash/virtual_keyboard_controller_unittest.cc:76: ~MockEventBlocker() {
override

https://codereview.chromium.org/834163002/diff/40001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/834163002/diff/40001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:139: friend class
MaximizeModeControllerTest;
Friend your test so you don't have to expose a method (just like the other tests
are doing).

[Mr4D (OOO till 08-26), 2015-01-06 22:46:48]

lgtm for the change after the changes requested by sky.

For the sake of completeness: flackr added the rotation code, so he might have
an additional comment.

[rsadam, 2015-01-06 22:50:55]

On 2015/01/06 22:46:48, Mr4D wrote:
> lgtm for the change after the changes requested by sky.

Will address these presently!

> For the sake of completeness: flackr added the rotation code, so he might have
> an additional comment.

flackr is on vacation until after branch point, and this is a release blocker,
would it make sense to land the fix and then return to it when he's back?

[rsadam, 2015-01-06 23:18:41]

https://codereview.chromium.org/834163002/diff/40001/ash/virtual_keyboard_con...
File ash/virtual_keyboard_controller_unittest.cc (right):

https://codereview.chromium.org/834163002/diff/40001/ash/virtual_keyboard_con...
ash/virtual_keyboard_controller_unittest.cc:76: ~MockEventBlocker() {
On 2015/01/06 22:34:04, sky wrote:
> override

Done.

https://codereview.chromium.org/834163002/diff/40001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/834163002/diff/40001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:139: friend class
MaximizeModeControllerTest;
On 2015/01/06 22:34:04, sky wrote:
> Friend your test so you don't have to expose a method (just like the other
tests
> are doing).

Done.

[sky, 2015-01-07 16:01:53]

https://codereview.chromium.org/834163002/diff/60001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/834163002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:146: void
InstallEventBlockerForTest(
Not that your test is a friend you don't need this function, right? You can
manipulate event_blocker_ directly, right?

[rsadam, 2015-01-07 16:04:32]

https://codereview.chromium.org/834163002/diff/60001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/834163002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:146: void
InstallEventBlockerForTest(
On 2015/01/07 16:01:52, sky wrote:
> Not that your test is a friend you don't need this function, right? You can
> manipulate event_blocker_ directly, right?

That was my first approach, however I got errors that the method I was accessing
was private. This led me to believe that even though the test class was a
friend, the test instances themselves were not. Is this not the case?

[rsadam, 2015-01-07 16:05:49]

On 2015/01/07 16:04:32, rsadam wrote:
>
https://codereview.chromium.org/834163002/diff/60001/ash/wm/maximize_mode/max...
> File ash/wm/maximize_mode/maximize_mode_controller.h (right):
> 
>
https://codereview.chromium.org/834163002/diff/60001/ash/wm/maximize_mode/max...
> ash/wm/maximize_mode/maximize_mode_controller.h:146: void
> InstallEventBlockerForTest(
> On 2015/01/07 16:01:52, sky wrote:
> > Not that your test is a friend you don't need this function, right? You can
> > manipulate event_blocker_ directly, right?
> 
> That was my first approach, however I got errors that the method I was
accessing
> was private. This led me to believe that even though the test class was a
> friend, the test instances themselves were not. Is this not the case?

Oh wait, I misunderstood your comment. You meant in maximize mode. Yeah you're
right, I'll fix that!

[rsadam, 2015-01-07 16:13:02]

https://codereview.chromium.org/834163002/diff/60001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/834163002/diff/60001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:146: void
InstallEventBlockerForTest(
On 2015/01/07 16:01:52, sky wrote:
> Not that your test is a friend you don't need this function, right? You can
> manipulate event_blocker_ directly, right?

Done.

[sky, 2015-01-07 17:10:09]

LGTM

[rsadam, 2015-01-07 17:22:44]

The CQ bit was checked by rsadam@chromium.org

[commit-bot: I haz the power, 2015-01-07 17:23:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/834163002/80001

[commit-bot: I haz the power, 2015-01-07 17:54:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-07 17:56:23]

Patchset 5 (id:??) landed as
https://crrev.com/bbaf2c83b69fe1e0881809f2c0b7a66606d6a15a
Cr-Commit-Position: refs/heads/master@{#310316}