Screenshare browser test fails on mac |
|||||||
Issue descriptionBrowser test WebRtcDesktopCaptureBrowserTest.RunsScreenshareFromOneTabToAnother fails on Mac due to a thread checker. See https://chromium-review.googlesource.com/c/chromium/src/+/738040 Running with Mac disabled for until this is investigated. sergeyu@, you've touched the affected file, can you have a look and triage? FATAL:thread_restrictions.cc(105)] Check failed: !g_base_sync_primitives_disallowed.Get().Get(). Waiting on a //base sync primitive is not allowed on this thread to prevent jank and deadlock. If waiting on a //base sync primitive is unavoidable, do it within the scope of a ScopedAllowBaseSyncPrimitives. If in a test, use ScopedAllowBaseSyncPrimitivesForTesting. 0 browser_tests 0x000000010515588c base::debug::StackTrace::StackTrace(unsigned long) + 28 1 browser_tests 0x0000000105179e70 logging::LogMessage::~LogMessage() + 224 2 browser_tests 0x0000000105220549 base::internal::AssertBaseSyncPrimitivesAllowed() + 121 3 browser_tests 0x00000001051f177f base::WaitableEvent::TimedWaitUntil(base::TimeTicks const&) + 31 4 browser_tests 0x00000001051f19b1 base::WaitableEvent::TimedWait(base::TimeDelta const&) + 49 5 browser_tests 0x0000000102e981f4 rtc::Event::Wait(int) + 36 6 browser_tests 0x0000000107939e1d webrtc::EventWrapperImpl::Wait(unsigned long) + 13 7 browser_tests 0x000000010862bcbb webrtc::DesktopConfigurationMonitor::Lock() + 27 8 browser_tests 0x0000000103b50660 webrtc::(anonymous namespace)::ScreenCapturerMac::CaptureFrame() + 96 9 browser_tests 0x00000001053e7e95 NativeDesktopMediaList::Worker::RefreshThumbnails(std::__1::vector<content::DesktopMediaID, std::__1::allocator<content::DesktopMediaID> > const&, gfx::Size const&) + 229 10 browser_tests 0x0000000105156205 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) + 261 11 browser_tests 0x0000000105207e91 base::internal::TaskTracker::RunOrSkipTask(std::__1::unique_ptr<base::internal::Task, std::__1::default_delete<base::internal::Task> >, base::internal::Sequence*, bool) + 881 12 browser_tests 0x0000000105208cc2 base::internal::TaskTrackerPosix::RunOrSkipTask(std::__1::unique_ptr<base::internal::Task, std::__1::default_delete<base::internal::Task> >, base::internal::Sequence*, bool) + 162 13 browser_tests 0x0000000105207074 base::internal::TaskTracker::RunNextTask(scoped_refptr<base::internal::Sequence>, base::internal::CanScheduleSequenceObserver*) + 356 14 browser_tests 0x00000001051feb53 base::internal::SchedulerWorker::Thread::ThreadMain() + 691 15 browser_tests 0x000000010521302f base::(anonymous namespace)::ThreadFunc(void*) + 95
,
Jan 10 2018
//base OWNERS really want to be aware of all WaitableEvent/ConditionVariable uses in Chrome, because they are a common source of hangs and deadlocks. You'll need to add DesktopConfigurationMonitor as a friend of ScopedAllowBaseSyncPrimitives and instantiate a ScopedAllowBaseSyncPrimitives in scopes where you wait. MayBlock() grants you permission to do blocking file/network operations, but it is not enough to be able to use WaitableEvent/ConditionVariable.
,
Jan 10 2018
DesktopConfigurationMonitor is in WebRTC repo. I don't think we can use ScopedAllowBaseSyncPrimitives there. WebRTC's sync primitives are overridden with base sync classes, see third_party/webrtc_overrides/rtc_base/event.cc . What's the recommended solution in this case? We could add ScopedAllowBaseSyncPrimitives in every place in chromium where webrtc::DesktopCapturer is used, but it doesn't look like the right solution.
,
Jan 11 2018
,
Jan 11 2018
My gut tells me that there's a way to avoid needing a lock at this place and possiby at all in this class. However I don't have time to look at it this week. Erik, is there someone on your team that can?
,
Jan 12 2018
Removing usage of //base sync primitives is the best solution. If you don't have time for it, you can add a ScopedAllowBaseSyncPrimitives in third_party/webrtc_overrides/rtc_base/event.cc, where including base/ files is allowed. +gab@: Can you comment as a //base owner?
,
Jan 12 2018
I can do that - or something along those lines. I added the event override in Chrome specifically to catch cases where we do these types of things in WebRTC. So in that way, this bug is a success, or rather finding it, is :) I'd like to continue finding these issues and at the same time not regress from the current state. So in this particular case, I'd prefer to only resort to ScopedAllowBaseSyncPrimitives in the places that need it (DesktopConfigurationMonitor etc), and not the Event class wholesale. I hope to have some more time to look closer at this in the next few days. If you Sergey can also take a look at DesktopConfigurationMonitor, then that would be good. I see that it's got things like calls to abort() in it, which is unusual for webrtc code, so it could be that this could have been thought of as a temporary solution but has been mostly unowned for years.
,
Jan 12 2018
From a synchronization point of view I also don't understand how correctness is being guaranteed by using an event object to implement Lock/Unlock behavior. There seem to be more than one thread that can call Set() and Wait() regardless of the state of the event (Lock() doesn't actually "lock" anything and a following call to Unlock() might not match with the previous Set() in the event that another thread 'unlocked' the event already without actually owning it).
,
Jan 12 2018
As far as scoped allowances go, if you must use it : definitely prefer adding them at the caller/user site (which then is explicitly aware of the incurred wait) rather than have a blanket allowance on the type (which defeats the purpose of the check). Le ven. 12 janv. 2018 20 h 03, tommi via monorail < monorail+v2.233641965@chromium.org> a écrit :
,
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
,
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
,
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
,
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
,
May 9 2018
ping, what's the latest status here? Thanks!
,
Oct 23
,
Oct 23
,
Oct 23
,
Oct 23
This issue is still reproducible. I will work on a fix.
,
Oct 25
What I am also observing is that, when I trigger this DesktopConfigurationMonitor::DisplaysReconfigured code, it causes an error. I am just plugging in an external monitor during the capture session to reproduce. [11446:172551:1025/144058.176607:ERROR:screen_capturer_mac.mm(314)] The selected screen cannot be found for capturing. [11446:172551:1025/144058.178635:ERROR:video_capture_device_client.cc(490)] error@ OnCaptureResult@../../content/browser/media/capture/desktop_capture_device.cc:320, The desktop capturer has failed., OS message: Invalid argument (22)
,
Oct 25
On a second test, above error happens when display is optimized for the external monitor. When I change defaults such that display is timized for the built-in monitor, capture session continues as expected.
,
Oct 25
Maybe it's related to the change at https://cs.chromium.org/chromium/src/third_party/webrtc/modules/desktop_capture/mac/desktop_configuration.mm?l=176?
,
Oct 30
The following revision refers to this bug: https://webrtc.googlesource.com/src.git/+/eaf337a1411c26e707449b805e605fc723898172 commit eaf337a1411c26e707449b805e605fc723898172 Author: Emircan Uysaler <emircan@webrtc.org> Date: Tue Oct 30 16:05:21 2018 Remove event wait logic from DesktopConfigurationMonitor This class exposes Wait()-Set() logic to synchronize events. - There is a bug in checking EventWrapper::Wait() as it returns [1,2]. Negating these values cause us to always pass timeout checks. - There is a general problem in this class with waiter. There are 2 scenarios: 1) Lock()-Unlock()-DisplaysReconfigured() In this scenario, Wait() in DisplaysReconfigured() immediately passes as event is already signaled. Next Lock() call won't continue until Set() is called in DisplaysReconfigured(). This blocks capture thread from accessing display until reconfiguration completes. 2) Lock()-DisplaysReconfigured()-Unlock() In this scenario, Wait() in DisplaysReconfigured() passes when Unlock() called. Capture thread accesses display while reconfiguration happens. Note that we are only delaying the OS delegate thread here. As an experiment, adding Sleep() in DisplaysReconfigured() results in no change, because it looks like OS uses this thread for only delegates but not for the actual display switch. Overall, (1) doesnt seem necessary as (2) already accesses display while reconfiguration happens. (2) doesn't seem necessary as blocking system delegate thread doesn't help. Therefore, I changed the class to only protect from race condition on |desktop_configuration_|. Bug: chromium:796889 Change-Id: I37263305e5ac629e21ff9e977952cf4a21bae19f Reviewed-on: https://webrtc-review.googlesource.com/c/108560 Commit-Queue: Emircan Uysaler <emircan@webrtc.org> Reviewed-by: Tommi <tommi@webrtc.org> Cr-Commit-Position: refs/heads/master@{#25437} [modify] https://crrev.com/eaf337a1411c26e707449b805e605fc723898172/modules/desktop_capture/mac/desktop_configuration_monitor.cc [modify] https://crrev.com/eaf337a1411c26e707449b805e605fc723898172/modules/desktop_capture/mac/desktop_configuration_monitor.h [modify] https://crrev.com/eaf337a1411c26e707449b805e605fc723898172/modules/desktop_capture/mac/screen_capturer_mac.mm [modify] https://crrev.com/eaf337a1411c26e707449b805e605fc723898172/modules/desktop_capture/mouse_cursor_monitor_mac.mm [modify] https://crrev.com/eaf337a1411c26e707449b805e605fc723898172/modules/desktop_capture/window_capturer_mac.mm
,
Oct 30
Feel free to reopen if you hit any issues as it is a little risky change.
,
Oct 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8309477a707e831908b954a314b8055de7631499 commit 8309477a707e831908b954a314b8055de7631499 Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Date: Tue Oct 30 23:44:27 2018 Roll src/third_party/webrtc 3b149e4be880..1a92cd7312ca (22 commits) https://webrtc.googlesource.com/src.git/+log/3b149e4be880..1a92cd7312ca git log 3b149e4be880..1a92cd7312ca --date=short --no-merges --format='%ad %ae %s' 2018-10-30 chromium-webrtc-autoroll@webrtc-ci.iam.gserviceaccount.com Roll chromium_revision 34bb9a9162..0cb3899c4e (603839:603959) 2018-10-30 mellem@webrtc.org Create a MediaTransportState enum and add a state callback to MediaTransport. 2018-10-30 emircan@webrtc.org Remove event wait logic from DesktopConfigurationMonitor 2018-10-30 alessiob@webrtc.org AGC2: renaming GainCurveApplier to Limiter. 2018-10-30 ilnik@webrtc.org Revert "Use only first payload timestamp for RTCP SR generation for audio" 2018-10-30 crodbro@webrtc.org Fix for clock reset repair. 2018-10-30 nisse@webrtc.org Delete StreamInterface::ReadLine. 2018-10-30 Peter) Slatala Update media transport settings struct 2018-10-30 terelius@webrtc.org Add support for field trials in peerconnection_client|server 2018-10-30 ilnik@webrtc.org Use only first payload timestamp for RTCP SR generation for audio 2018-10-30 terelius@webrtc.org Add field trial to enable the new RTC event log format. 2018-10-30 artit@webrtc.org Revert "Disabled TestPacketBuffer.SeqNumWrapOneFrame test due to clang update" 2018-10-30 robert@bares.me Always call ConvertToI420 with positive crop_height 2018-10-30 nisse@webrtc.org Delete OptionsFile class. Refactored only user, TurnFileAuth. 2018-10-30 srte@webrtc.org Makes PacketResult::GetSentPacket const. 2018-10-30 chromium-webrtc-autoroll@webrtc-ci.iam.gserviceaccount.com Roll chromium_revision 89ed1da2c8..34bb9a9162 (603733:603839) 2018-10-30 nisse@webrtc.org Delete unused function rtc::Flow. 2018-10-30 artit@webrtc.org Add iOS SDK unit tests for nalu_rewriter 2018-10-30 kron@webrtc.org Propagate SDP negotiation of extmap-allow-mixed to RtpHeaderExtensionMap 2018-10-30 oprypin@webrtc.org Remove likely obsolete entries from WATCHLISTS 2018-10-30 chromium-webrtc-autoroll@webrtc-ci.iam.gserviceaccount.com Roll chromium_revision 03b56190ff..89ed1da2c8 (603619:603733) 2018-10-29 chromium-webrtc-autoroll@webrtc-ci.iam.gserviceaccount.com Roll chromium_revision 55624cc6cd..03b56190ff (603513:603619) Created with: gclient setdep -r src/third_party/webrtc@1a92cd7312ca The AutoRoll server is located here: https://autoroll.skia.org/r/webrtc-chromium-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary. CQ_INCLUDE_TRYBOTS=luci.chromium.try:linux_chromium_archive_rel_ng;luci.chromium.try:mac_chromium_archive_rel_ng BUG=chromium:None,chromium:796889,chromium:none,chromium:887464,chromium:None,chromium:None,chromium:None,chromium:None TBR=webrtc-chromium-sheriffs-robots@google.com Change-Id: I6de52c483d648ac07efede2b984bc1b7228e6754 Reviewed-on: https://chromium-review.googlesource.com/c/1308696 Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Cr-Commit-Position: refs/heads/master@{#604058} [modify] https://crrev.com/8309477a707e831908b954a314b8055de7631499/DEPS |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by sergeyu@chromium.org
, Dec 21 2017Owner: fdoray@chromium.org