Fix possible multiple calls to MidiManager::StartInitialization() in StartSession()

media/midi/midi_manager.cc

 // Copyright (c) 2013 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 "media/midi/midi_manager.h"
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 20 matching lines @@
   SESSION_STARTED,
   SESSION_ENDED,
   INITIALIZED,
   INPUT_PORT_ADDED,
   OUTPUT_PORT_ADDED,
 
   // New items should be inserted here, and |MAX| should point the last item.
   MAX = INITIALIZED,
 };
 
-// Used in StartSession.
-enum class Completion {
-  COMPLETE_SYNCHRONOUSLY,
-  INVOKE_INITIALIZATION,
-  COMPLETE_ASYNCHRONOUSLY,
-};
-
 void ReportUsage(Usage usage) {
   UMA_HISTOGRAM_ENUMERATION("Media.Midi.Usage",
                             static_cast<Sample>(usage),
                             static_cast<Sample>(Usage::MAX) + 1);
 }
 
 }  // namespace
 
 MidiManager::MidiManager()
-    : initialized_(false), finalized_(false), result_(Result::NOT_INITIALIZED) {
+    : initialization_state_(InitializationState::NOT_STARTED),
+      finalized_(false),
+      result_(Result::NOT_INITIALIZED) {
   ReportUsage(Usage::CREATED);
 }
 
 MidiManager::~MidiManager() {
   // Make sure that Finalize() is called to clean up resources allocated on
   // the Chrome_IOThread.
   base::AutoLock auto_lock(lock_);
   CHECK(finalized_);
 }
 
@@ skipping 16 matching lines @@
                               base::Unretained(this)));
     session_thread_runner_ = nullptr;
   } else {
     finalized_ = true;
   }
 }
 
 void MidiManager::StartSession(MidiManagerClient* client) {
   ReportUsage(Usage::SESSION_STARTED);
 
-  Completion completion = Completion::COMPLETE_SYNCHRONOUSLY;
-  Result result = Result::NOT_INITIALIZED;
+  bool needs_initialization = false;
 
   {
     base::AutoLock auto_lock(lock_);
     if (clients_.find(client) != clients_.end() ||
         pending_clients_.find(client) != pending_clients_.end()) {
       // Should not happen. But just in case the renderer is compromised.
       NOTREACHED();
       return;
     }
 
-    if (initialized_) {
+    // Do not accept a new request if Shutdown() was already called.

[Shao-Chuan Lee, 2016/10/24 08:54:34]

I guess this check should be moved in front of the COMPLETED check, otherwise
the client would assume this MidiManager is still alive.

[Takashi Toyoshima, 2016/10/24 10:07:29]

Thanks, this looks a nice improvement!

+    if (finalized_) {
+      client->CompleteStartSession(Result::INITIALIZATION_ERROR);
+      return;
+    }
+
+    if (initialization_state_ == InitializationState::COMPLETED) {
       // Platform dependent initialization was already finished for previously
       // initialized clients.
       if (result_ == Result::OK) {
         AddInitialPorts(client);
         clients_.insert(client);
       }
       // Complete synchronously with |result_|;
-      result = result_;
-    } else {
-      bool too_many_pending_clients_exist =
-          pending_clients_.size() >= kMaxPendingClientCount;
-      // Do not accept a new request if the pending client list contains too
-      // many clients, or Shutdown() was already called.
-      if (too_many_pending_clients_exist || finalized_) {
-        result = Result::INITIALIZATION_ERROR;
-      } else {
-        // Call StartInitialization() only for the first request.
-        if (pending_clients_.empty()) {
-          completion = Completion::INVOKE_INITIALIZATION;
-          session_thread_runner_ = base::ThreadTaskRunnerHandle::Get();
-        } else {
-          completion = Completion::COMPLETE_ASYNCHRONOUSLY;
-        }
-        pending_clients_.insert(client);
-      }
+      client->CompleteStartSession(result_);
+      return;
     }
 
-    if (completion == Completion::COMPLETE_SYNCHRONOUSLY) {
-      client->CompleteStartSession(result);
+    // Do not accept a new request if the pending client list contains too
+    // many clients.
+    if (pending_clients_.size() >= kMaxPendingClientCount) {
+      client->CompleteStartSession(Result::INITIALIZATION_ERROR);
       return;
     }
+
+    if (initialization_state_ == InitializationState::NOT_STARTED) {
+      // Set fields protected by |lock_| here and call StartInitialization()
+      // later.
+      needs_initialization = true;
+      session_thread_runner_ = base::ThreadTaskRunnerHandle::Get();
+      initialization_state_ = InitializationState::STARTED;
+    }
+
+    pending_clients_.insert(client);
   }
 
-  if (completion == Completion::INVOKE_INITIALIZATION) {
+  if (needs_initialization) {
     // Lazily initialize the MIDI back-end.
     TRACE_EVENT0("midi", "MidiManager::StartInitialization");
     // CompleteInitialization() will be called asynchronously when platform
     // dependent initialization is finished.
     StartInitialization();
   }
 }
 
 void MidiManager::EndSession(MidiManagerClient* client) {
   ReportUsage(Usage::SESSION_ENDED);
@@ skipping 83 matching lines @@
   ReportUsage(Usage::INITIALIZED);
   UMA_HISTOGRAM_ENUMERATION("Media.Midi.InputPorts",
                             static_cast<Sample>(input_ports_.size()),
                             kMaxUmaDevices + 1);
   UMA_HISTOGRAM_ENUMERATION("Media.Midi.OutputPorts",
                             static_cast<Sample>(output_ports_.size()),
                             kMaxUmaDevices + 1);
 
   base::AutoLock auto_lock(lock_);
   DCHECK(clients_.empty());
-  DCHECK(!initialized_);
-  initialized_ = true;
+  DCHECK_EQ(initialization_state_, InitializationState::STARTED);
+  initialization_state_ = InitializationState::COMPLETED;
   result_ = result;
 
   for (auto* client : pending_clients_) {
     if (result_ == Result::OK) {
       AddInitialPorts(client);
       clients_.insert(client);
     }
     client->CompleteStartSession(result_);
   }
   pending_clients_.clear();
@@ skipping 12 matching lines @@
   Finalize();
   base::AutoLock auto_lock(lock_);
   finalized_ = true;
 
   // Detach all clients so that they do not call MidiManager methods any more.
   for (auto* client : clients_)
     client->Detach();
 }
 
 }  // namespace midi