Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2170463003:
Web MIDI: Add CHECK()s to ensure MidiManager owned threads are terminated (Closed)
|
Created:
4 years, 5 months ago by Takashi Toyoshima Modified:
4 years, 5 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, toyoshim+midi_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWeb MIDI: Add CHECK()s to ensure MidiManager owned threads are terminated
All MidiManager owned threads are stopped and joined before MidiManager
is destructed. But let me add CHECK()s so that it aborts when these threads
are still running, just in case.
BUG=629954
Committed: https://crrev.com/8fb95e706d70e82d0f9c41fba01b7919d16b8ab3
Cr-Commit-Position: refs/heads/master@{#407424}
Patch Set 1 #
Total comments: 6
Patch Set 2 : review #11 #
Messages
Total messages: 22 (11 generated)
Description was changed from ========== Web MIDI: Add CHECK()s to ensure MidiManager owned threads are terminated All MidiManager owned threads are stopped and joined before MidiManager is destructed. But let me add CHECK()s so that it aborts when these threads are still running, just in case. BUG=629954 ========== to ========== Web MIDI: Add CHECK()s to ensure MidiManager owned threads are terminated All MidiManager owned threads are stopped and joined before MidiManager is destructed. But let me add CHECK()s so that it aborts when these threads are still running, just in case. BUG=629954 ==========
toyoshim@chromium.org changed reviewers: + yhirano@chromium.org
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL.
toyoshim@chromium.org changed reviewers: + agoode@chromium.org
lgtm
https://codereview.chromium.org/2170463003/diff/1/media/midi/midi_manager.cc File media/midi/midi_manager.cc (right): https://codereview.chromium.org/2170463003/diff/1/media/midi/midi_manager.cc#... media/midi/midi_manager.cc:63: CHECK(finalized_); Do you need to take lock_ here? https://codereview.chromium.org/2170463003/diff/1/media/midi/midi_manager_als... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/2170463003/diff/1/media/midi/midi_manager_als... media/midi/midi_manager_alsa.cc:161: // Extra DCHECK to verify all members are already reset. You could change all the DCHECK to CHECK here. https://codereview.chromium.org/2170463003/diff/1/media/midi/midi_manager_als... media/midi/midi_manager_alsa.cc:169: CHECK(!send_thread_.IsRunning()); I don't think you can be sure of the result of IsRunning() if called from a different thread than calls Start().
https://codereview.chromium.org/2170463003/diff/1/media/midi/midi_manager.cc File media/midi/midi_manager.cc (right): https://codereview.chromium.org/2170463003/diff/1/media/midi/midi_manager.cc#... media/midi/midi_manager.cc:63: CHECK(finalized_); Good point. I'll fix this. https://codereview.chromium.org/2170463003/diff/1/media/midi/midi_manager_als... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/2170463003/diff/1/media/midi/midi_manager_als... media/midi/midi_manager_alsa.cc:169: CHECK(!send_thread_.IsRunning()); IsRunning() is safe to be called from any thread. Of course, Start() and Stop() could be called any time. But in our case, these are called only on the IO thread in StartInitialization() and Finalize(), and the IO thread stops before destructing MidiManager and BrowserMainLoop. So this check works in practice.
lgtm
https://codereview.chromium.org/2170463003/diff/1/media/midi/midi_manager_als... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/2170463003/diff/1/media/midi/midi_manager_als... media/midi/midi_manager_alsa.cc:161: // Extra DCHECK to verify all members are already reset. On 2016/07/21 15:40:30, Adam Goode wrote: > You could change all the DCHECK to CHECK here. Done.
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org, agoode@chromium.org Link to the patchset: https://codereview.chromium.org/2170463003/#ps20001 (title: "review #11")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Web MIDI: Add CHECK()s to ensure MidiManager owned threads are terminated All MidiManager owned threads are stopped and joined before MidiManager is destructed. But let me add CHECK()s so that it aborts when these threads are still running, just in case. BUG=629954 ========== to ========== Web MIDI: Add CHECK()s to ensure MidiManager owned threads are terminated All MidiManager owned threads are stopped and joined before MidiManager is destructed. But let me add CHECK()s so that it aborts when these threads are still running, just in case. BUG=629954 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Web MIDI: Add CHECK()s to ensure MidiManager owned threads are terminated All MidiManager owned threads are stopped and joined before MidiManager is destructed. But let me add CHECK()s so that it aborts when these threads are still running, just in case. BUG=629954 ========== to ========== Web MIDI: Add CHECK()s to ensure MidiManager owned threads are terminated All MidiManager owned threads are stopped and joined before MidiManager is destructed. But let me add CHECK()s so that it aborts when these threads are still running, just in case. BUG=629954 Committed: https://crrev.com/8fb95e706d70e82d0f9c41fba01b7919d16b8ab3 Cr-Commit-Position: refs/heads/master@{#407424} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8fb95e706d70e82d0f9c41fba01b7919d16b8ab3 Cr-Commit-Position: refs/heads/master@{#407424} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
