New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 834581 link

Starred by 7 users

Webcam device monitoring on Mac broken

Reported by aisn...@gmail.com, Apr 19 2018

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.117 Safari/537.36

Steps to reproduce the problem:
1. open page: https://webrtc.github.io/samples/src/content/devices/input-output/
2. check video source dropdown list
3. plug in new USB web camera
4. reload the page
5. check the video source dropdown list again

What is the expected behavior?
should see the new device there

What went wrong?
no new device there until restart the chrome

Did this work before? Yes 65

Does this work in other browsers? N/A

Chrome version: 66.0.3359.117  Channel: n/a
OS Version: OS X 10.12.6
Flash Version:
 
Labels: Needs-Bisect Needs-Triage-M66
Cc: sindhu.chelamcherla@chromium.org
Labels: Triaged-ET TE-NeedsTriageFromHYD
Unable to reproduce this issue on 66.0.3359.117 using Mac with inbuilt camera i.e; Observing dropdown even after reloading page. As ET team do not have USB web camera forwarding this bug to Inhouse and adding TE-NeedsTriageFromHYD. Could someone from Inhouse please have a look at this issue.

Thanks!

Comment 3 by aisn...@gmail.com, Apr 19 2018

have to plugin the USB web camera. On new MBP, maybe you need the type-c to USB adapter.
Cc: kkaluri@chromium.org
Labels: -TE-NeedsTriageFromHYD Needs-Feedback
Unable to reproduce this issue on Mac 10.13.3 with Chrome Stable #66.0.3359.117, M65, M60

Steps Followed :
1. Navigate to https://webrtc.github.io/samples/src/content/devices/input-output/
2. In built webcam is detected
3. Insert USB WebCam
4. Click on the Video Source dropdown

Expected:
It should detect the USB WebCam

Actual :
1. It not detected the USB WebCam
2. On refreshing webpage, on clicking Video source dropdown USB webcam is detected
3. Seeing the same behaviour on FireFox browser

Attaching the screen-cast for reference

aisnote@ Could you please look into it and let us know your observations. If Possible please help us with screen-cast of expected behaviour for our further triage.

Thank You....
834581-M65.mp4
6.0 MB View Download
834581-M60.mp4
2.4 MB View Download

Comment 5 by aisn...@gmail.com, Apr 20 2018

please make sure you used the Chrome 66.0.3359.117 version.
This issue only happened on Chrome 66 on MacOS.
Previous version is ok.

Project Member

Comment 6 by sheriffbot@chromium.org, Apr 20 2018

Labels: -Needs-Feedback
Thank you for providing more feedback. Adding the requester to the cc list.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 7 by guidou@chromium.org, Apr 20 2018

Components: -Blink>WebRTC Blink>GetUserMedia
Labels: -Pri-2 Pri-1
Owner: guidou@chromium.org
Status: Assigned (was: Unconfirmed)
Labels: TE-NeedsTriageFromHYD
As per comment#5 forwarding to Inhouse team. Could someone from Inhouse please check issue on 66.0.3359.117 and provide a screencast.

Thanks!

Comment 9 by guidou@chromium.org, Apr 23 2018

Labels: -Needs-Triage-M66 M66
I can reproduce the issue. The issue is flaky. Sometimes, the device plugging or unplugging is detected, but sometimes it isn't.
As a quick fix, I will disable caching of enumeration results on Mac, which will make it work as in M65.
Labels: -Needs-Bisect -TE-NeedsTriageFromHYD
https://chromium-review.googlesource.com/c/chromium/src/+/1023855 will fix this issue by disabling the internal cache of video-device enumeration results.
Filed  bug 835765  to track fixing the video device monitor on Mac.
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40ba1dc55e3d37b4a813fb6968a02deab382be3d

commit 40ba1dc55e3d37b4a813fb6968a02deab382be3d
Author: Guido Urdaneta <guidou@chromium.org>
Date: Mon Apr 23 14:23:53 2018

Disable caching of video-device enumerations on Mac.

There is an issue with the Mac video device monitor that is causing
cache inconsistencies.

Bug:  834581 
Change-Id: Idc1a0d953509fee7787fb07d19376d4800f76939
Reviewed-on: https://chromium-review.googlesource.com/1023855
Reviewed-by: Henrik Boström <hbos@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552689}
[modify] https://crrev.com/40ba1dc55e3d37b4a813fb6968a02deab382be3d/content/browser/renderer_host/media/media_devices_manager.cc

Status: Fixed (was: Assigned)
Labels: Merge-Request-67 Merge-Request-66
Project Member

Comment 15 by sheriffbot@chromium.org, Apr 23 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Thanks Guidou@ - how safe is this merge overall?

Comment 17 by aisn...@gmail.com, Apr 23 2018

so when I can get the new Chrome with this fixed?  thanks.

Comment 18 by guidou@google.com, Apr 23 2018

abdulsyed@: the merge is quite safe. A noop on non-Mac platforms and a one-liner on Mac.
Labels: -Merge-Request-67 Merge-Approved-67
Approving this for M67. Branch: 3396
Project Member

Comment 20 by bugdroid1@chromium.org, Apr 23 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/320e98d8a631bf184cab784c84b7c6c6bc6e0cd2

commit 320e98d8a631bf184cab784c84b7c6c6bc6e0cd2
Author: Guido Urdaneta <guidou@chromium.org>
Date: Mon Apr 23 21:42:52 2018

Disable caching of video-device enumerations on Mac.

There is an issue with the Mac video device monitor that is causing
cache inconsistencies.

Bug:  834581 
Change-Id: Idc1a0d953509fee7787fb07d19376d4800f76939
Reviewed-on: https://chromium-review.googlesource.com/1023855
Reviewed-by: Henrik Boström <hbos@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#552689}(cherry picked from commit 40ba1dc55e3d37b4a813fb6968a02deab382be3d)
Reviewed-on: https://chromium-review.googlesource.com/1024854
Reviewed-by: Abdul Syed <abdulsyed@google.com>
Commit-Queue: Abdul Syed <abdulsyed@google.com>
Cr-Commit-Position: refs/branch-heads/3396@{#243}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/320e98d8a631bf184cab784c84b7c6c6bc6e0cd2/content/browser/renderer_host/media/media_devices_manager.cc

Labels: -merge-merged-3396 Merge-Approved-67
I've merged this to M67 branch. let's look at how this looks in dev tomorrow, and we can then review for stable merge.
Labels: -Merge-Approved-67 merge-merged-3396
Thank you Abdul for M67 merge - Removing "Merge-Approved-67"and applying "Merge-merged-3396" label.
Cc: abdulsyed@chromium.org
Labels: Needs-Feedback
Since TE doesn't able to repro this issue on the reported version,
As per comment #9 requesting dev guidou@ to verify this issue on #67.0.3396.18 and update the behaviour.

Thank You...
I verified that the issue is fixed in Canary 68.0.3406.0.

However, the problem still reproduces on Dev 67.0.3396.18.
I also tried 66.0.3359.117 with a command-line parameter that has an effect similar to the patch and the problem still reproduces. Therefore, I recommend not merging to M66 until we know more about the root cause of the problem.

I suspect there is some other bug that was fixed in M68 that, combined with r552689, resolves the issue.
I will try a bisect to see if I can find the root cause in M66 and M67.
Labels: -Merge-Review-66 Merge-Rejected-66
Rejecting merge per #25. This should be a very rare scenario. 
Status: Assigned (was: Fixed)
Cc: niklase@chromium.org
After bisecting, I found out that the root cause for this problem is r535188.

Comment 30 Deleted

Given this root cause, the fix in r552689, while harmless, does not fix the issue.
Since this is appears to be working well in M68, I'll try to find the CL that actually fixed it to see if we can merge that instead.
Cc: guidou@chromium.org
Owner: ----
This is actually still broken in the latest Canary.
A more reliable way to test this is to go to https://guidou.github.io/enumdemo4.html and add and remove a camera multiple times.
In good builds, the list is updated correctly and automatically.
In builds after r535188 the list sometimes is not updated correctly for cameras.
r552689 helps mitigate, but it is not a proper fix and should actually be reverted.

Cc: tommi@chromium.org rsesek@chromium.org
Components: -Blink>GetUserMedia Internals>Core
Status: Untriaged (was: Assigned)
I confirmed that reverting r535188 fixes the issue in a local build.
Since rsesek@ and mark@, who respectively wrote and reviewed the culprit CL are both OOO, changing the component and marking the bug as untriaged.

 Issue 835765  has been merged into this issue.
 Issue 837587  has been merged into this issue.
Owner: ellyjo...@chromium.org
Status: Assigned (was: Untriaged)
ellyjones@: Since rsesek@ is OOO can you help us triage this or decide if reverting r535188 and merging the revert to M66 and M67 is a good course of action?

For reference, the class doing the monitoring of video devices is DeviceMonitorMac, https://cs.chromium.org/chromium/src/media/device_monitors/device_monitor_mac.mm
Cc: googyl@google.com
Reverting r535188 is risky - it is quite a foundational change and only rsesek@ and mmentovai@ really have context on it. Do we have a good understanding of why r535188 would have caused this failure?
Cc: ellyjo...@chromium.org
Labels: ReleaseBlock-Stable
Owner: rsesek@chromium.org
The regression is worse than originally reported since it affects not only enumerations and the devicechange event, but also the state and events of video MediaStreamTracks.

Assigning to rsesek@ based on #39.
 Issue 836721  has been merged into this issue.
Project Member

Comment 42 by bugdroid1@chromium.org, May 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d187c36a9676b7f88c6519bb16fc287f9b191967

commit d187c36a9676b7f88c6519bb16fc287f9b191967
Author: Guido Urdaneta <guidou@chromium.org>
Date: Wed May 02 11:04:13 2018

Revert "Disable caching of video-device enumerations on Mac."

This reverts commit 40ba1dc55e3d37b4a813fb6968a02deab382be3d.

Reason for revert: Does not fix the problem.

Original change's description:
> Disable caching of video-device enumerations on Mac.
>
> There is an issue with the Mac video device monitor that is causing
> cache inconsistencies.
>
> Bug:  834581 
> Change-Id: Idc1a0d953509fee7787fb07d19376d4800f76939
> Reviewed-on: https://chromium-review.googlesource.com/1023855
> Reviewed-by: Henrik Boström <hbos@chromium.org>
> Commit-Queue: Guido Urdaneta <guidou@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#552689}

TBR=hbos@chromium.org,guidou@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  834581 
Change-Id: I7e46c1a336d03511d97b2c7a02192e3474c9693a
Reviewed-on: https://chromium-review.googlesource.com/1033732
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555347}
[modify] https://crrev.com/d187c36a9676b7f88c6519bb16fc287f9b191967/content/browser/renderer_host/media/media_devices_manager.cc

Labels: -merge-merged-3396 Merge-Request-67
adbulsyed@: Given that we now know the root cause and that the patch r550428 does not fix the issue (it merely mitigates it in some cases), I'm requesting merge of r555347 (which reverts r550428) to M67.
Summary: Webcam device monitoring on Mac broken (was: Can not enum device list on demand while plugin or plug out new USB web camera)
This is regressed in M66. How critical and safe  to merge r555347 (which reverts r550428) to M67? 
It is safe. It is not critical, but it improves performance by reducing low-level device enumerations and it should also help mitigate issues like bug 806037, which are caused by more frequent device low-level enumerations.
Labels: -Merge-Request-67 Merge-Approved-67
Approving merge for r555347  to M67 branch 3396 based on comment #46. Pls merge ASAP. Thank you.
Project Member

Comment 48 by bugdroid1@chromium.org, May 3 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/411f2eb18905bc6fbea8034564d2494f69680009

commit 411f2eb18905bc6fbea8034564d2494f69680009
Author: Guido Urdaneta <guidou@chromium.org>
Date: Thu May 03 08:37:21 2018

Revert "Disable caching of video-device enumerations on Mac."

This reverts commit 40ba1dc55e3d37b4a813fb6968a02deab382be3d.

Reason for revert: Does not fix the problem.

Original change's description:
> Disable caching of video-device enumerations on Mac.
>
> There is an issue with the Mac video device monitor that is causing
> cache inconsistencies.
>
> Bug:  834581 
> Change-Id: Idc1a0d953509fee7787fb07d19376d4800f76939
> Reviewed-on: https://chromium-review.googlesource.com/1023855
> Reviewed-by: Henrik Boström <hbos@chromium.org>
> Commit-Queue: Guido Urdaneta <guidou@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#552689}

TBR=hbos@chromium.org,guidou@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  834581 
Change-Id: I7e46c1a336d03511d97b2c7a02192e3474c9693a
Reviewed-on: https://chromium-review.googlesource.com/1033732
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#555347}(cherry picked from commit d187c36a9676b7f88c6519bb16fc287f9b191967)
Reviewed-on: https://chromium-review.googlesource.com/1041765
Cr-Commit-Position: refs/branch-heads/3396@{#453}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/411f2eb18905bc6fbea8034564d2494f69680009/content/browser/renderer_host/media/media_devices_manager.cc

 Issue 838879  has been merged into this issue.
Thanks Elly. Yes, reverting r535188 would have been a high-risk change.

The issue is most likely because the code doing device enumeration was implicitly depending on a thread being backed by a CFRunLoop MessagePump. All that needs to be done is to ensure the thread doing the device enumeration has a TYPE_UI MessagePump. See a similar fix here: https://chromium-review.googlesource.com/c/chromium/src/+/1029113/3/content/browser/media/capture/desktop_capture_device.cc

Also, it's unfortunate that this wasn't caught by any automated test coverage.
rsesek@: I will try a similar patch and will ask you to review it.

The reason this wasn't caught by automated tests is that this issue can only be reproduced if by adding and/or removing webcams from the system, and we do not have a mechanism to emulate this.
Owner: guidou@chromium.org
Hm, I maybe spoke too soon in #50 if this is flaky rather than deterministically failing.

I've been reproing this on the test site in #32 Chromium revision 6bc702ce4e6080adf5fff8e998e5138de88f1f7c. I think there may be some kind of racy-ness issue in device_monitor_mac.mm. What I'm doing is

1) Launching Chromium on my Mac pro (no built-in video capture device) without a webcam attached
2) Plugging in an external USB webcam
3) Quickly removing the USB webcam after the device appears on enumdemo4.html

I notice that the "videoinput" device shows up after (2) and the entry remains stuck on the webpage in (3). In the debugger, I added a breakpoint on SuspendObserverDelegate::DoOnDeviceChanged and then had the debugger print the passed NSArray of devices, while also running with --vmodule=device_monitor_mac=1. This is what I observe in the log:


===== Launching and plugging in the device:

Process 47374 launched: './out/debug/Chromium.app/Contents/MacOS/Chromium' (x86_64)
[47374:775:0507/171644.308290:VERBOSE1:device_monitor_mac.mm(444)] Monitoring via AVFoundation
(lldb)  po devices
<__NSArrayI 0x15a168220>(
<AVCaptureHALDevice: 0x1001c2fa0 [Built-in Line Input][AppleHDAEngineInput:1B,0,1,1:4]>,
<AVCaptureHALDevice: 0x1001a38e0 [Built-in Digital Input][AppleHDAEngineInput:1B,0,1,2:5]>,
<AVCaptureDALDevice: 0x15a1d3700 [HD Pro Webcam C920][0xfa400000046d082d]>
)


(lldb)  c
Process 47374 resuming

Command #2 'c' continued the target.
[47374:775:0507/171704.925378:VERBOSE1:device_monitor_mac.mm(99)] Video device has been added, id: AppleHDAEngineInput:1B,0,1,1:4
[47374:775:0507/171704.925467:VERBOSE1:device_monitor_mac.mm(99)] Video device has been added, id: AppleHDAEngineInput:1B,0,1,2:5
[47374:775:0507/171704.925513:VERBOSE1:device_monitor_mac.mm(99)] Video device has been added, id: 0xfa400000046d082d
[47374:775:0507/171704.925557:VERBOSE1:device_monitor_mac.mm(109)] Video device has been removed, id: invalid
(lldb)  po devices
<__NSArrayI 0x159e752b0>(
<AVCaptureHALDevice: 0x1001c2fa0 [Built-in Line Input][AppleHDAEngineInput:1B,0,1,1:4]>,
<AVCaptureHALDevice: 0x1001a38e0 [Built-in Digital Input][AppleHDAEngineInput:1B,0,1,2:5]>,
<AVCaptureHALDevice: 0x15a0c16a0 [HD Pro Webcam C920][AppleUSBAudioEngine:Unknown Manufacturer:HD Pro Webcam C920:XXX]>,
<AVCaptureDALDevice: 0x15a1d3700 [HD Pro Webcam C920][0xfa400000046d082d]>
)


(lldb)  c
Process 47374 resuming

Command #2 'c' continued the target.
[47374:775:0507/171705.438498:VERBOSE1:device_monitor_mac.mm(99)] Video device has been added, id: AppleUSBAudioEngine:Unknown Manufacturer:HD Pro Webcam C920:XXX


===== Unplugging the device:

(lldb)  po devices
<__NSArrayI 0x168601940>(
<AVCaptureHALDevice: 0x1001c2fa0 [Built-in Line Input][AppleHDAEngineInput:1B,0,1,1:4]>,
<AVCaptureHALDevice: 0x1001a38e0 [Built-in Digital Input][AppleHDAEngineInput:1B,0,1,2:5]>,
<AVCaptureHALDevice: 0x15a0c16a0 [HD Pro Webcam C920][AppleUSBAudioEngine:Unknown Manufacturer:HD Pro Webcam C920:XXX]>
)


(lldb)  c
Process 47374 resuming

Command #2 'c' continued the target.
[47374:775:0507/171707.112047:VERBOSE1:device_monitor_mac.mm(109)] Video device has been removed, id: 0xfa400000046d082d
2018-05-07 17:17:07.112320-0400 Chromium[47374:8251808] [AudioHAL_Client] AudioHardware.cpp:1200:AudioObjectRemovePropertyListener:  AudioObjectRemovePropertyListener: no object with given ID 74
2018-05-07 17:17:07.112378-0400 Chromium[47374:8251808] [AudioHAL_Client] AudioHardware.cpp:1200:AudioObjectRemovePropertyListener:  AudioObjectRemovePropertyListener: no object with given ID 74
2018-05-07 17:17:07.112403-0400 Chromium[47374:8251808] [AudioHAL_Client] AudioHardware.cpp:1200:AudioObjectRemovePropertyListener:  AudioObjectRemovePropertyListener: no object with given ID 74
2018-05-07 17:17:07.112423-0400 Chromium[47374:8251808] [AudioHAL_Client] AudioHardware.cpp:1200:AudioObjectRemovePropertyListener:  AudioObjectRemovePropertyListener: no object with given ID 74
(lldb)  po devices
<__NSArrayI 0x1002b5cb0>(
<AVCaptureHALDevice: 0x1001c2fa0 [Built-in Line Input][AppleHDAEngineInput:1B,0,1,1:4]>,
<AVCaptureHALDevice: 0x1001a38e0 [Built-in Digital Input][AppleHDAEngineInput:1B,0,1,2:5]>
)


(lldb)  c
Process 47374 resuming

Command #2 'c' continued the target.
[47374:775:0507/171707.197157:VERBOSE1:device_monitor_mac.mm(109)] Video device has been removed, id: AppleUSBAudioEngine:Unknown Manufacturer:HD Pro Webcam C920:5F72777F:3


As noted above, the system is not reporting the webcam as attached via [AVCaptureDevice devices], but it is still showing on the demo page. So it looks like the system is indeed broadcasting the device added/removed notifications correctly, so maybe something in DeviceMonitorMac is not correctly handling it.

A couple of odd lines in the log are the "AudioObjectRemovePropertyListener: no object with given ID 74" and "Video device has been removed, id: invalid" -- not sure if those are typical.

But this seems like a data race because if I pause the debugger for long enough in DoOnDeviceChanged() I cannot get the issue to repro. But if I don't pause the debugger in that method, I can get it to repro with fairly regularly. That would mean my MessagePumpDefault change just tickled an existing threading bug, rather than actually introducing one.
If I run with --disable-features=MediaDevicesSystemMonitorCaching, I can still reproduce the videoinput device getting stuck after step (3) above. But with that feature disabled, explicitly refreshing the video list via the button on the page removes the stale entry.

So this does seem to be some kind of threading/cache invalidation issue.
Cc: chfremer@chromium.org
I'll look more into it. Like you say, the device monitoring code appears to still be correct, so the bug is probably in the lower-level device-capture code that performs device enumerations.
I discovered that the bug does not reproduce when passing --disable-features=MojoVideoCapture
Will dig further.
Since #56 indicates that this might be related to the video capture service, I will take a look at this today as well.
The issue seems to be that the main thread of the utility process for the video-capture service is now of type DEFAULT instead of type UI.
Apparently this causes issues with the AVFoundation code for enumerating video devices.
The device monitor works fine because it uses the UI thread of the browser process.
It looks like there's a flag you can pass to the utility process to instruct it to start with a main-thread MessageLoop of TYPE_UI: https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_client.cc?l=2070&rcl=b4428e582e2a1e0f82b8a671646bcb4acee447a5
Project Member

Comment 60 by bugdroid1@chromium.org, May 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/191635e56f362891dd7ee750598679e3eb27cab6

commit 191635e56f362891dd7ee750598679e3eb27cab6
Author: Guido Urdaneta <guidou@chromium.org>
Date: Wed May 09 16:00:53 2018

Use a message loop of type UI on the main thread of video-capture service

AVFoundation calls made by the video-capture service on its main thread
require a CFRunLoop which is provided by a UI message loop.
This fixes a number of regressions related to detection of plugged/unplugged
devices that appeared when the default message loops on Mac were changed from
CFRunLoop to Default.

Bug:  834581 
Change-Id: If565f400e52407c2f71e32f93e9472556051bad1
Reviewed-on: https://chromium-review.googlesource.com/1049931
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557202}
[modify] https://crrev.com/191635e56f362891dd7ee750598679e3eb27cab6/chrome/browser/BUILD.gn
[modify] https://crrev.com/191635e56f362891dd7ee750598679e3eb27cab6/chrome/browser/DEPS
[modify] https://crrev.com/191635e56f362891dd7ee750598679e3eb27cab6/chrome/browser/chrome_content_browser_client.cc

Labels: Merge-Request-67 Merge-Request-66
Labels: -Merge-Rejected-66 -merge-merged-3396
Requesting merge of r557202, which is a complete fix, to both M67 and M66.
Project Member

Comment 63 by sheriffbot@chromium.org, May 9 2018

Labels: -Merge-Request-67 Merge-Review-67
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
I don't feel comfortable taking one more merge in r557202 as M67 stable promotion is coming soon, see the reason below:

* Per Comment 46, this is not critical. 
* we already took merges in for M67 at comment #20 and #48.
* This is regressed in M66, not M67 regression
* Also r557202 is not yet baked/verified in canary yet. 




 

Labels: -Merge-Request-66 Merge-Rejected-66
Labels: M-67
NextAction: 2018-05-10

Comment 67 by kbr@chromium.org, May 10 2018

Blockedon: 841548
It is critical to merge r557202 to M67 and, IMO, also to M66.
This regression is much more serious than we initially thought.
Monitoring issues when plugging/unplugging webcams is just the easiest way to show the problem, but this potentially affects all video capture in a flaky, hard-to-diagnose way. 
Without r557202, an assumption required by all the low-level Mac APIs we use for video capture does not hold in M66 and M67.
The NextAction date has arrived: 2018-05-10
Labels: TE-Verified-68.0.3426.0 TE-Verified-M68
Verified this issue on Mac 10.13.3 with chrome #68.0.3426.0 and observed the fix is working as expected in comment #0. Hence adding TE-Verified labels

Attaching the screen-cast for reference
834581-M68.mp4
1.9 MB View Download
Blockedon: -841548
I also verified with Canary 68.0.3426.0 using the procedure in #32.

Labels: -Merge-Review-67 Merge-Approved-67
Approving merge to M67 branch 3396 based on comment #68, #70 and #72. Please merge ASAP. Thank you.
Project Member

Comment 74 by bugdroid1@chromium.org, May 10 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/648bde0a9cad0b04e947e2bf863fe99f1a7b8a2b

commit 648bde0a9cad0b04e947e2bf863fe99f1a7b8a2b
Author: Guido Urdaneta <guidou@chromium.org>
Date: Thu May 10 15:11:36 2018

Use a message loop of type UI on the main thread of video-capture service

AVFoundation calls made by the video-capture service on its main thread
require a CFRunLoop which is provided by a UI message loop.
This fixes a number of regressions related to detection of plugged/unplugged
devices that appeared when the default message loops on Mac were changed from
CFRunLoop to Default.

Bug:  834581 
Change-Id: If565f400e52407c2f71e32f93e9472556051bad1
Reviewed-on: https://chromium-review.googlesource.com/1049931
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#557202}(cherry picked from commit 191635e56f362891dd7ee750598679e3eb27cab6)
Reviewed-on: https://chromium-review.googlesource.com/1053967
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#544}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/648bde0a9cad0b04e947e2bf863fe99f1a7b8a2b/chrome/browser/BUILD.gn
[modify] https://crrev.com/648bde0a9cad0b04e947e2bf863fe99f1a7b8a2b/chrome/browser/DEPS
[modify] https://crrev.com/648bde0a9cad0b04e947e2bf863fe99f1a7b8a2b/chrome/browser/chrome_content_browser_client.cc

Status: Verified (was: Assigned)
Labels: -Merge-Rejected-66 Merge-Request-66
Requesting merge to M66 based on comment #68.
Requesting a postmortem for this. Please see go/chrome-postmortems for the process to follow as this is broken on M66 which is currently in stable.

Comment 79 by tommi@chromium.org, May 14 2018

chfremer - can you start the post mortem?
govind - does comment #78 mean that merge to 66 is approved?
Re #79: No, pls wait for abdulsyed@ (M66 Release TPM) M66 merge review and approval. AFAIK, there is no M66 respin plan. Thank you.
In an offline discussion with abdulsyed@, the conclusion was that merging this to M66 would be considered only if there were a new M66 respin, but there is currently no plan for such a respin.

Comment 82 by aisn...@gmail.com, May 14 2018

what's the final decision for this issue?
I wondering when we can get this issue fixed.
Labels: -Merge-Request-66 Merge-Rejected-66
The fix is already included in M67 Beta, and will be targeted for Stable when M67 is promoted to stable (2 weeks). 
Labels: TE-Verified-M67 TE-Verified-67.0.3396.48
Verified this issue on Mac 10.13.3 with chrome #67.0.3396.48 and observed the fix is working as expected in comment #0. Hence adding TE-Verified labels

I created the post mortem: go/postmortem101280
Do I need to do anything before sending it out?

Sign in to add a comment