ozone: drm: Fix HardwareDisplayController use-after-free

ui/ozone/platform/drm/gpu/screen_manager.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/ozone/platform/drm/gpu/screen_manager.h"
 
 #include <xf86drmMode.h>
 
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "ui/gfx/geometry/point.h"
@@ skipping 80 matching lines @@
   controllers_.push_back(new HardwareDisplayController(
       scoped_ptr<CrtcController>(new CrtcController(drm, crtc, connector))));
 }
 
 void ScreenManager::RemoveDisplayController(const scoped_refptr<DrmDevice>& drm,
                                             uint32_t crtc) {
   HardwareDisplayControllers::iterator it = FindDisplayController(drm, crtc);
   if (it != controllers_.end()) {
     bool is_mirrored = (*it)->IsMirrored();
     (*it)->RemoveCrtc(drm, crtc);
-    if (!is_mirrored) {
-      UpdateControllerToWindowMapping();
-      controllers_.erase(it);

[alexst (slow to review), 2015/04/15 00:50:02]

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]

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:41]

Yeah, order is messed up.

-    }
+    if (!is_mirrored)
+      RemoveController(it);
   }
 }
 
 bool ScreenManager::ConfigureDisplayController(
     const scoped_refptr<DrmDevice>& drm,
     uint32_t crtc,
     uint32_t connector,
     const gfx::Point& origin,
     const drmModeModeInfo& mode) {
   bool status =
@@ skipping 162 matching lines @@
 }
 
 bool ScreenManager::HandleMirrorMode(
     HardwareDisplayControllers::iterator original,
     HardwareDisplayControllers::iterator mirror,
     const scoped_refptr<DrmDevice>& drm,
     uint32_t crtc,
     uint32_t connector) {
   (*mirror)->AddCrtc((*original)->RemoveCrtc(drm, crtc));
   if ((*mirror)->Enable()) {
-    controllers_.erase(original);
+    RemoveController(original);

[dnicoara, 2015/04/15 13:43:41]

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]

Done.

     return true;
   }
 
   LOG(ERROR) << "Failed to switch to mirror mode";
 
   // When things go wrong revert back to the previous configuration since
   // it is expected that the configuration would not have changed if
   // things fail.
   (*original)->AddCrtc((*mirror)->RemoveCrtc(drm, crtc));
   (*original)->Enable();
@@ skipping 28 matching lines @@
 
 DrmWindow* ScreenManager::FindWindowAt(const gfx::Rect& bounds) const {
   for (auto pair : window_map_) {
     if (pair.second->bounds() == bounds)
       return pair.second;
   }
 
   return nullptr;
 }
 
+void ScreenManager::RemoveController(
+    HardwareDisplayControllers::iterator controller) {
+  controllers_.erase(controller);
+  UpdateControllerToWindowMapping();
+}
+
 }  // namespace ui