ozone: drm: Fix HardwareDisplayController use-after-free

ozone: drm: Fix HardwareDisplayController use-after-free

Any time we destroy a HardwareDisplayController, we have to be careful
to dissociate it from any DrmWindow objects. Make a new RemoveController
funtion to handle this.

BUG=477129
TEST=Unplug HDMI display from panther on login screen. No crash.

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

[spang, 2015-04-15 00:23:02]

spang@chromium.org changed reviewers:
+ alexst@chromium.org, dnicoara@chromium.org

[alexst (slow to review), 2015-04-15 00:50:03]

https://codereview.chromium.org/1072383003/diff/1/ui/ozone/platform/drm/gpu/s...
File ui/ozone/platform/drm/gpu/screen_manager.cc (left):

https://codereview.chromium.org/1072383003/diff/1/ui/ozone/platform/drm/gpu/s...
ui/ozone/platform/drm/gpu/screen_manager.cc:103: controllers_.erase(it);
I want to say there was a reason why this was removed after the mapping was
updated. The controller was disabled so didn't affect the window assignment, but
still a valid ptr for the duration of  UpdateControllerToWindowMapping();

Maybe Danny remembers.

[spang, 2015-04-15 02:12:11]

https://codereview.chromium.org/1072383003/diff/1/ui/ozone/platform/drm/gpu/s...
File ui/ozone/platform/drm/gpu/screen_manager.cc (left):

https://codereview.chromium.org/1072383003/diff/1/ui/ozone/platform/drm/gpu/s...
ui/ozone/platform/drm/gpu/screen_manager.cc:103: controllers_.erase(it);
On 2015/04/15 00:50:02, alexst wrote:
> I want to say there was a reason why this was removed after the mapping was
> updated. The controller was disabled so didn't affect the window assignment,
but
> still a valid ptr for the duration of  UpdateControllerToWindowMapping();
> 
> Maybe Danny remembers.

I don't think we are intentionally invoking UB and crashing. The order just
seems mixed up to me.

[dnicoara, 2015-04-15 13:43:42]

Would you mind adding/updating one of the unittests to make sure a window isn't
associated with an invalid controller?

https://codereview.chromium.org/1072383003/diff/1/ui/ozone/platform/drm/gpu/s...
File ui/ozone/platform/drm/gpu/screen_manager.cc (left):

https://codereview.chromium.org/1072383003/diff/1/ui/ozone/platform/drm/gpu/s...
ui/ozone/platform/drm/gpu/screen_manager.cc:103: controllers_.erase(it);
On 2015/04/15 02:12:11, spang wrote:
> On 2015/04/15 00:50:02, alexst wrote:
> > I want to say there was a reason why this was removed after the mapping was
> > updated. The controller was disabled so didn't affect the window assignment,
> but
> > still a valid ptr for the duration of  UpdateControllerToWindowMapping();
> > 
> > Maybe Danny remembers.
> 
> I don't think we are intentionally invoking UB and crashing. The order just
> seems mixed up to me.

Yeah, order is messed up.

https://codereview.chromium.org/1072383003/diff/1/ui/ozone/platform/drm/gpu/s...
File ui/ozone/platform/drm/gpu/screen_manager.cc (right):

https://codereview.chromium.org/1072383003/diff/1/ui/ozone/platform/drm/gpu/s...
ui/ozone/platform/drm/gpu/screen_manager.cc:285: RemoveController(original);
This will cause UpdateControllerToWindowMapping() to be called twice since
HandleMirrorMode is just a helper function. The update happens in
ConfigureDisplayController().

[spang, 2015-04-15 13:48:24]

https://codereview.chromium.org/1072383003/diff/1/ui/ozone/platform/drm/gpu/s...
File ui/ozone/platform/drm/gpu/screen_manager.cc (right):

https://codereview.chromium.org/1072383003/diff/1/ui/ozone/platform/drm/gpu/s...
ui/ozone/platform/drm/gpu/screen_manager.cc:285: RemoveController(original);
On 2015/04/15 13:43:41, dnicoara wrote:
> This will cause UpdateControllerToWindowMapping() to be called twice since
> HandleMirrorMode is just a helper function. The update happens in
> ConfigureDisplayController().

Done.

[dnicoara, 2015-04-15 13:58:55]

lgtm

[spang, 2015-04-15 14:37:29]

The CQ bit was checked by spang@chromium.org

[spang, 2015-04-15 14:37:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
dnicoara@chromium.org
Link to the patchset: https://codereview.chromium.org/1072383003/#ps40001
(title: "add test")

[commit-bot: I haz the power, 2015-04-15 14:37:49]

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

[commit-bot: I haz the power, 2015-04-15 15:03:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-15 15:04:22]

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