UI/Display: Simplify and Clean Up ConfigureDisplayTask

This CL simplifies and cleans up the way ConfigureDisplayTask manages
its state over sync/async callbacks to reduce complexity and race-risk
by tightening up the way calls to Run() and OnConfigure() are handled.

In addition, this CL adds a test to ensure that we attempt to
reconfigure displays when any of them fail, not just the last one.

Finally, it removes ConfigureDisplaysTaskTest.ConfigureWithNoDisplays,
since this case should never occur in reality.

TEST=display_unittests

Change-Id: Icf34b00205e980df2bb5facbe6da5387a3e9dce3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2422559
Commit-Queue: Gil Dekel <gildekel@chromium.org>
Reviewed-by: Daniel Nicoara <dnicoara@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814355}

ui/display/manager/configure_displays_task.cc

@@ -96,11 +96,7 @@ ConfigureDisplaysTask::ConfigureDisplaysTask(
     : delegate_(delegate),
       requests_(requests),
       callback_(std::move(callback)),
-      is_configuring_(false),
-      num_displays_configured_(0),
       task_status_(SUCCESS) {
-  for (size_t i = 0; i < requests_.size(); ++i)
-    pending_request_indexes_.push(i);
   delegate_->AddObserver(this);
 }
 
@@ -109,81 +105,46 @@ ConfigureDisplaysTask::~ConfigureDisplaysTask() {
 }
 
 void ConfigureDisplaysTask::Run() {
-  // Synchronous configurators will recursively call Run(). In that case just
-  // defer their call to the next iteration in the while-loop. This is done to
-  // guard against stack overflows if the display has a large list of broken
-  // modes.
-  if (is_configuring_)
-    return;
-
-  {
-    base::AutoReset<bool> recursivity_guard(&is_configuring_, true);
-    // The callback passed to delegate_->Configure() could be run synchronously
-    // or async. If it runs synchronously and any display fails and tries to
-    // reconfigure, new requests will get added to |pending_request_indexes_|,
-    // then another attempt to reconfigure will recursively call Run(). However
-    // |is_configuring_| will block the run due to the reason mentioned above.
-    // Hence, after we configure the first set of pending requests (loop over
-    // |current_pending_requests_count|), we check again if the new requests
-    // were added (!pending_request_indexes_.empty()).
-    while (!pending_request_indexes_.empty()) {
-      std::vector<display::DisplayConfigurationParams> config_requests;
-      size_t current_pending_requests_count = pending_request_indexes_.size();
-      for (size_t i = 0; i < current_pending_requests_count; ++i) {
-        size_t index = pending_request_indexes_.front();
-        DisplayConfigureRequest* request = &requests_[index];
-        pending_request_indexes_.pop();
-
-        const bool internal =
-            request->display->type() == DISPLAY_CONNECTION_TYPE_INTERNAL;
-        base::UmaHistogramExactLinear(
-            internal ? "ConfigureDisplays.Internal.Modeset.Resolution"
-                     : "ConfigureDisplays.External.Modeset.Resolution",
-            ComputeDisplayResolutionEnum(request->mode),
-            base::size(kDisplayResolutionSamples) *
-                    base::size(kDisplayResolutionSamples) +
-                2);
-        base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
-            internal ? "ConfigureDisplays.Internal.Modeset.RefreshRate"
-                     : "ConfigureDisplays.External.Modeset.RefreshRate",
-            1, 240, 18, base::HistogramBase::kUmaTargetedHistogramFlag);
-        histogram->Add(request->mode ? std::round(request->mode->refresh_rate())
-                                     : 0);
-
-        display::DisplayConfigurationParams display_config_params(
-            request->display->display_id(), request->origin, request->mode);
-        config_requests.push_back(std::move(display_config_params));
-      }
-      if (!config_requests.empty()) {
-        delegate_->Configure(
-            config_requests,
-            base::BindOnce(&ConfigureDisplaysTask::OnConfigured,
-                           weak_ptr_factory_.GetWeakPtr()));
-      }
-    }
+  DCHECK(!requests_.empty());
+
+  std::vector<display::DisplayConfigurationParams> config_requests;
+  for (const auto& request : requests_) {
+    config_requests.emplace_back(request.display->display_id(), request.origin,
+                                 request.mode);
+
+    const bool internal =
+        request.display->type() == DISPLAY_CONNECTION_TYPE_INTERNAL;
+    base::UmaHistogramExactLinear(
+        internal ? "ConfigureDisplays.Internal.Modeset.Resolution"
+                 : "ConfigureDisplays.External.Modeset.Resolution",
+        ComputeDisplayResolutionEnum(request.mode),
+        base::size(kDisplayResolutionSamples) *
+                base::size(kDisplayResolutionSamples) +
+            2);
+    base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
+        internal ? "ConfigureDisplays.Internal.Modeset.RefreshRate"
+                 : "ConfigureDisplays.External.Modeset.RefreshRate",
+        1, 240, 18, base::HistogramBase::kUmaTargetedHistogramFlag);
+    histogram->Add(request.mode ? std::round(request.mode->refresh_rate()) : 0);
   }
 
-  // Nothing should be modified after the |callback_| is called since the
-  // task may be deleted in the callback.
-  if (num_displays_configured_ == requests_.size())
-    std::move(callback_).Run(task_status_);
+  delegate_->Configure(config_requests,
+                       base::BindOnce(&ConfigureDisplaysTask::OnConfigured,
+                                      weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ConfigureDisplaysTask::OnConfigurationChanged() {}
 
 void ConfigureDisplaysTask::OnDisplaySnapshotsInvalidated() {
-  base::queue<size_t> empty_queue;
-  pending_request_indexes_.swap(empty_queue);
   // From now on, don't access |requests_[index]->display|; they're invalid.
   task_status_ = ERROR;
   weak_ptr_factory_.InvalidateWeakPtrs();
-  Run();
+  std::move(callback_).Run(task_status_);
 }
 
 void ConfigureDisplaysTask::OnConfigured(
     const base::flat_map<int64_t, bool>& statuses) {
   bool config_success = true;
-
   // Check if all displays are successfully configured.
   for (const auto& status : statuses) {
     int64_t display_id = status.first;
@@ -206,11 +167,11 @@ void ConfigureDisplaysTask::OnConfigured(
         display_success);
   }
 
+  // Update displays upon success or prep |requests_| for reconfiguration.
   if (config_success) {
-    for (const auto& status : statuses) {
-      auto request = GetRequestForDisplayId(status.first, requests_);
-      request->display->set_current_mode(request->mode);
-      request->display->set_origin(request->origin);
+    for (auto& request : requests_) {
+      request.display->set_current_mode(request.mode);
+      request.display->set_origin(request.origin);
     }
   } else {
     bool should_reconfigure = false;
@@ -230,37 +191,25 @@ void ConfigureDisplaysTask::OnConfigured(
         }
       }
     }
-    // When reconfiguring, reconfigure all displays, not only the failing ones
-    // as they could potentially depend on each other.
     if (should_reconfigure) {
-      for (const auto& status : statuses) {
-        auto const_iterator = GetRequestForDisplayId(status.first, requests_);
-        auto request = requests_.erase(const_iterator, const_iterator);
-        size_t index = std::distance(requests_.begin(), request);
-        pending_request_indexes_.push(index);
-      }
-      if (task_status_ == SUCCESS)
-        task_status_ = PARTIAL_SUCCESS;
+      task_status_ = PARTIAL_SUCCESS;
       Run();
       return;
     }
   }
 
-  // If no reconfigurations are happening, update the final state.
-  for (const auto& status : statuses) {
-    auto request = GetRequestForDisplayId(status.first, requests_);
-    bool internal =
-        request->display->type() == DISPLAY_CONNECTION_TYPE_INTERNAL;
+  // Update the final state.
+  for (auto& request : requests_) {
+    bool internal = request.display->type() == DISPLAY_CONNECTION_TYPE_INTERNAL;
     base::UmaHistogramBoolean(
         internal ? "ConfigureDisplays.Internal.Modeset.FinalStatus"
                  : "ConfigureDisplays.External.Modeset.FinalStatus",
         config_success);
   }
 
-  num_displays_configured_ += statuses.size();
   if (!config_success)
     task_status_ = ERROR;
-  Run();
+  std::move(callback_).Run(task_status_);
 }
 
 }  // namespace display

ui/display/manager/configure_displays_task.h

@@ -76,17 +76,6 @@ class DISPLAY_MANAGER_EXPORT ConfigureDisplaysTask
   // task is done and the task status.
   ResponseCallback callback_;
 
-  // Stores the indexes of pending requests in |requests_|.
-  base::queue<size_t> pending_request_indexes_;
-
-  // Used to keep make sure that synchronous executions do not recurse during
-  // the configuration.
-  bool is_configuring_;
-
-  // Number of display configured. This is used to check whether there are
-  // pending requests.
-  size_t num_displays_configured_;
-
   Status task_status_;
 
   base::WeakPtrFactory<ConfigureDisplaysTask> weak_ptr_factory_{this};

ui/display/manager/configure_displays_task_unittest.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include <stddef.h>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/run_loop.h"
@@ -24,22 +25,23 @@ class ConfigureDisplaysTaskTest : public testing::Test {
   ConfigureDisplaysTaskTest()
       : delegate_(&log_),
         small_mode_(gfx::Size(1366, 768), false, 60.0f),
-        big_mode_(gfx::Size(2560, 1600), false, 60.0f) {
-    displays_[0] = FakeDisplaySnapshot::Builder()
-                       .SetId(123)
-                       .SetNativeMode(small_mode_.Clone())
-                       .SetCurrentMode(small_mode_.Clone())
-                       .Build();
-
-    displays_[1] = FakeDisplaySnapshot::Builder()
-                       .SetId(456)
-                       .SetNativeMode(big_mode_.Clone())
-                       .SetCurrentMode(big_mode_.Clone())
-                       .AddMode(small_mode_.Clone())
-                       .Build();
-  }
+        big_mode_(gfx::Size(2560, 1600), false, 60.0f) {}
   ~ConfigureDisplaysTaskTest() override = default;
 
+  void SetUp() override {
+    displays_.push_back(FakeDisplaySnapshot::Builder()
+                            .SetId(123)
+                            .SetNativeMode(small_mode_.Clone())
+                            .SetCurrentMode(small_mode_.Clone())
+                            .Build());
+    displays_.push_back(FakeDisplaySnapshot::Builder()
+                            .SetId(456)
+                            .SetNativeMode(big_mode_.Clone())
+                            .SetCurrentMode(big_mode_.Clone())
+                            .AddMode(small_mode_.Clone())
+                            .Build());
+  }
+
   void ConfigureCallback(ConfigureDisplaysTask::Status status) {
     callback_called_ = true;
     status_ = status;
@@ -56,7 +58,7 @@ class ConfigureDisplaysTaskTest : public testing::Test {
   const DisplayMode small_mode_;
   const DisplayMode big_mode_;
 
-  std::unique_ptr<DisplaySnapshot> displays_[2];
+  std::vector<std::unique_ptr<DisplaySnapshot>> displays_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ConfigureDisplaysTaskTest);
@@ -64,20 +66,6 @@ class ConfigureDisplaysTaskTest : public testing::Test {
 
 }  // namespace
 
-TEST_F(ConfigureDisplaysTaskTest, ConfigureWithNoDisplays) {
-  ConfigureDisplaysTask::ResponseCallback callback = base::BindOnce(
-      &ConfigureDisplaysTaskTest::ConfigureCallback, base::Unretained(this));
-
-  ConfigureDisplaysTask task(&delegate_, std::vector<DisplayConfigureRequest>(),
-                             std::move(callback));
-
-  task.Run();
-
-  EXPECT_TRUE(callback_called_);
-  EXPECT_EQ(ConfigureDisplaysTask::SUCCESS, status_);
-  EXPECT_EQ(kNoActions, log_.GetActionsAndClear());
-}
-
 TEST_F(ConfigureDisplaysTaskTest, ConfigureWithOneDisplay) {
   ConfigureDisplaysTask::ResponseCallback callback = base::BindOnce(
       &ConfigureDisplaysTaskTest::ConfigureCallback, base::Unretained(this));
@@ -100,9 +88,8 @@ TEST_F(ConfigureDisplaysTaskTest, ConfigureWithTwoDisplay) {
       &ConfigureDisplaysTaskTest::ConfigureCallback, base::Unretained(this));
 
   std::vector<DisplayConfigureRequest> requests;
-  for (size_t i = 0; i < base::size(displays_); ++i) {
-    requests.emplace_back(displays_[i].get(), displays_[i]->native_mode(),
-                          gfx::Point());
+  for (const auto& display : displays_) {
+    requests.emplace_back(display.get(), display->native_mode(), gfx::Point());
   }
 
   ConfigureDisplaysTask task(&delegate_, requests, std::move(callback));
@@ -170,9 +157,8 @@ TEST_F(ConfigureDisplaysTaskTest, ConfigureWithTwoDisplayFails) {
   delegate_.set_max_configurable_pixels(1);
 
   std::vector<DisplayConfigureRequest> requests;
-  for (size_t i = 0; i < base::size(displays_); ++i) {
-    requests.emplace_back(displays_[i].get(), displays_[i]->native_mode(),
-                          gfx::Point());
+  for (const auto& display : displays_) {
+    requests.emplace_back(display.get(), display->native_mode(), gfx::Point());
   }
 
   ConfigureDisplaysTask task(&delegate_, requests, std::move(callback));
@@ -196,16 +182,15 @@ TEST_F(ConfigureDisplaysTaskTest, ConfigureWithTwoDisplayFails) {
             log_.GetActionsAndClear());
 }
 
-TEST_F(ConfigureDisplaysTaskTest, ConfigureWithTwoDisplaysPartialSuccess) {
+TEST_F(ConfigureDisplaysTaskTest, ReconfigureLastDisplayPartialSuccess) {
   ConfigureDisplaysTask::ResponseCallback callback = base::BindOnce(
       &ConfigureDisplaysTaskTest::ConfigureCallback, base::Unretained(this));
 
   delegate_.set_max_configurable_pixels(small_mode_.size().GetArea());
 
   std::vector<DisplayConfigureRequest> requests;
-  for (size_t i = 0; i < base::size(displays_); ++i) {
-    requests.emplace_back(displays_[i].get(), displays_[i]->native_mode(),
-                          gfx::Point());
+  for (const auto& display : displays_) {
+    requests.emplace_back(display.get(), display->native_mode(), gfx::Point());
   }
 
   ConfigureDisplaysTask task(&delegate_, requests, std::move(callback));
@@ -229,6 +214,50 @@ TEST_F(ConfigureDisplaysTaskTest, ConfigureWithTwoDisplaysPartialSuccess) {
             log_.GetActionsAndClear());
 }
 
+TEST_F(ConfigureDisplaysTaskTest, ReconfigureMiddleDisplayPartialSuccess) {
+  ConfigureDisplaysTask::ResponseCallback callback = base::BindOnce(
+      &ConfigureDisplaysTaskTest::ConfigureCallback, base::Unretained(this));
+
+  displays_.push_back(FakeDisplaySnapshot::Builder()
+                          .SetId(789)
+                          .SetNativeMode(small_mode_.Clone())
+                          .SetCurrentMode(small_mode_.Clone())
+                          .Build());
+
+  delegate_.set_max_configurable_pixels(small_mode_.size().GetArea());
+
+  std::vector<DisplayConfigureRequest> requests;
+  for (const auto& display : displays_) {
+    requests.emplace_back(display.get(), display->native_mode(), gfx::Point());
+  }
+
+  ConfigureDisplaysTask task(&delegate_, requests, std::move(callback));
+  task.Run();
+
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(ConfigureDisplaysTask::PARTIAL_SUCCESS, status_);
+  EXPECT_EQ(JoinActions(GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
+                                       &small_mode_})
+                            .c_str(),
+                        GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
+                                       &big_mode_})
+                            .c_str(),
+                        GetCrtcAction({displays_[2]->display_id(), gfx::Point(),
+                                       &small_mode_})
+                            .c_str(),
+                        GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
+                                       &small_mode_})
+                            .c_str(),
+                        GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
+                                       &small_mode_})
+                            .c_str(),
+                        GetCrtcAction({displays_[2]->display_id(), gfx::Point(),
+                                       &small_mode_})
+                            .c_str(),
+                        nullptr),
+            log_.GetActionsAndClear());
+}
+
 TEST_F(ConfigureDisplaysTaskTest, AsyncConfigureWithTwoDisplaysPartialSuccess) {
   ConfigureDisplaysTask::ResponseCallback callback = base::BindOnce(
       &ConfigureDisplaysTaskTest::ConfigureCallback, base::Unretained(this));
@@ -237,9 +266,8 @@ TEST_F(ConfigureDisplaysTaskTest, AsyncConfigureWithTwoDisplaysPartialSuccess) {
   delegate_.set_max_configurable_pixels(small_mode_.size().GetArea());
 
   std::vector<DisplayConfigureRequest> requests;
-  for (size_t i = 0; i < base::size(displays_); ++i) {
-    requests.emplace_back(displays_[i].get(), displays_[i]->native_mode(),
-                          gfx::Point());
+  for (const auto& display : displays_) {
+    requests.emplace_back(display.get(), display->native_mode(), gfx::Point());
   }
 
   ConfigureDisplaysTask task(&delegate_, requests, std::move(callback));

ui/ozone/platform/drm/host/host_drm_device.cc

@@ -12,6 +12,7 @@
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/task_runner.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "mojo/public/cpp/bindings/pending_associated_remote.h"
 #include "ui/display/types/display_snapshot.h"
@@ -139,7 +140,12 @@ void HostDrmDevice::GpuConfigureNativeDisplays(
     base::flat_map<int64_t, bool> dummy_statuses;
     for (const auto& config : config_requests)
       dummy_statuses.insert(std::make_pair(config.id, false));
-    std::move(callback).Run(dummy_statuses);
+
+    // Post this task to protect the callstack from accumulating too many
+    // recursive calls to ConfigureDisplaysTask::Run() in cases in which the GPU
+    // process crashes repeatedly.
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(callback), dummy_statuses));
   }
 }