New issue
Advanced search Search tips

Issue 795340 link

Starred by 1 user

Issue metadata

Status: Duplicate
Merged: issue 796889
Owner:
Closed: Oct 23
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

Chromium DBG building will crash at starting desktop capture picker on Mac OSX

Project Member Reported by braveyao@chromium.org, Dec 15 2017

Issue description

Chrome Version: chromium building on ToT
OS: OSX 10.12.6

What steps will reproduce the problem?
(1) build chromium on Mac with ToT, setting is_debug=true in args.gn
(2) prepare for desktop capture test, either by adding sample extension at /src/chrome/common/extensions/docs/examples/api/desktopCapture, or using Hangout or https://appear.in
(3) try to launch the picker window

What is the expected result?
picker can be launched and desktop capture can be started

What happens instead?
chromium crashes.

Please use labels and text to provide additional information.
The call stack is as enclosed.
So far only see this problem on OSX.
It's introduced by cl https://chromium-review.googlesource.com/c/chromium/src/+/793832

For graphics-related bugs, please copy/paste the contents of the about:gpu
page at the end of this report.

 
stack.txt
11.0 KB View Download

Comment 1 by tommi@chromium.org, Jan 8 2018

Cc: sprang@chromium.org
Hey Erik - I looked at the ScreenCapturerMac + DesktopConfigurationMonitor implementations and am wondering if we could avoid waiting on the event from the main thread. One thought I had was whether we could make it so that the configuration is always (and only) changed on the main thread, then we don't need synchronization to read it.

However, I haven't managed to fully understand the synchronization model in DesktopConfigurationMonitor and how the Lock/Unlock methods are meant to be used. It seems to me like there are two ways for the event to become "Locked / Unlocked" (since the event is not a lock, the terminology is throwing me off a bit). It can be locked/unlocked when we get callbacks from the system, but also when external code calls the Lock/Unlock methods. At least from my inspection, it doesn't look like these two are protected from stepping on each other's toes.  Do you know the code?
Cc: guidou@chromium.org
I do not know this code, other than I had an issues with it that guido@ helped me out with:
https://chromium-review.googlesource.com/c/chromium/src/+/809005
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 17 2018

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/39d93da776c2ff5ce3bc78eaa8f81b6eb42d7a2a

commit 39d93da776c2ff5ce3bc78eaa8f81b6eb42d7a2a
Author: Tommi <tommi@webrtc.org>
Date: Wed Jan 17 14:36:36 2018

Add ScopedAllowBaseSyncPrimitives to WebRTC for compatibility with Chromium.
This will be overridden in Chromium in a future CL and following that,
used inside of WebRTC as needed while issues with blocking events are
being fixed.

Bug:  chromium:796889 ,  chromium:795340 
Change-Id: I098a03d9bad621f1349ef483b82e4d8e786a8a75
Reviewed-on: https://webrtc-review.googlesource.com/40140
Reviewed-by: Kári Helgason <kthelgason@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21659}
[modify] https://crrev.com/39d93da776c2ff5ce3bc78eaa8f81b6eb42d7a2a/rtc_base/event.h

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 30 2018

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

commit 1eb1e65c9f12d28f522f619ead1fdd841f764273
Author: Tommi <tommi@chromium.org>
Date: Tue Jan 30 16:55:26 2018

Add rtc::ScopedAllowBaseSyncPrimitives for WebRTC.

This class links together Chromium's ScopedAllowBaseSyncPrimitives and WebRTCs.
Now that WebRTC is starting to use Chromium's sync primitives, this allows
us to continue reduce locking inside of WebRTC that can cause slowdown
on Chromium threads, while guarding against regressions.

Bug:  796889 ,  795340 
Change-Id: Icd71c13e04433aa67e3f22f5d510aa724b413fa9
Reviewed-on: https://chromium-review.googlesource.com/871631
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Tommi <tommi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532920}
[modify] https://crrev.com/1eb1e65c9f12d28f522f619ead1fdd841f764273/base/threading/thread_restrictions.h
[modify] https://crrev.com/1eb1e65c9f12d28f522f619ead1fdd841f764273/third_party/webrtc_overrides/rtc_base/event.h

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 30 2018

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/0a3593c25dbc96b7d66d17ab77fc9984ab2bf245

commit 0a3593c25dbc96b7d66d17ab77fc9984ab2bf245
Author: Tommi <tommi@webrtc.org>
Date: Tue Jan 30 17:56:36 2018

Add ScopedAllowBaseSyncPrimitives for DesktopConfigurationMonitor.
This is a temporary measure until the synchronization method
used in the class, gets fixed.

Bug:  chromium:796889 ,  chromium:795340 
Change-Id: Ie3d394ae42f005e8e0f353d04ea9c1d053ea9fd2
Reviewed-on: https://webrtc-review.googlesource.com/40460
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21812}
[modify] https://crrev.com/0a3593c25dbc96b7d66d17ab77fc9984ab2bf245/modules/desktop_capture/mac/desktop_configuration_monitor.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Jan 31 2018

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/70294c8eab2f7bcc4678c3b597beda5ee504b47c

commit 70294c8eab2f7bcc4678c3b597beda5ee504b47c
Author: Henrik Grunell <henrikg@webrtc.org>
Date: Wed Jan 31 08:32:19 2018

Revert "Add ScopedAllowBaseSyncPrimitives for DesktopConfigurationMonitor."

This reverts commit 0a3593c25dbc96b7d66d17ab77fc9984ab2bf245.

Reason for revert: breaks WebRTC roll to Chromium. https://chromium-review.googlesource.com/c/chromium/src/+/894164

[19742:771:0130/150628.286256:FATAL:thread_restrictions.cc(67)] Check failed: !g_blocking_disallowed.Get().Get(). To allow //base sync primitives in a scope where blocking is disallowed use ScopedAllowBaseSyncPrimitivesOutsideBlockingScope.
0   browser_tests                       0x0000000108d3682c base::debug::StackTrace::StackTrace(unsigned long) + 28
1   browser_tests                       0x0000000108d5b210 logging::LogMessage::~LogMessage() + 224
2   browser_tests                       0x0000000108e04366 base::ScopedAllowBaseSyncPrimitives::ScopedAllowBaseSyncPrimitives() + 150
3   browser_tests                       0x000000010be59c48 webrtc::DesktopConfigurationMonitor::Lock() + 24
4   browser_tests                       0x0000000106dbf229 webrtc::DesktopCapturer::CreateRawScreenCapturer(webrtc::DesktopCaptureOptions const&) + 313
5   browser_tests                       0x000000010be58725 webrtc::DesktopCapturer::CreateScreenCapturer(webrtc::DesktopCaptureOptions const&) + 21
6   browser_tests                       0x00000001074dc209 content::DesktopCaptureDevice::Create(content::DesktopMediaID const&) + 169
(...)


Original change's description:
> Add ScopedAllowBaseSyncPrimitives for DesktopConfigurationMonitor.
> This is a temporary measure until the synchronization method
> used in the class, gets fixed.
> 
> Bug:  chromium:796889 ,  chromium:795340 
> Change-Id: Ie3d394ae42f005e8e0f353d04ea9c1d053ea9fd2
> Reviewed-on: https://webrtc-review.googlesource.com/40460
> Reviewed-by: Erik Språng <sprang@webrtc.org>
> Commit-Queue: Tommi <tommi@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#21812}

TBR=tommi@webrtc.org,sprang@webrtc.org

Change-Id: I6237c3df7e33918d9fe2e46bad0f6f96cda77cd1
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:796889 ,  chromium:795340 
Reviewed-on: https://webrtc-review.googlesource.com/46540
Reviewed-by: Henrik Grunell <henrikg@webrtc.org>
Commit-Queue: Henrik Grunell <henrikg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21817}
[modify] https://crrev.com/70294c8eab2f7bcc4678c3b597beda5ee504b47c/modules/desktop_capture/mac/desktop_configuration_monitor.cc

Is there any update on this issue?
Ping. Is this really a Pri-1?

Comment 9 by tommi@chromium.org, Apr 26 2018

Labels: -Pri-1 Pri-2

Comment 10 by tommi@chromium.org, Jun 18 2018

Owner: niklase@chromium.org
Mergedinto: 796889
Status: Duplicate (was: Assigned)

Sign in to add a comment