[M58] exo: Fix multi-display cursor crash

[M58] exo: Fix multi-display cursor crash

The cursor surface is parented to the primary root window, which may
change when using multiple displays. This CL fixes a crash caused by
removing the child from the primary root window when the parent is a
different root window or the child is orphaned.

BUG=714832
TEST=No crashes after changing primary display.

[Dominik Laskowski, 2017-04-27 06:11:16]

domlaskowski@chromium.org changed reviewers:
+ oshima@chromium.org, reveman@chromium.org

[Dominik Laskowski, 2017-04-27 06:11:17]

PTAL.

[reveman, 2017-04-27 13:19:23]

Thanks. Please split the two fixes into two patches. Descriptions that make it
clear why previous code was wrong is also needed before we can land this.

[Dominik Laskowski, 2017-04-27 14:30:13]

Description was changed from

==========
[M58] exo: Fix multi-display regressions

Multi-display support was punted to M59. This CL is a workaround for two
regressions that affect M58:

  1) Cursor-related crashes after changing the primary display.
  2) Windows getting stuck when dragged beyond the primary display.

BUG=714832
TEST=Manual on Caroline M58.
==========

to

==========
[M58] exo: Fix multi-display cursor

The cursor surface was being parented to the target root window, which
can change when using multiple displays.

Since windows are confined to the primary display in M58, this CL uses
the primary root window as the parent of the surface, and only removes
the child if not orphaned.

BUG=714832
TEST=Manual on Caroline M58.
==========

[Dominik Laskowski, 2017-04-27 14:31:00]

On 2017/04/27 13:19:23, reveman wrote:
> Thanks. Please split the two fixes into two patches. Descriptions that make it
> clear why previous code was wrong is also needed before we can land this.

Done.

[reveman, 2017-04-27 15:55:05]

Are non-arc windows really confined to primary display in M58? What's the plan
for fixing this in ToT? Common practice is to always fix issues in ToT before
fixing anything on a branch.

https://codereview.chromium.org/2847563003/diff/20001/components/exo/pointer.cc
File components/exo/pointer.cc (right):

https://codereview.chromium.org/2847563003/diff/20001/components/exo/pointer....
components/exo/pointer.cc:92: if (surface_->window()->parent())
How can surface_->window()->parent() be null? Can we DCHECK instead?

[Dominik Laskowski, 2017-04-27 17:02:05]

Right, a limitation of this workaround is that cursor scale might be incorrect
for non-ARC windows, but that was already broken for other reasons. The proper
fix for ToT is [1], but it's large and doesn't merge cleanly.

[1] https://codereview.chromium.org/2780623002/

https://codereview.chromium.org/2847563003/diff/20001/components/exo/pointer.cc
File components/exo/pointer.cc (right):

https://codereview.chromium.org/2847563003/diff/20001/components/exo/pointer....
components/exo/pointer.cc:92: if (surface_->window()->parent())
On 2017/04/27 15:55:04, reveman wrote:
> How can surface_->window()->parent() be null? Can we DCHECK instead?

If the primary display is removed.

[reveman, 2017-04-28 22:09:58]

Can we land this in ToT first?

[Dominik Laskowski, 2017-04-29 01:36:12]

On 2017/04/28 22:09:58, reveman wrote:
> Can we land this in ToT first?

Ditto. Very low risk, i.e. code is equivalent for single display.

[elijahtaylor1, 2017-05-02 17:51:27]

elijahtaylor@chromium.org changed reviewers:
+ elijahtaylor@chromium.org

[elijahtaylor1, 2017-05-02 17:51:27]

This seems simple enough, if the change isn't compatible with M59 or ToT then
I'm comfortable making it on the branch.  Can we land this this week in M58? 
It's a top crasher for caroline.

[Dominik Laskowski, 2017-05-03 15:24:09]

I guess it doesn't hurt to land this on ToT, other than a bit of churn rebasing
the proper fix. Will upload a patch shortly.

[Dominik Laskowski, 2017-05-03 15:40:45]

Never mind. It doesn't merge cleanly, and depends on the other M58 CL:

https://codereview.chromium.org/2847763002/

[Dominik Laskowski, 2017-05-04 21:31:54]

Description was changed from

==========
[M58] exo: Fix multi-display cursor

The cursor surface was being parented to the target root window, which
can change when using multiple displays.

Since windows are confined to the primary display in M58, this CL uses
the primary root window as the parent of the surface, and only removes
the child if not orphaned.

BUG=714832
TEST=Manual on Caroline M58.
==========

to

==========
[M58] exo: Fix multi-display cursor crash

The cursor surface is parented to the primary root window, which may
change when using multiple displays. This CL fixes a crash caused by
removing the child from the primary root window when the parent is a
different root window or the child is orphaned.

BUG=714832
TEST=No crashes after changing primary display.
==========

[Dominik Laskowski, 2017-05-04 22:13:47]

Closing. Will cherry-pick from ToT instead.