Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(471)

Issue 264053002: Web MIDI: introduce pending client count limit to start sessions (Closed)

Created:
6 years, 7 months ago by Takashi Toyoshima
Modified:
6 years, 7 months ago
Reviewers:
yukawa, yhirano
CC:
chromium-reviews, feature-media-reviews_chromium.org
Visibility:
Public.

Description

Web MIDI: introduce pending client count limit to start sessions Accepting infinite number of session start requests may exhaust memory. Too many requests in a short period for finishing platform dependent initialization can be assumed a malicious attack or something like that. This change makes MIDIManager reject new requests once the pending client count reaches the limit. Note that try passed except for slow win_chromium_compile_dbg. NOTRY=true BUG=n/a TEST=media_unittests --gtest_filter='MidiManagerTest.*' Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268687

Patch Set 1 #

Total comments: 2

Patch Set 2 : more comments #

Patch Set 3 : fix memory leak #

Total comments: 10

Patch Set 4 : review #2 #

Patch Set 5 : one more fix #

Total comments: 2

Patch Set 6 : trybots with ScopedVector #

Patch Set 7 : rebase #

Patch Set 8 : INITIALIZATION should be received synchronously, others asynchronously #

Total comments: 2

Patch Set 9 : renames by #8 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+85 lines, -22 lines) Patch
M media/midi/midi_manager.h View 1 2 3 4 5 6 1 chunk +5 lines, -3 lines 0 comments Download
M media/midi/midi_manager.cc View 1 2 3 4 5 6 3 chunks +24 lines, -11 lines 0 comments Download
M media/midi/midi_manager_unittest.cc View 1 2 3 4 5 6 7 8 7 chunks +56 lines, -8 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
Takashi Toyoshima
Hi OWNERS, This is minor topic, so please take a look when you have a ...
6 years, 7 months ago (2014-05-04 09:13:36 UTC) #1
yukawa
https://codereview.chromium.org/264053002/diff/40001/media/midi/midi_manager.cc File media/midi/midi_manager.cc (right): https://codereview.chromium.org/264053002/diff/40001/media/midi/midi_manager.cc#newcode55 media/midi/midi_manager.cc:55: if (too_many_pending_clients_exist) { |too_many_pending_clients_exist| isn't guarded by |clients_lock_|. Is ...
6 years, 7 months ago (2014-05-04 22:44:54 UTC) #2
Takashi Toyoshima
Thanks. Also, I'm looking the compile error that only happens at the last trybot. https://codereview.chromium.org/264053002/diff/40001/media/midi/midi_manager.cc ...
6 years, 7 months ago (2014-05-04 23:28:44 UTC) #3
yukawa
https://codereview.chromium.org/264053002/diff/40001/media/midi/midi_manager.cc File media/midi/midi_manager.cc (right): https://codereview.chromium.org/264053002/diff/40001/media/midi/midi_manager.cc#newcode55 media/midi/midi_manager.cc:55: if (too_many_pending_clients_exist) { Got it. Thanks. https://codereview.chromium.org/264053002/diff/40001/media/midi/midi_manager_unittest.cc File media/midi/midi_manager_unittest.cc ...
6 years, 7 months ago (2014-05-04 23:45:55 UTC) #4
Takashi Toyoshima
Thanks, Now I'm waiting for try results with Patch Set 6. https://codereview.chromium.org/264053002/diff/80001/media/midi/midi_manager_unittest.cc File media/midi/midi_manager_unittest.cc (right): ...
6 years, 7 months ago (2014-05-05 00:59:06 UTC) #5
Takashi Toyoshima
Yep, it works. PTAL again.
6 years, 7 months ago (2014-05-06 01:09:42 UTC) #6
Takashi Toyoshima
Now it is rebased with small changes. PTAL.
6 years, 7 months ago (2014-05-06 23:43:39 UTC) #7
yukawa
LGTM with a minor suggestion. https://codereview.chromium.org/264053002/diff/140001/media/midi/midi_manager_unittest.cc File media/midi/midi_manager_unittest.cc (right): https://codereview.chromium.org/264053002/diff/140001/media/midi/midi_manager_unittest.cc#newcode188 media/midi/midi_manager_unittest.cc:188: ScopedVector<FakeMidiManagerClient> clients; I feel ...
6 years, 7 months ago (2014-05-07 00:07:13 UTC) #8
Takashi Toyoshima
https://codereview.chromium.org/264053002/diff/140001/media/midi/midi_manager_unittest.cc File media/midi/midi_manager_unittest.cc (right): https://codereview.chromium.org/264053002/diff/140001/media/midi/midi_manager_unittest.cc#newcode188 media/midi/midi_manager_unittest.cc:188: ScopedVector<FakeMidiManagerClient> clients; Sound reasonable. Done.
6 years, 7 months ago (2014-05-07 02:09:11 UTC) #9
Takashi Toyoshima
The CQ bit was checked by toyoshim@chromium.org
6 years, 7 months ago (2014-05-07 02:09:16 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/264053002/160001
6 years, 7 months ago (2014-05-07 02:13:57 UTC) #11
Takashi Toyoshima
The CQ bit was unchecked by toyoshim@chromium.org
6 years, 7 months ago (2014-05-07 07:46:23 UTC) #12
Takashi Toyoshima
Now all trybots pass except for win_chromium_compile_dbg, and pending queue for it seems to be ...
6 years, 7 months ago (2014-05-07 07:47:20 UTC) #13
Takashi Toyoshima
The CQ bit was checked by toyoshim@chromium.org
6 years, 7 months ago (2014-05-07 07:48:31 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/264053002/160001
6 years, 7 months ago (2014-05-07 07:52:32 UTC) #15
Takashi Toyoshima
The CQ bit was unchecked by toyoshim@chromium.org
6 years, 7 months ago (2014-05-07 08:01:29 UTC) #16
Takashi Toyoshima
The CQ bit was checked by toyoshim@chromium.org
6 years, 7 months ago (2014-05-07 08:02:07 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/264053002/160001
6 years, 7 months ago (2014-05-07 08:06:09 UTC) #18
commit-bot: I haz the power
6 years, 7 months ago (2014-05-07 08:10:39 UTC) #19
Message was sent while issue was closed.
Change committed as 268687

Powered by Google App Engine
This is Rietveld 408576698