Manage MIDI related objects and sessions' lifecycles correctly

Following changes are needed to maintain MIDIPortInfoList contents
consistent in each MidiMessageFilter instances when MidiManager
update the list asynchronously.

Essential changes:
- Call EndSession iff a session is open.
- Do not use client_id to communicate between browser and renderer.
  Multiple clients are managed in a MidiMessageFilter existing in
  each renderer.

Minor changes:
- Remove midi_manager_ checks in MIDIHost, and add CHECK insteads.
- Rename MidiManagerFilter::StartSession to MidiManagerFilter::AddClient
  to be aligned with RemoveClient. A session can be shared with multiple
  clients.

After this change, MidiManager::EndSession is called from each Renderer
and RendererHost at the right timing, e.g., GC collects the last MIDIAccess
reference in JavaScript. See, also http://crbug.com/424859.

BUG=424859

Review URL: https://codereview.chromium.org/662853003

Cr-Commit-Position: refs/heads/master@{#300851}

5 files changed, 42 insertions, 42 deletions

diff --git a/media/midi/midi_manager.cc b/media/midi/midi_manager.cc
index c53eef4..b0439ec 100644
--- a/media/midi/midi_manager.cc
+++ b/media/midi/midi_manager.cc
@@ -26,7 +26,7 @@ MidiManager* MidiManager::Create() {
 }
 #endif
 
-void MidiManager::StartSession(MidiManagerClient* client, int client_id) {
+void MidiManager::StartSession(MidiManagerClient* client) {
   bool session_is_ready;
   bool session_needs_initialization = false;
   bool too_many_pending_clients_exist = false;
@@ -34,6 +34,12 @@ void MidiManager::StartSession(MidiManagerClient* client, int client_id) {
   {
     base::AutoLock auto_lock(lock_);
     session_is_ready = initialized_;
+    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 (!session_is_ready) {
       // Do not accept a new request if the pending client list contains too
       // many clients.
@@ -43,7 +49,7 @@ void MidiManager::StartSession(MidiManagerClient* client, int client_id) {
       if (!too_many_pending_clients_exist) {
         // Call StartInitialization() only for the first request.
         session_needs_initialization = pending_clients_.empty();
-        pending_clients_.insert(std::make_pair(client, client_id));
+        pending_clients_.insert(client);
       }
     }
   }
@@ -58,7 +64,7 @@ void MidiManager::StartSession(MidiManagerClient* client, int client_id) {
     }
     if (too_many_pending_clients_exist) {
       // Return an error immediately if there are too many requests.
-      client->CompleteStartSession(client_id, MIDI_INITIALIZATION_ERROR);
+      client->CompleteStartSession(MIDI_INITIALIZATION_ERROR);
       return;
     }
     // CompleteInitialization() will be called asynchronously when platform
@@ -75,10 +81,12 @@ void MidiManager::StartSession(MidiManagerClient* client, int client_id) {
       clients_.insert(client);
     result = result_;
   }
-  client->CompleteStartSession(client_id, result);
+  client->CompleteStartSession(result);
 }
 
 void MidiManager::EndSession(MidiManagerClient* client) {
+  // At this point, |client| can be in the destruction process, and calling
+  // any method of |client| is dangerous.
   base::AutoLock auto_lock(lock_);
   clients_.erase(client);
   pending_clients_.erase(client);
@@ -121,8 +129,8 @@ void MidiManager::ReceiveMidiData(
     double timestamp) {
   base::AutoLock auto_lock(lock_);
 
-  for (ClientList::iterator i = clients_.begin(); i != clients_.end(); ++i)
-    (*i)->ReceiveMidiData(port_index, data, length, timestamp);
+  for (MidiManagerClient* client : clients_)
+    client->ReceiveMidiData(port_index, data, length, timestamp);
 }
 
 void MidiManager::CompleteInitializationInternal(MidiResult result) {
@@ -134,12 +142,10 @@ void MidiManager::CompleteInitializationInternal(MidiResult result) {
   initialized_ = true;
   result_ = result;
 
-  for (PendingClientMap::iterator it = pending_clients_.begin();
-       it != pending_clients_.end();
-       ++it) {
+  for (MidiManagerClient* client : pending_clients_) {
     if (result_ == MIDI_OK)
-      clients_.insert(it->first);
-    it->first->CompleteStartSession(it->second, result_);
+      clients_.insert(client);
+    client->CompleteStartSession(result_);
   }
   pending_clients_.clear();
 }
diff --git a/media/midi/midi_manager.h b/media/midi/midi_manager.h
index 9fd7a21..1e2bcb8 100644
--- a/media/midi/midi_manager.h
+++ b/media/midi/midi_manager.h
@@ -5,7 +5,6 @@
 #ifndef MEDIA_MIDI_MIDI_MANAGER_H_
 #define MEDIA_MIDI_MIDI_MANAGER_H_
 
-#include <map>
 #include <set>
 #include <vector>
 
@@ -32,7 +31,7 @@ class MEDIA_EXPORT MidiManagerClient {
 
   // CompleteStartSession() is called when platform dependent preparation is
   // finished.
-  virtual void CompleteStartSession(int client_id, MidiResult result) = 0;
+  virtual void CompleteStartSession(MidiResult result) = 0;
 
   // ReceiveMidiData() is called when MIDI data has been received from the
   // MIDI system.
@@ -71,7 +70,7 @@ class MEDIA_EXPORT MidiManager {
   // Otherwise CompleteStartSession() is called with proper MidiResult code.
   // StartSession() and EndSession() can be called on the Chrome_IOThread.
   // CompleteStartSession() will be invoked on the same Chrome_IOThread.
-  void StartSession(MidiManagerClient* client, int client_id);
+  void StartSession(MidiManagerClient* client);
 
   // A client calls EndSession() to stop receiving MIDI data.
   void EndSession(MidiManagerClient* client);
@@ -148,12 +147,11 @@ class MEDIA_EXPORT MidiManager {
   void CompleteInitializationInternal(MidiResult result);
 
   // Keeps track of all clients who wish to receive MIDI data.
-  typedef std::set<MidiManagerClient*> ClientList;
-  ClientList clients_;
+  typedef std::set<MidiManagerClient*> ClientSet;
+  ClientSet clients_;
 
   // Keeps track of all clients who are waiting for CompleteStartSession().
-  typedef std::multimap<MidiManagerClient*, int> PendingClientMap;
-  PendingClientMap pending_clients_;
+  ClientSet pending_clients_;
 
   // Keeps a SingleThreadTaskRunner of the thread that calls StartSession in
   // order to invoke CompleteStartSession() on the thread.
diff --git a/media/midi/midi_manager_unittest.cc b/media/midi/midi_manager_unittest.cc
index 846e1bc..4a52ca6 100644
--- a/media/midi/midi_manager_unittest.cc
+++ b/media/midi/midi_manager_unittest.cc
@@ -54,16 +54,14 @@ class FakeMidiManager : public MidiManager {
 
 class FakeMidiManagerClient : public MidiManagerClient {
  public:
-  explicit FakeMidiManagerClient(int client_id)
-      : client_id_(client_id),
-        result_(MIDI_NOT_SUPPORTED),
+  FakeMidiManagerClient()
+      : result_(MIDI_NOT_SUPPORTED),
         wait_for_result_(true) {}
   ~FakeMidiManagerClient() override {}
 
   // MidiManagerClient implementation.
-  void CompleteStartSession(int client_id, MidiResult result) override {
+  void CompleteStartSession(MidiResult result) override {
     EXPECT_TRUE(wait_for_result_);
-    CHECK_EQ(client_id_, client_id);
     result_ = result;
     wait_for_result_ = false;
   }
@@ -74,7 +72,6 @@ class FakeMidiManagerClient : public MidiManagerClient {
                        double timestamp) override {}
   void AccumulateMidiBytesSent(size_t size) override {}
 
-  int client_id() const { return client_id_; }
   MidiResult result() const { return result_; }
 
   MidiResult WaitForResult() {
@@ -86,7 +83,6 @@ class FakeMidiManagerClient : public MidiManagerClient {
   }
 
  private:
-  int client_id_;
   MidiResult result_;
   bool wait_for_result_;
 
@@ -105,7 +101,7 @@ class MidiManagerTest : public ::testing::Test {
     EXPECT_FALSE(manager_->start_initialization_is_called_);
     EXPECT_EQ(0U, manager_->GetClientCount());
     EXPECT_EQ(0U, manager_->GetPendingClientCount());
-    manager_->StartSession(client, client->client_id());
+    manager_->StartSession(client);
     EXPECT_EQ(0U, manager_->GetClientCount());
     EXPECT_EQ(1U, manager_->GetPendingClientCount());
     EXPECT_TRUE(manager_->start_initialization_is_called_);
@@ -122,7 +118,7 @@ class MidiManagerTest : public ::testing::Test {
     // StartInitialization() should not be called for the second and later
     // sessions.
     manager_->start_initialization_is_called_ = false;
-    manager_->StartSession(client, client->client_id());
+    manager_->StartSession(client);
     EXPECT_EQ(nth == 1, manager_->start_initialization_is_called_);
     manager_->start_initialization_is_called_ = true;
   }
@@ -153,7 +149,7 @@ class MidiManagerTest : public ::testing::Test {
 
 TEST_F(MidiManagerTest, StartAndEndSession) {
   scoped_ptr<FakeMidiManagerClient> client;
-  client.reset(new FakeMidiManagerClient(0));
+  client.reset(new FakeMidiManagerClient);
 
   StartTheFirstSession(client.get());
   CompleteInitialization(MIDI_OK);
@@ -163,7 +159,7 @@ TEST_F(MidiManagerTest, StartAndEndSession) {
 
 TEST_F(MidiManagerTest, StartAndEndSessionWithError) {
   scoped_ptr<FakeMidiManagerClient> client;
-  client.reset(new FakeMidiManagerClient(1));
+  client.reset(new FakeMidiManagerClient);
 
   StartTheFirstSession(client.get());
   CompleteInitialization(MIDI_INITIALIZATION_ERROR);
@@ -175,9 +171,9 @@ TEST_F(MidiManagerTest, StartMultipleSessions) {
   scoped_ptr<FakeMidiManagerClient> client1;
   scoped_ptr<FakeMidiManagerClient> client2;
   scoped_ptr<FakeMidiManagerClient> client3;
-  client1.reset(new FakeMidiManagerClient(0));
-  client2.reset(new FakeMidiManagerClient(1));
-  client3.reset(new FakeMidiManagerClient(1));
+  client1.reset(new FakeMidiManagerClient);
+  client2.reset(new FakeMidiManagerClient);
+  client3.reset(new FakeMidiManagerClient);
 
   StartTheFirstSession(client1.get());
   StartTheNthSession(client2.get(), 2);
@@ -199,16 +195,15 @@ TEST_F(MidiManagerTest, TooManyPendingSessions) {
   ScopedVector<FakeMidiManagerClient> many_existing_clients;
   many_existing_clients.resize(MidiManager::kMaxPendingClientCount);
   for (size_t i = 0; i < MidiManager::kMaxPendingClientCount; ++i) {
-    many_existing_clients[i] = new FakeMidiManagerClient(i);
+    many_existing_clients[i] = new FakeMidiManagerClient;
     StartTheNthSession(many_existing_clients[i], i + 1);
   }
 
   // Push the last client that should be rejected for too many pending requests.
   scoped_ptr<FakeMidiManagerClient> additional_client(
-      new FakeMidiManagerClient(MidiManager::kMaxPendingClientCount));
+      new FakeMidiManagerClient);
   manager_->start_initialization_is_called_ = false;
-  manager_->StartSession(additional_client.get(),
-                         additional_client->client_id());
+  manager_->StartSession(additional_client.get());
   EXPECT_FALSE(manager_->start_initialization_is_called_);
   EXPECT_EQ(MIDI_INITIALIZATION_ERROR, additional_client->result());
 
@@ -232,7 +227,7 @@ TEST_F(MidiManagerTest, AbortSession) {
   // A client starting a session can be destructed while an asynchronous
   // initialization is performed.
   scoped_ptr<FakeMidiManagerClient> client;
-  client.reset(new FakeMidiManagerClient(0));
+  client.reset(new FakeMidiManagerClient);
 
   StartTheFirstSession(client.get());
   EndSession(client.get(), 0, 0);
@@ -246,10 +241,10 @@ TEST_F(MidiManagerTest, AbortSession) {
 
 TEST_F(MidiManagerTest, CreateMidiManager) {
   scoped_ptr<FakeMidiManagerClient> client;
-  client.reset(new FakeMidiManagerClient(0));
+  client.reset(new FakeMidiManagerClient);
 
   scoped_ptr<MidiManager> manager(MidiManager::Create());
-  manager->StartSession(client.get(), client->client_id());
+  manager->StartSession(client.get());
 
   MidiResult result = client->WaitForResult();
   // This #ifdef needs to be identical to the one in media/midi/midi_manager.cc.
diff --git a/media/midi/midi_manager_usb_unittest.cc b/media/midi/midi_manager_usb_unittest.cc
index b4f939f..1eeae9e 100644
--- a/media/midi/midi_manager_usb_unittest.cc
+++ b/media/midi/midi_manager_usb_unittest.cc
@@ -78,7 +78,7 @@ class FakeMidiManagerClient : public MidiManagerClient {
         logger_(logger) {}
   ~FakeMidiManagerClient() override {}
 
-  void CompleteStartSession(int client_id, MidiResult result) override {
+  void CompleteStartSession(MidiResult result) override {
     complete_start_session_ = true;
     result_ = result;
   }
@@ -159,7 +159,7 @@ class MidiManagerUsbTest : public ::testing::Test {
  protected:
   void Initialize() {
     client_.reset(new FakeMidiManagerClient(&logger_));
-    manager_->StartSession(client_.get(), 0);
+    manager_->StartSession(client_.get());
   }
 
   void Finalize() {
diff --git a/media/midi/midi_result.h b/media/midi/midi_result.h
index 1e10401..2fb58a4 100644
--- a/media/midi/midi_result.h
+++ b/media/midi/midi_result.h
@@ -9,7 +9,8 @@ namespace media {
 
 // Result codes for MIDI.
 enum MidiResult {
-  MIDI_OK,
+  MIDI_NOT_INITIALIZED = -1,
+  MIDI_OK = 0,
   MIDI_NOT_SUPPORTED,
   MIDI_INITIALIZATION_ERROR,