Web MIDI: speculative valgrind fix for a unit test on mac

media/midi/midi_manager_mac_unittest.cc

Index: media/midi/midi_manager_mac_unittest.cc
diff --git a/media/midi/midi_manager_mac_unittest.cc b/media/midi/midi_manager_mac_unittest.cc
index e608d7c6ede7a517bda627c9b1cdea0e37be27ed..092af8725b4a78da5dbe1ef1418aab587e806b19 100644
--- a/media/midi/midi_manager_mac_unittest.cc
+++ b/media/midi/midi_manager_mac_unittest.cc
@@ -10,6 +10,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
+#include "base/synchronization/lock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace media {
@@ -20,15 +21,24 @@ void Noop(const MIDIPacketList*, void*, void*) {}
 
 class FakeMidiManagerClient : public MidiManagerClient {
  public:
-  FakeMidiManagerClient()
-      : result_(MIDI_NOT_SUPPORTED),
-        wait_for_result_(true),
-        wait_for_port_(true) {}
+  FakeMidiManagerClient() {
+    base::AutoLock lock(lock_);

[yhirano, 2015/03/02 04:58:37]

It's hard to imagine that anyone can touch a FakeMidiManagerClient that is being
constructed. Can you remove the constructor change?

[Takashi Toyoshima, 2015/03/02 10:52:18]

I placed it not to avoid from racing, but for explicit memory barrier to expose
initialized values to other threads. Practically said, this won't be needed
because thread creation in StartSession will set memory barrier implicitly.

OK, I'll remove this since it will be overkill.

+    result_ = MIDI_NOT_SUPPORTED;
+    wait_for_result_ = true;
+    wait_for_port_ = true;
+  }
 
   // MidiManagerClient implementation.
   void AddInputPort(const MidiPortInfo& info) override {}
   void AddOutputPort(const MidiPortInfo& info) override {
-    CHECK(!wait_for_result_);
+    base::AutoLock lock(lock_);
+    // AddOutputPort may be called before CompleteStartSession() is invoked
+    // if one or more MIDI devices including virtual ports are connected.
+    // Just ignore in such cases.
+    if (wait_for_result_)
+      return;
+
+    EXPECT_TRUE(wait_for_port_);

[yhirano, 2015/03/02 04:58:37]

This function is not called on the test thread. I'm not sure if EXPECT_TRUE
works correctly.

[Takashi Toyoshima, 2015/03/02 10:52:17]

I placed AutoLock at line 34 since wait_for_result_ is also needed to be
protected.

[Takashi Toyoshima, 2015/03/02 11:06:21]

Oh, I misunderstood your comment.
I'll check documents and will change to CHECK if needed.

     info_ = info;
     wait_for_port_ = false;
   }
@@ -36,6 +46,7 @@ class FakeMidiManagerClient : public MidiManagerClient {
   void SetOutputPortState(uint32 port_index, MidiPortState state) override {}
 
   void CompleteStartSession(MidiResult result) override {
+    base::AutoLock lock(lock_);
     EXPECT_TRUE(wait_for_result_);
     result_ = result;
     wait_for_result_ = false;
@@ -45,15 +56,25 @@ class FakeMidiManagerClient : public MidiManagerClient {
                        double timestamp) override {}
   void AccumulateMidiBytesSent(size_t size) override {}
 
+  bool GetWaitForResult() {
+    base::AutoLock lock(lock_);
+    return wait_for_result_;
+  }
+
+  bool GetWaitForPort() {
+    base::AutoLock lock(lock_);
+    return wait_for_port_;
+  }
+
   MidiResult WaitForResult() {
-    while (wait_for_result_) {
+    while (GetWaitForResult()) {
       base::RunLoop run_loop;
       run_loop.RunUntilIdle();
     }
     return result_;
   }
   MidiPortInfo WaitForPort() {
-    while (wait_for_port_) {
+    while (GetWaitForPort()) {
       base::RunLoop run_loop;
       run_loop.RunUntilIdle();
     }
@@ -61,6 +82,7 @@ class FakeMidiManagerClient : public MidiManagerClient {
   }
 
  private:
+  base::Lock lock_;
   MidiResult result_;
   bool wait_for_result_;
   MidiPortInfo info_;