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

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

MidiManagerMacTest.MidiNotification was marked in
valgrind/gtest_exclude/media_unittests.gtest_mac.txt.

Remove the entry with some speculative fixes.
Since the problem can not be reproduced with the latest
valgrind on Yosemite.

If this patch is not enough, let's suppress it again.

BUG=462483
TEST=media_unittests --gtest_filter=MidiManagerMacTest.MidiNotification
TEST=valgrind media_unittests --gtest_filter=MidiManagerMacTest.MidiNotification  # does not hangup on yosemite

Committed: https://crrev.com/efd39d0198de405210b6d7d5aa38aafd0bfa462c
Cr-Commit-Position: refs/heads/master@{#318860}

[Takashi Toyoshima, 2015-03-01 18:02:17]

toyoshim@chromium.org changed reviewers:
+ yhirano@chromium.org

[Takashi Toyoshima, 2015-03-01 18:05:25]

toyoshim@chromium.org changed reviewers:
+ bruening@chromium.org

[Takashi Toyoshima, 2015-03-01 18:10:13]

yhirano: can you take a look?
bruening: fyi (memory sheriff who disable this test)

I fixed some potential issues, but I could not set up reproduce environment
because valgrind does not support Yosemite that is the OS X version I use. It
seems that there are no buildbot for valgrind on mac, can I try this fix
speculatively?

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

https://codereview.chromium.org/971593002/diff/40001/media/midi/midi_manager_...
File media/midi/midi_manager_mac_unittest.cc (right):

https://codereview.chromium.org/971593002/diff/40001/media/midi/midi_manager_...
media/midi/midi_manager_mac_unittest.cc:25: base::AutoLock lock(lock_);
It's hard to imagine that anyone can touch a FakeMidiManagerClient that is being
constructed. Can you remove the constructor change?

https://codereview.chromium.org/971593002/diff/40001/media/midi/midi_manager_...
media/midi/midi_manager_mac_unittest.cc:41: EXPECT_TRUE(wait_for_port_);
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:18]

https://codereview.chromium.org/971593002/diff/40001/media/midi/midi_manager_...
File media/midi/midi_manager_mac_unittest.cc (right):

https://codereview.chromium.org/971593002/diff/40001/media/midi/midi_manager_...
media/midi/midi_manager_mac_unittest.cc:25: base::AutoLock lock(lock_);
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.

https://codereview.chromium.org/971593002/diff/40001/media/midi/midi_manager_...
media/midi/midi_manager_mac_unittest.cc:41: EXPECT_TRUE(wait_for_port_);
I placed AutoLock at line 34 since wait_for_result_ is also needed to be
protected.

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

https://codereview.chromium.org/971593002/diff/40001/media/midi/midi_manager_...
File media/midi/midi_manager_mac_unittest.cc (right):

https://codereview.chromium.org/971593002/diff/40001/media/midi/midi_manager_...
media/midi/midi_manager_mac_unittest.cc:41: EXPECT_TRUE(wait_for_port_);
Oh, I misunderstood your comment.
I'll check documents and will change to CHECK if needed.

[Takashi Toyoshima, 2015-03-02 12:11:22]

I could not find documentation that explicitly says we can use gtest macros in
other threads. So, I fixed them to set a flag, and check it on the test thread.

[Takashi Toyoshima, 2015-03-02 16:50:55]

I'll try to run patch set 4 on mavericks tomorrow.

[Takashi Toyoshima, 2015-03-03 07:30:45]

% ./third_party/valgrind/mac_10.7/bin/valgrind
valgrind: Unknown/uninstalled VG_PLATFORM 'amd64-darwin'
And, crbug.com/441425 says valgrind for mac supports only Snow Leopard.
So, I want to give up to check this fix in local, but try submitting to see what
happens there.

Can you take another look?

[yhirano, 2015-03-03 08:41:18]

lgtm

[Takashi Toyoshima, 2015-03-03 09:52:23]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2015-03-03 09:53:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/971593002/60001

[commit-bot: I haz the power, 2015-03-03 10:31:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-03 10:31:33]

Patchset 4 (id:??) landed as
https://crrev.com/efd39d0198de405210b6d7d5aa38aafd0bfa462c
Cr-Commit-Position: refs/heads/master@{#318860}