STR: 1. (Prep) In about:config modify media.getusermedia.screensharing.allowed_domains by appending ", fiddle.jshell.net" to the long tail of comma-separated sites. 2. Open URL, and share entire screen. 3. Click one of the resolution buttons (e.g. [320]). Expected result: Video element grows in size Actual result: - In opt build: Video element grows in size, but when I hit the fiddle "Run" button to rerun, the screensharing code no longer responds. Also, the screen sharing indicator on the OSX desktop remains, when it should have gone away, sometimes reading "Sharing with 0 tabs" when clicked on after I close the tab. Eventually it crashes in some form or other, one of the forms is like this: https://crash-stats.mozilla.com/report/index/9cb1a789-ae2e-4de8-9209-91cb12161003 . - In Debug build: assert-crash: > frame #4: 0x00000001070beb13 XUL`webrtc::MouseCursorMonitorMac::Init(this=0x000000017d2ac480, callback=0x000000011e1cf840, mode=SHAPE_AND_POSITION) + 83 at mouse_cursor_monitor_mac.mm:82 > 79 MouseCursorMonitorMac::~MouseCursorMonitorMac() {} > 80 > 81 void MouseCursorMonitorMac::Init(Callback* callback, Mode mode) { > -> 82 assert(!callback_); > 83 assert(callback); > 84 > 85 callback_ = callback; > (lldb) bt > * thread #102: tid = 0x2cfb78, 0x00007fff91e510ae libsystem_kernel.dylib`__pthread_kill + 10, name = 'VideoCapture', stop reason = signal SIGABRT > frame #0: 0x00007fff91e510ae libsystem_kernel.dylib`__pthread_kill + 10 > frame #1: 0x00007fff98e9a500 libsystem_pthread.dylib`pthread_kill + 90 > frame #2: 0x00007fff85b5137b libsystem_c.dylib`abort + 129 > frame #3: 0x00007fff85b189c4 libsystem_c.dylib`__assert_rtn + 321 > * frame #4: 0x00000001070beb13 XUL`webrtc::MouseCursorMonitorMac::Init(this=0x000000017d2ac480, callback=0x000000011e1cf840, mode=SHAPE_AND_POSITION) + 83 at mouse_cursor_monitor_mac.mm:82 > frame #5: 0x0000000107095c0a XUL`webrtc::DesktopAndCursorComposer::Start(this=0x000000011e1cf830, callback=0x0000000130133010) + 106 at desktop_and_cursor_composer.cc:135 > frame #6: 0x00000001070b9921 XUL`webrtc::DesktopCaptureImpl::StartCapture(this=0x0000000130133000, capability=0x000070000210a670) + 113 at desktop_capture_impl.cc:784 > frame #7: 0x00000001070f7aab XUL`webrtc::ViECapturer::Start(this=0x000000011c64e000, capture_capability=0x000070000210a8a8) + 379 at vie_capturer.cc:263 > frame #8: 0x00000001070f7870 XUL`webrtc::ViECaptureImpl::StartCapture(this=0x0000000182a9e428, capture_id=4097, capture_capability=0x000070000210a8a8) + 384 at vie_capture_impl.cc:228 > frame #9: 0x0000000105d565d3 XUL`mozilla::camera::CamerasParent::RecvStartCapture(this=0x000000011ded22f8)::$_17::operator()() const + 563 at CamerasParent.cpp:853 > frame #10: 0x0000000105d562e9 XUL`mozilla::media::LambdaRunnable<mozilla::camera::CamerasParent::RecvStartCapture(this=0x000000011ded22e0)::$_17>::Run() + 25 at MediaUtils.h:197 > frame #11: 0x0000000102001c9b XUL`nsThread::ProcessNextEvent(this=0x0000000122a91c00, aMayWait=false, aResult=0x000070000210aa8e) + 1211 at nsThread.cpp:1067 > frame #12: 0x0000000102087c2c XUL`NS_ProcessNextEvent(aThread=0x0000000122a91c00, aMayWait=false) + 140 at nsThreadUtils.cpp:290 > frame #13: 0x0000000102a036b6 XUL`mozilla::ipc::MessagePumpForNonMainThreads::Run(this=0x000000011eca9540, aDelegate=0x000070000210ad68) + 646 at MessagePump.cpp:354 > frame #14: 0x00000001028fdac5 XUL`MessageLoop::RunInternal(this=0x000070000210ad68) + 117 at message_loop.cc:235 > frame #15: 0x00000001028fda25 XUL`MessageLoop::RunHandler(this=0x000070000210ad68) + 21 at message_loop.cc:228 > frame #16: 0x00000001028fd9cd XUL`MessageLoop::Run(this=0x000070000210ad68) + 45 at message_loop.cc:208 > frame #17: 0x000000010294f43f XUL`base::Thread::ThreadMain(this=0x000000011d048d80) + 831 at thread.cc:180 > frame #18: 0x00000001029500be XUL`ThreadFunc(closure=0x000000011d048d80) + 30 at platform_thread_posix.cc:38 > frame #19: 0x00007fff98e979b1 libsystem_pthread.dylib`_pthread_body + 131 > frame #20: 0x00007fff98e9792e libsystem_pthread.dylib`_pthread_start + 168 > frame #21: 0x00007fff98e95385 libsystem_pthread.dylib`thread_start + 13 > (lldb) The code that asserts was added in Bug 987979, and this appears to be a regression from that, although I had trouble narrowing down a regression range on OSX, because the behavior predating this was unreliable for me as well when I hit the fiddle "Run" button to retry the steps, sometimes working, but sometimes hosing the screensharing code on subsequent runs. I had to dig back as far as revision: 2235e56c94cf61614902fd3a4ac7b837f7154b97 (2015-09-22), to find a version that didn't have any problems with these steps. Still we should probably focus on the immediate crash first. Looks like the screensharing code never liked being stopped and restarted much (which is the codepath applyConstraints exercises).

Just realized some of the crashes I saw from this, like https://crash-stats.mozilla.com/report/index/fef4bb2b-4a71-4fde-a99a-e5ba52161003 has a non-null crash address.

some related crashes I saw while fiddling: https://crash-stats.mozilla.com/report/index/13a87372-707d-40b1-8cf5-4e8372161003 https://crash-stats.mozilla.com/report/index/a80920d1-5382-4681-8819-d95072161003 https://crash-stats.mozilla.com/report/index/ab0947e5-a05b-46d4-b7d0-225292161003 (in x11 capturer? can poke google) https://crash-stats.mozilla.com/report/index/3871da42-049c-4b01-935b-ae0222161003 (oom, probably side effect)

Screen/etc capture never implemented Stop()/StopCapture()/etc, and would assert and/or do bad things if you tried to Stop it and then re-Start() it. (in debug builds it's caught by assert()s)

Comment on attachment 8797801 [details] [diff] [review] Implement ::Stop() for screen/window/app capture Review of attachment 8797801 [details] [diff] [review]: ----------------------------------------------------------------- Lgtm. ::: media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor.h @@ +77,4 @@ > virtual void Init(Callback* callback, Mode mode) = 0; > > + // clears the callback > + virtual void Stop() = 0; Maybe add a Start method for symmetry? ::: media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor_x11.cc @@ +132,5 @@ > > +void MouseCursorMonitorX11::Stop() { > + callback_ = NULL; > + if (have_xfixes_) { > + x_display_->RemoveEventHandler(xfixes_event_base_ + XFixesCursorNotify, There's no corresponding ::Start() method... Does this mean MouseCursorMonitorX11::Init is called again on start? If so, we should at least remove the comment that says it can only be called once [1]. [1] https://dxr.mozilla.org/mozilla-central/rev/42c95d88aaaa7c2eca1d278399421d437441ac4d/media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor_x11.cc#113

Thanks. Before landing I'm going to do some thread-safety analysis around ensuring use of the callback_ ptrs won't be racy.

a few more classes that needed updating, even though we aren't using them. For these I needed to move some cleanup code around

Comment on attachment 8798144 [details] [diff] [review] Implement ::Stop() for windows gdi and magnifier screencapture Review of attachment 8798144 [details] [diff] [review]: ----------------------------------------------------------------- r=me if you remove the setting of disable_composition_ to false. ::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc @@ +169,5 @@ > + if (disable_composition_) { > + // Restore Aero. > + if (composition_func_) > + (*composition_func_)(DWM_EC_ENABLECOMPOSITION); > + disable_composition_ = false; disable_composition_ is a flag set in the constructor, and should not be set to false here, because it's not set in Start(). ::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc @@ +84,5 @@ > + } > + > + if (mag_lib_handle_) { > + FreeLibrary(mag_lib_handle_); > + mag_lib_handle_ = NULL; Why do we need to free the Magnification.dll on Stop()? Can't we check if it's already open in Start/InitializeMagnifier() [1] instead? [1] https://dxr.mozilla.org/mozilla-central/rev/ea104eeb14cc54da9a06c3766da63f73117723a0/media/webrtc/trunk/webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc#255

> disable_composition_ is a flag set in the constructor, and should not be set > to false here, because it's not set in Start(). Good point. > > + if (mag_lib_handle_) { > > + FreeLibrary(mag_lib_handle_); > > + mag_lib_handle_ = NULL; > > Why do we need to free the Magnification.dll on Stop()? > > Can't we check if it's already open in Start/InitializeMagnifier() [1] > instead? a) I want to clean up all resources possible on Stop so I'm sure it's ok to Start() again b) I don't really care about efficiency here; we don't even use this ;-)

Need to verify if this is really a sec bug (I think it is)

Comment on attachment 8797801 [details] [diff] [review] Implement ::Stop() for screen/window/app capture Approval Request Comment [Feature/regressing bug #]:ApplyConstraints support [User impact if declined]: Possible crashes when applyConstraints() is used [Describe test coverage new/current, TreeHerder]: We don't exercise applyConstraints on screenshares in automation; this might be possible. We have fiddles (noted here) we've used to test it manually. [Risks and why]: generally low - Support for Stop was never implemented, and is needed for applyConstraints(). This does cause new code paths to be exercised, though most are pretty simple. [String/UUID change made/needed]: none

Comment on attachment 8797801 [details] [diff] [review] Implement ::Stop() for screen/window/app capture Fix a regression related to screensharing. Take it in 51 aurora.

Comment on attachment 8800309 [details] [diff] [review] Disable applyConstraints for screensharing in 50. Review of attachment 8800309 [details] [diff] [review]: ----------------------------------------------------------------- I'd written basically the same patch... :-)

Comment on attachment 8800309 [details] [diff] [review] Disable applyConstraints for screensharing in 50. Approval Request Comment [Feature/regressing bug #]: ApplyConstraints support [User impact if declined]: Possible crashes when applyConstraints() is used [Describe test coverage new/current, TreeHerder]: Manual testing [Risks and why]: Very low risk - merely ignores requests to change resolution/framerate in Beta, to avoid exposing any new code paths in beta. Full fix is in aurora and nightly. [String/UUID change made/needed]: none

Comment on attachment 8800309 [details] [diff] [review] Disable applyConstraints for screensharing in 50. Crash fix, has stabilized on Nightly for a week, Beta50+

Crashed using the STR from the description and Firefox 49 on Windows 10 x64. Crash report: bp-3994f9c6-838a-4637-be28-890d82161103 Confirming that the crash no longer occurs on Windows 10x64, Ubuntu 12.04x86 and Mac OS X 10.11 using: * Fx 50 RC, build ID 20161101104304, * Latest 51.0a2 Aurora, build ID 20161102004003, * Latest 52.0a1 Nightly, build ID 20161102030205.