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

media/midi/midi_manager_mac_unittest.cc

 // Copyright 2015 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_mac.h"
 
 #include <CoreMIDI/MIDIServices.h>
 
 #include "base/logging.h"
 #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 {
 
 namespace {
 
 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;
   }
   void SetInputPortState(uint32 port_index, MidiPortState state) override {}
   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;
   }
 
   void ReceiveMidiData(uint32 port_index, const uint8* data, size_t size,
                        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();
     }
     return info_;
   }
 
  private:
+  base::Lock lock_;
   MidiResult result_;
   bool wait_for_result_;
   MidiPortInfo info_;
   bool wait_for_port_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeMidiManagerClient);
 };
 
 class MidiManagerMacTest : public ::testing::Test {
  public:
@@ skipping 43 matching lines @@
   EndSession(client.get());
   if (ep)
     MIDIEndpointDispose(ep);
   if (midi_client)
     MIDIClientDispose(midi_client);
 }
 
 }  // namespace
 
 }  // namespace media