Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2396463002:
Remove dangerous CHECK. Add unit test. (Closed)
|
Created:
4 years, 2 months ago by Max Morin Modified:
4 years, 2 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, miu+watch_chromium.org, audio-mojo-cl_google.com Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove dangerous CHECK. Add unit test.
New unit test failed before changes, passes now.
BUG=652610
Committed: https://crrev.com/17fe3c43c3dcfa4c81486e63548a3f985402f68f
Cr-Commit-Position: refs/heads/master@{#423483}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use bad_message. Add another unit test. #
Total comments: 2
Patch Set 3 : Updated bad message name. #Patch Set 4 : Also update code. #
Total comments: 7
Patch Set 5 : Fix. #
Messages
Total messages: 24 (8 generated)
Description was changed from ========== Remove dangerous CHECK. Add unit test. New unit test failed before changes, passes now. BUG=652610 ========== to ========== Remove dangerous CHECK. Add unit test. New unit test failed before changes, passes now. BUG=652610 ==========
maxmorin@chromium.org changed reviewers: + guidou@chromium.org
Guido: PTAL. I know you are busy right now, so don't worry if you cannot take a look at this right away, it's not time critical :). I added you since you added this CHECK in https://codereview.chromium.org/1323403005/.
https://codereview.chromium.org/2396463002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2396463002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_renderer_host.cc:501: SendErrorMessage(stream_id); Since this should happen only with a bad renderer, wouldn't it be better to use bad_message::ReceivedBadMessage?
On 2016/10/04 09:57:11, Guido Urdaneta wrote: > https://codereview.chromium.org/2396463002/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/media/audio_renderer_host.cc (right): > > https://codereview.chromium.org/2396463002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/media/audio_renderer_host.cc:501: > SendErrorMessage(stream_id); > Since this should happen only with a bad renderer, wouldn't it be better to use > bad_message::ReceivedBadMessage? Agree. PTAL. I added another unit test (for requesting device authorization with bad security origin) as a bonus.
lgtm % nit
lgtm % nit https://codereview.chromium.org/2396463002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2396463002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:502: this, bad_message::ARH_CREATED_WITHOUT_AUTHORIZATION); nit: I think ARH_CREATED_STREAM_WITHOUT_AUTHORIZATION is clearer. Otherwise it looks like it was the ARH the one that was created without authorization.
maxmorin@chromium.org changed reviewers: + asvitkine@chromium.org, chcunningham@chromium.org, nasko@chromium.org
Thanks Guido. chcunningham: PTAL at content/browser/renderer_host/media/*. nasko: PTAL at content/browser/bad_message.h. Alexei: PTAL at tools/metrics/histograms/histograms.xml. Those changes are autogenerated by update_bad_message_reasons.py so they should be fine. https://codereview.chromium.org/2396463002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2396463002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:502: this, bad_message::ARH_CREATED_WITHOUT_AUTHORIZATION); On 2016/10/04 13:05:45, Guido Urdaneta wrote: > nit: I think ARH_CREATED_STREAM_WITHOUT_AUTHORIZATION is clearer. Otherwise it > looks like it was the ARH the one that was created without authorization. Done.
media LGTM % nits https://codereview.chromium.org/2396463002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/2396463002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:285: media::AudioParameters params( I don't think you're using params here. https://codereview.chromium.org/2396463002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:469: kNondefaultDeviceId, url::Origin(GURL(kBadSecurityOrigin))); Do you mean to pass in the bad origin? The method name makes me think the bad origin is decided by the method.
https://codereview.chromium.org/2396463002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2396463002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:72468: + <int value="110" label="WSI_UNEXPECTED_ADD_CHANNEL_REQUEST"/> Any idea what happened here? If the meaning of those messages changed, then looking at UMA data for those values would be misleading - because depending on Chrome version 110 and 111 could mean different things.
https://codereview.chromium.org/2396463002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2396463002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:72468: + <int value="110" label="WSI_UNEXPECTED_ADD_CHANNEL_REQUEST"/> On 2016/10/04 16:53:45, Alexei Svitkine (slow) wrote: > Any idea what happened here? If the meaning of those messages changed, then > looking at UMA data for those values would be misleading - because depending on > Chrome version 110 and 111 could mean different things. These changes are from https://codereview.chromium.org/2119973002, where Ilya Sherman asked a similar question (but didn't get an answer). Maybe reach out to Darin if you're concerned? 139 is from https://codereview.chromium.org/2364633004, where the author simply forgot to run the updater script.
lgtm https://codereview.chromium.org/2396463002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2396463002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:72468: + <int value="110" label="WSI_UNEXPECTED_ADD_CHANNEL_REQUEST"/> On 2016/10/04 20:26:54, Max Morin Chromium wrote: > On 2016/10/04 16:53:45, Alexei Svitkine (slow) wrote: > > Any idea what happened here? If the meaning of those messages changed, then > > looking at UMA data for those values would be misleading - because depending > on > > Chrome version 110 and 111 could mean different things. > > These changes are from https://codereview.chromium.org/2119973002, where Ilya > Sherman asked a similar question (but didn't get an answer). Maybe reach out to > Darin if you're concerned? > > 139 is from https://codereview.chromium.org/2364633004, where the author simply > forgot to run the updater script. Thanks. I just checked and it should be OK since we saw no data for the old values - commented on that CL. Updating this enum SGTM.
https://codereview.chromium.org/2396463002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/2396463002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:285: media::AudioParameters params( On 2016/10/04 15:39:30, chcunningham wrote: > I don't think you're using params here. Done. https://codereview.chromium.org/2396463002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:469: kNondefaultDeviceId, url::Origin(GURL(kBadSecurityOrigin))); On 2016/10/04 15:39:30, chcunningham wrote: > Do you mean to pass in the bad origin? The method name makes me think the bad > origin is decided by the method. Done.
LGTM
The CQ bit was checked by maxmorin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org, chcunningham@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2396463002/#ps80001 (title: "Fix.")
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 ========== Remove dangerous CHECK. Add unit test. New unit test failed before changes, passes now. BUG=652610 ========== to ========== Remove dangerous CHECK. Add unit test. New unit test failed before changes, passes now. BUG=652610 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Remove dangerous CHECK. Add unit test. New unit test failed before changes, passes now. BUG=652610 ========== to ========== Remove dangerous CHECK. Add unit test. New unit test failed before changes, passes now. BUG=652610 Committed: https://crrev.com/17fe3c43c3dcfa4c81486e63548a3f985402f68f Cr-Commit-Position: refs/heads/master@{#423483} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/17fe3c43c3dcfa4c81486e63548a3f985402f68f Cr-Commit-Position: refs/heads/master@{#423483}
Message was sent while issue was closed.
On 2016/10/06 09:06:54, commit-bot: I haz the power wrote: > Patchset 5 (id:??) landed as > https://crrev.com/17fe3c43c3dcfa4c81486e63548a3f985402f68f > Cr-Commit-Position: refs/heads/master@{#423483} Thanks for the fast reviews everyone!
Message was sent while issue was closed.
Description was changed from ========== Remove dangerous CHECK. Add unit test. New unit test failed before changes, passes now. BUG=652610 Committed: https://crrev.com/17fe3c43c3dcfa4c81486e63548a3f985402f68f Cr-Commit-Position: refs/heads/master@{#423483} ========== to ========== Remove dangerous CHECK. Add unit test. New unit test failed before changes, passes now. BUG=652610 Committed: https://crrev.com/17fe3c43c3dcfa4c81486e63548a3f985402f68f Cr-Commit-Position: refs/heads/master@{#423483} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
