Renderer can crash during an enumeration in debug builds while devices are added/removed |
||||||
Issue descriptionWhen a page issues a enumerateDevices() or getSources() request, the renderer can crash if devices are added or removed before the request is completed. This happens only on debug builds due to a DCHECK intended to ensure that only on reply is received from the browser for each device type and request. However, if a device is removed or added before the request is completed, it is possible that the browser sends more than one reply. This does not affect the enumeration results, as the latest reply from the browser is always used.
,
Apr 28 2016
,
Apr 28 2016
Thanks for the fix, guidou@! Do you think your fix needs to be merged into M51, or is it ok to wait until it's naturally picked up with M52?
,
May 2 2016
tnakamura: this bug affected only builds with DCHECK enabled, so it's better to just wait for M52.
,
May 2 2016
Thanks! I'll leave it to you to verify since the testing team typically doesn't use DCHECK-enabled builds.
,
May 3 2016
[triage] Does it make sense to add a test for this even though its only for debug builds?
,
May 10 2016
[triage]: I think #1 states that this is very difficult to test automatically, but there should be a unit test right?
,
May 10 2016
This bug required adding or removing a device concurrently with an ongoing enumeration to be triggered. I found that difficult to reliably reproduce with a unit test, while very easy to reproduce using physical devices. I think the issue was small and obvious enough that it's not worth spending time trying to find a reliable way to reproduce it with unit tests.
,
May 16 2016
But you can just call OnDevicesEnumerated like this unit test here: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/media/user_media_client_impl_unittest.cc&sq=package:chromium&type=cs&l=267 and pass in a request with empty input device lists? Is the problem that it's hard to get the class into a state where we have such a request stored?
,
May 17 2016
Unit test going in: https://codereview.chromium.org/1982593003/. Removing MissingTests.
,
May 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5da650481866cad8802589a1178c3537df79f437 commit 5da650481866cad8802589a1178c3537df79f437 Author: guidou <guidou@chromium.org> Date: Tue May 17 16:32:55 2016 Remove bogus DCHECK in content::UserMediaClientImpl and add unit test. The DCHECK verifies that a reply must correspond to a live request, but that is not necessarily true due to device changes that may occur concurrently with the request. BUG= 607179 Review-Url: https://codereview.chromium.org/1982593003 Cr-Commit-Position: refs/heads/master@{#394144} [modify] https://crrev.com/5da650481866cad8802589a1178c3537df79f437/content/renderer/media/user_media_client_impl.cc [modify] https://crrev.com/5da650481866cad8802589a1178c3537df79f437/content/renderer/media/user_media_client_impl_unittest.cc
,
Jun 3 2016
Moving this to Verified because of the addition of a test. Thank you, guidou@! |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by bugdroid1@chromium.org
, Apr 28 2016