Closed Bug 1254102 Opened 4 years ago Closed 4 years ago

[e10s] Switching from remote tabs to non-remote tabs stops browser sharing working/causes fatal assertions in debug mode

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox46 --- unaffected
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: standard8, Assigned: gcp, NeedInfo)

References

Details

(Keywords: regression)

Attachments

(2 files)

I was just testing out some of Hello in e10s mode with the patch from bug 1249365 applied.

STR:

- Set up a conversation with two people joined.
- Switch between remote tabs (i.e. standard web pages)

=> Works fine, scrolling keeps working

- Switch to an about:preferences tab

=> The shared view doesn't update
=> A debug build will crash at this stage (see below)

- In a non-debug build switch back to a remote tab

=> Tab sharing doesn't continue.

There's no way to restore it without going out the conversation and back in again.

Here's the end of a log from a debug build:

[Parent 83988] WARNING: short timeout didn't get an id (ipc/rcv) timed out 10004003
: file /Users/mark/loop/gecko-dev/ipc/glue/SharedMemoryBasic_mach.mm, line 622
Assertion failure: false (Receiver message short time out), at /Users/mark/loop/gecko-dev/ipc/glue/SharedMemoryBasic_mach.mm:623
#01: mozilla::ipc::Shmem::ShareTo(mozilla::ipc::Shmem::IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead, int, int)[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0x547068]
#02: mozilla::ipc::PBackgroundParent::CreateSharedMemory(unsigned long, mozilla::ipc::SharedMemory::SharedMemoryType, bool, int*)[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0x83e184]
#03: mozilla::camera::PCamerasParent::AllocShmem(unsigned long, mozilla::ipc::SharedMemory::SharedMemoryType, mozilla::ipc::Shmem*)[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0x8d6a2f]
#04: mozilla::ShmemBuffer mozilla::ShmemPool::Get<mozilla::camera::CamerasParent>(mozilla::camera::CamerasParent*, unsigned long)[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0x25af93b]
#05: mozilla::camera::CamerasParent::DeliverFrameOverIPC(mozilla::camera::CaptureEngine, int, mozilla::ShmemBuffer, unsigned char*, unsigned long, unsigned int, long long, long long)[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0x25a41f0]
#06: mozilla::camera::DeliverFrameRunnable::Run()[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0x25b1e19]
#07: nsThread::ProcessNextEvent(bool, bool*)[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0xf0060]
#08: NS_ProcessNextEvent(nsIThread*, bool)[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0x12fc43]
#09: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*)[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0x544b7f]
#10: MessageLoop::Run()[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0x50549c]
#11: nsThread::ThreadFunc(void*)[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/XUL +0xedda3]
#12: _pt_root[/Users/mark/loop/gecko-dev/objdir-ff/dist/NightlyDebug.app/Contents/MacOS/libnss3.dylib +0x306770]
#13: _pthread_body[/usr/lib/system/libsystem_pthread.dylib +0x405a]
#14: _pthread_body[/usr/lib/system/libsystem_pthread.dylib +0x3fd7]
Flags: needinfo?(mreavy)
gcp: any thoughts?  It looks like the patch from bug 1249365 doesn't handle chrome-based tabs.  Can you take this?

FYI for anyone looking at this bug: a fundamental difference between normal tabs and about: tabs is that normal tabs live in content, and about: tabs live in the master process, so the tab data is coming from a different place.  a) is it being correctly captured by the tabvideosource to begin with, b) is there some other problem?  It's quite possible that tab capture of about: tabs simply doesn't work in e10s even with the patch in bug 1249365.
Rank: 10
Flags: needinfo?(mreavy) → needinfo?(gpascutto)
Priority: -- → P1
This looks like an issue with Mac OS X Shmem?

[Parent 83988] WARNING: short timeout didn't get an id (ipc/rcv) timed out 10004003
: file /Users/mark/loop/gecko-dev/ipc/glue/SharedMemoryBasic_mach.mm, line 622
Assertion failure: false (Receiver message short time out), at /Users/mark/loop/gecko-dev/ipc/glue/SharedMemoryBasic_mach.mm:623

Does this reproduce on Windows/Linux?
Flags: needinfo?(gpascutto) → needinfo?(standard8)
Note it would be acceptable for this release if the tab stream paused or blacked out or otherwise did not display the non-remote tab, so long as it doesn't crash.
Assignee: nobody → gpascutto
I just tried this on a Windows 8 VM using an opt build. When I switched to the about:preferences tab, the sharing stopped working, and the conversation window went blank. The conversation window might be something else, but the sharing definitely seemed to stop working.
Flags: needinfo?(standard8)
Flags: needinfo?(gpascutto)
I'm not sure what I'm needinfoed for? I already gave my best guess at the cause, and it's wrong, so this will need to be debugged. The bug is already assigned to me.
Flags: needinfo?(gpascutto)
Sorry, I guess I was wanting to make sure you'd seen my comments.

Do you have an ETA for looking at this? We are trying to enable e10s for Loop (due to e10s being released to more users) and this is one of the things that is blocking it.
I can reproduce on Linux too, it just crashes outside PCameras() code:

hread 1 (Thread 0x7f1c77582b00 (LWP 23536)):
#0  0x00007f1c6eb6407d in nanosleep () at ../sysdeps/unix/syscall-template.S:81
#1  0x00007f1c6eb63f14 in __sleep (seconds=0) at ../sysdeps/unix/sysv/linux/sleep.c:137
#2  0x00007f1c739d2faf in ah_crap_handler(int) (signum=signum@entry=11)
    at /home/morbo/hg/firefox/toolkit/xre/nsSigHandlers.cpp:103
---Type <return> to continue, or q <return> to quit---
#3  0x00007f1c739d2fef in child_ah_crap_handler(int) (signum=11)
    at /home/morbo/hg/firefox/toolkit/xre/nsSigHandlers.cpp:115
#4  0x00007f1c7433e297 in AsmJSFaultHandler(int, siginfo_t*, void*) (signum=<optimized out>, info=0x7ffe4d3ee1b0, context=0x7ffe4d3ee080) at /home/morbo/hg/firefox/js/src/asmjs/WasmSignalHandlers.cpp:1161
#5  0x00007f1c773018d0 in <signal handler called> () at /lib/x86_64-linux-gnu/libpthread.so.0
#6  0x00007f1c727e045e in mozilla::MediaEngineTabVideoSource::InitRunnable::Run() (this=0x20)
    at /home/morbo/hg/firefox/dom/base/nsPIDOMWindowInlines.h:25
#7  0x00007f1c727e045e in mozilla::MediaEngineTabVideoSource::InitRunnable::Run() (this=0x7f1c56655a40)
    at /home/morbo/hg/firefox/dom/media/webrtc/MediaEngineTabVideoSource.cpp:90
#8  0x00007f1c706207e7 in nsThread::ProcessNextEvent(bool, bool*) (this=0x7f1c64a05600, aMayWait=<optimized out>, aResult=0x7ffe4d3ee7cf) at /home/morbo/hg/firefox/xpcom/threads/nsThread.cpp:994
#9  0x00007f1c7065cbd9 in NS_ProcessNextEvent(nsIThread*, bool) (aThread=<optimized out>, aMayWait=<optimized out>)
    at /home/morbo/hg/firefox/xpcom/glue/nsThreadUtils.cpp:297
#10 0x00007f1c70a5099c in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=this@entry=0x7f1c64a4cd80, aDelegate=aDelegate@entry=0x7ffe4d3eeab0) at /home/morbo/hg/firefox/ipc/glue/MessagePump.cpp:97
#11 0x00007f1c70a50bea in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (this=0x7f1c64a4cd80, aDelegate=0x7ffe4d3eeab0) at /home/morbo/hg/firefox/ipc/glue/MessagePump.cpp:295
#12 0x00007f1c709edf86 in MessageLoop::RunInternal() (this=this@entry=0x7ffe4d3eeab0)
    at /home/morbo/hg/firefox/ipc/chromium/src/base/message_loop.cc:234
#13 0x00007f1c709ee312 in MessageLoop::Run() (this=0x7ffe4d3eeab0)
    at /home/morbo/hg/firefox/ipc/chromium/src/base/message_loop.cc:227
#14 0x00007f1c709ee312 in MessageLoop::Run() (this=0x7ffe4d3eeab0)
    at /home/morbo/hg/firefox/ipc/chromium/src/base/message_loop.cc:201
#15 0x00007f1c72da4381 in nsBaseAppShell::Run() (this=0x7f1c5f0be4a0)
    at /home/morbo/hg/firefox/widget/nsBaseAppShell.cpp:156
#16 0x00007f1c739d157e in XRE_RunAppShell() () at /home/morbo/hg/firefox/toolkit/xre/nsEmbedFunctions.cpp:795
#17 0x00007f1c70a50af0 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (this=0x7f1c64a4cd80, aDelegate=0x7ffe4d3eeab0) at /home/morbo/hg/firefox/ipc/glue/MessagePump.cpp:263
#18 0x00007f1c709edf86 in MessageLoop::RunInternal() (this=this@entry=0x7ffe4d3eeab0)
    at /home/morbo/hg/firefox/ipc/chromium/src/base/message_loop.cc:234
#19 0x00007f1c709ee312 in MessageLoop::Run() (this=0x7ffe4d3eeab0)
    at /home/morbo/hg/firefox/ipc/chromium/src/base/message_loop.cc:227
#20 0x00007f1c709ee312 in MessageLoop::Run() (this=this@entry=0x7ffe4d3eeab0)
    at /home/morbo/hg/firefox/ipc/chromium/src/base/message_loop.cc:201
#21 0x00007f1c739d22a7 in XRE_InitChildProcess(int, char**, mozilla::gmp::GMPLoader*) (aArgc=<optimized out>, aArgv=<optimized out>, aGMPLoader=<optimized out>) at /home/morbo/hg/firefox/toolkit/xre/nsEmbedFunctions.cpp:631
#22 0x00000000004088ab in content_process_main(int, char**) (argc=5, argv=0x7ffe4d3effa8)
    at /home/morbo/hg/firefox/ipc/app/../contentproc/plugin-container.cpp:237
#23 0x000000000040893a in main(int, char**) (argc=<optimized out>, argv=<optimized out>)
    at /home/morbo/hg/firefox/ipc/app/MozillaRuntimeMain.cpp:11

#6  0x00007f1c727e045e in AsOuter (this=0x20) at /home/morbo/hg/firefox/dom/base/nsPIDOMWindowInlines.h:25
25        MOZ_ASSERT(IsOuterWindow());
(gdb) 
#7  mozilla::MediaEngineTabVideoSource::InitRunnable::Run (this=0x7f1c56655a40)
    at /home/morbo/hg/firefox/dom/media/webrtc/MediaEngineTabVideoSource.cpp:90
90            nsGlobalWindow::GetOuterWindowWithId(mVideoSource->mWindowId)->AsOuter();
(gdb) 


The PCameras() timeout in the original description may be from a hang/crash instead.
blassey, khuey, any advice for debugging why this would fail when switching from content to chrome in an e10s Loop conversation?

https://dxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineTabVideoSource.cpp?from=dom%2Fmedia%2Fwebrtc%2FMediaEngineTabVideoSource.cpp#90
Flags: needinfo?(khuey)
Flags: needinfo?(blassey.bugs)
Is the Window ID from another process (and therefore GetOuterWindowWithId() is returning null)?
Flags: needinfo?(blassey.bugs)
Brad's theory in comment 9 seems the most likely reason for failure.
Flags: needinfo?(khuey)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #9)
> Is the Window ID from another process (and therefore GetOuterWindowWithId()
> is returning null)?

And if it is? I mean that it was working just fine - we've all seen it working at the Hello team before - thus this being a quite recent regression.
Flags: needinfo?(khuey)
Flags: needinfo?(blassey.bugs)
(In reply to Mike de Boer [:mikedeboer] from comment #11)
> And if it is? I mean that it was working just fine - we've all seen it
> working at the Hello team before - thus this being a quite recent regression.

...and with 'working' I mean that it was showing a black stream on the receiving side. It did not show the actual tabs' content, which is quite impossible atm due to it being in a different process than the Hello content window.
(In reply to Mike de Boer [:mikedeboer] from comment #11)
> we've all seen it working at the Hello team before

You've seen what working when in which circumstances?

From the looks of it, switching from content to chrome (hence e10s being relevant) breaks the TabVideoSource code here, which in turn causes PCameras() to get wedged in an unrecoverable state. Probably I need to fix PCameras() to be robusts to this as a first step, but then there's still the question how to fix TabVideoSource.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #13)
>  Probably I need to fix PCameras() to be robusts to this as a first step,

Actually, the TabVideoSource code needs to detect the error condition or it's just going to crash content due to the assertions.

If it's not expected to be able to share the chrome tabs then I can deal with this myself.
Flags: needinfo?(khuey)
Flags: needinfo?(blassey.bugs)
Yeah, let's please not support sharing chrome tabs in e10s mode. That'd be so much work with little gain. As long as it's back to being black and not crash, we'll be super happy!
>That'd be so much work with little gain.

Note though that the capturing code already sits in Chrome, so maybe it's not actually so difficult.

I added error handling for the case encountered in this bug, but after leaving the chrome tab and going back to content, the video sometimes does and sometimes does not resume. I need to dig a bit deeper here.
(In reply to Mike de Boer [:mikedeboer] from comment #11)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #9)
> > Is the Window ID from another process (and therefore GetOuterWindowWithId()
> > is returning null)?
> 
> And if it is? I mean that it was working just fine - we've all seen it
> working at the Hello team before - thus this being a quite recent regression.

Then bisection?

I feel like you're just asking us to guess at what might be wrong in code we're not familiar with :P
Observation so far: what seems to happen is that flipping quickly to chrome and back, the non-chrome tabs keep updating for a few seconds (with content that was displayed after the switch back from chrome) but then hang. So it's as if the missing frames are confusing something and it eventually stalls.
Marking as blocking 1248604 since it's effectively blocking our 1.2 release of the add-on.
gcp do you have a feel for when we could have a fix for this? We're hoping to have a feature complete add-on by Thursday, trying to understand if we should push that date out.
Blocks: 1248604
Flags: needinfo?(gpascutto)
I'm actively debugging this, but the involved area is a large part of the media stack.

My feeling right now is that MediaEngineTabVideoSource is likely not the responsible for this, but that somewhere on the sending/receiving side the steam gets stopped when the frames are temporarily halted.

jesup, you will need to assist here. Where could we be timing out things if frame delivery temporarily halts?

Regular camera starts:

Unnamed thread 0x7fbf1e5e0d60]: D/CamerasChild int mozilla::camera::CamerasChild::AllocateCaptureDevice(mozilla::camera
::CaptureEngine, const char*, unsigned int, int&, const nsACString_internal&)
[Cameras IPC]: D/CamerasChild virtual bool mozilla::camera::CamerasChild::RecvReplyAllocateCaptureDevice(const int&)
[Unnamed thread 0x7fbf1e5e0d60]: D/CamerasChild Capture Device allocated: 4097
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaManager Video device 4097 allocated for moz-safe-about:loopconversation#bGZD5CTS
bds
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaManager Start audio for stream 7fbf1d24f200
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaManager virtual nsresult mozilla::MediaEngineRemoteVideoSource::Start(mozilla::S
ourceMediaStream*, mozilla::TrackID)
[Unnamed thread 0x7fbf1e5e0d60]: D/CamerasChild int mozilla::camera::CamerasChild::StartCapture(mozilla::camera::Capture
Engine, int, webrtc::CaptureCapability&, webrtc::ExternalRenderer*)
[Cameras IPC]: D/CamerasChild virtual bool mozilla::camera::CamerasChild::RecvReplySuccess()
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaManager started all sources
[Main Thread]: D/MediaManager MediaCaptureWindowState: window 2147483655 capturing video audio    
[Main Thread]: D/MediaManager Returning success for getUserMedia()
[Cameras IPC]: D/CamerasChild virtual bool mozilla::camera::CamerasChild::RecvFrameSizeChange(const int&, const int&, co
nst int&, const int&)
[Cameras IPC]: D/MediaManager MediaEngineRemoteVideoSource Video FrameSizeChange: 640x480
[Cameras IPC]: V/MediaManager frame 0 (640x480); timestamp 595829718, ntp_time 3442594167, render_time 635987977
===========> last repeated regularly as video streams>

Screencapture starts on content:

[Unnamed thread 0x7fbf1e5e0d60]: D/CamerasChild int mozilla::camera::CamerasChild::NumberOfCaptureDevices(mozilla::camer
a::CaptureEngine)
[Cameras IPC]: V/MediaManager frame 84 (640x480); timestamp 596081718, ntp_time 3442596967, render_time 635990777
[Cameras IPC]: D/CamerasChild virtual bool mozilla::camera::CamerasChild::RecvReplyNumberOfCaptureDevices(const int&)
[Unnamed thread 0x7fbf1e5e0d60]: D/CamerasChild Capture Devices: 1
[Unnamed thread 0x7fbf1e5e0d60]: D/CamerasChild int mozilla::camera::CamerasChild::GetCaptureDevice(mozilla::camera::CaptureEngine, unsigned int, char*, unsigned int, char*, unsigned int)
[Cameras IPC]: D/CamerasChild virtual bool mozilla::camera::CamerasChild::RecvReplyFailure()
[Unnamed thread 0x7fbf1e5e0d60]: D/CamerasChild Cameras dispatch for IPC failed in GetCaptureDevice
[Unnamed thread 0x7fbf1e5e0d60]: D/GetUserMedia camera:GetCaptureDevice: Failed -1
=============> this is of no consequence, it's because the real camera is already allocated
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaEngineTabVideoSource Allocate
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaEngineTabVideoSource Restart
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaEngineTabVideoSource Start
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaEngineTabVideoSource InitRunnable
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaManager started all sources
[Main Thread]: D/MediaEngineTabVideoSource InitRunnable::Run
[Main Thread]: D/MediaEngineTabVideoSource InitRunnable::Run mVideoSource->mWindow set
[Main Thread]: D/MediaEngineTabVideoSource Draw
[Main Thread]: D/MediaManager MediaCaptureWindowState: window 2147483655 capturing video audio    browsershare
[Main Thread]: D/MediaManager Returning success for getUserMedia()
[Cameras IPC]: V/MediaManager frame 85 (640x480); timestamp 596084598, ntp_time 3442596999, render_time 635990809
[Cameras IPC]: V/MediaManager frame 86 (640x480); timestamp 596087478, ntp_time 3442597031, render_time 635990841
[Cameras IPC]: V/MediaManager frame 87 (640x480); timestamp 596090718, ntp_time 3442597067, render_time 635990877
[Main Thread]: D/MediaEngineTabVideoSource Draw
===========> last lines alternate, we see the MediaEngineTabVideoSource Draw function called

Screencapture tries to grab chrome:

[Unnamed thread 0x7fbf1e5e0d60]: D/MediaEngineTabVideoSource Allocate
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaEngineTabVideoSource Restart
[Cameras IPC]: V/MediaManager frame 2739 (640x480); timestamp 604046808, ntp_time 3442685468, render_time 636079278
[Main Thread]: D/MediaEngineTabVideoSource Draw
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaEngineTabVideoSource Start
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaEngineTabVideoSource InitRunnable
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaManager started all sources
...
[Main Thread]: D/MediaEngineTabVideoSource Draw
[Main Thread]: D/MediaEngineTabVideoSource InitRunnable::Run
[Main Thread]: D/MediaEngineTabVideoSource InitRunnable::Run: !globalWindow
===========> Window lookup fails because it's in chrome
[Main Thread]: D/MediaManager MediaCaptureWindowState: window 2147483655 capturing video audio    browsershare
[Main Thread]: D/MediaManager Returning success for getUserMedia()
[Main Thread]: D/MediaManager MediaCaptureWindowState: window 2147483655 capturing video audio    browsershare
[Main Thread]: D/MediaManager Listener removed on purpose, mFinished = 1
[Main Thread]: D/MediaManager Listener removed by DOM Destroy(), mFinished = 1
[Cameras IPC]: V/MediaManager frame 2746 (640x480); timestamp 604067688, ntp_time 3442685700, render_time 636079510
[Cameras IPC]: V/MediaManager frame 2747 (640x480); timestamp 604070568, ntp_time 3442685732, render_time 636079542
[Cameras IPC]: V/MediaManager frame 2748 (640x480); timestamp 604073808, ntp_time 3442685768, render_time 636079578
===========> MediaEngineTabVideoSource Draw calls are missing now!

Screencapture goes back to content:

[Unnamed thread 0x7fbf1e5e0d60]: D/MediaEngineTabVideoSource Allocate
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaEngineTabVideoSource Restart
...
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaEngineTabVideoSource Start
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaEngineTabVideoSource InitRunnable
[Unnamed thread 0x7fbf1e5e0d60]: D/MediaManager started all sources
[Main Thread]: D/MediaEngineTabVideoSource InitRunnable::Run
[Main Thread]: D/MediaEngineTabVideoSource InitRunnable::Run mVideoSource->mWindow set
[Main Thread]: D/MediaEngineTabVideoSource Draw
[Main Thread]: D/MediaManager MediaCaptureWindowState: window 2147483655 capturing video audio    browsershare
[Main Thread]: D/MediaManager Returning success for getUserMedia()
[Main Thread]: D/MediaManager MediaCaptureWindowState: window 2147483655 capturing video audio    browsershare
[Main Thread]: D/MediaManager Listener removed on purpose, mFinished = 1
[Main Thread]: D/MediaEngineTabVideoSource Draw
[Cameras IPC]: V/MediaManager frame 2819 (640x480); timestamp 604286568, ntp_time 3442688132, render_time 636081942
[Main Thread]: D/MediaManager Listener removed by DOM Destroy(), mFinished = 1
[Cameras IPC]: V/MediaManager frame 2821 (640x480); timestamp 604292688, ntp_time 3442688200, render_time 636082010
[Main Thread]: D/MediaEngineTabVideoSource Draw
[Cameras IPC]: V/MediaManager frame 2822 (640x480); timestamp 604295568, ntp_time 3442688232, render_time 636082042

===========> MediaEngineTabVideoSource Draw calls are alternating with frame delivery again BUT NOTHING ARRIVES ON REMOTE SIDE
Flags: needinfo?(gpascutto) → needinfo?(rjesup)
I tried modifying MediaEngineTabVideoSource to just keep outputting the old mImage if the case from this bug is detected, but *still* the remote ends video stops after some delay.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #16)
> Note though that the capturing code already sits in Chrome, so maybe it's
> not actually so difficult.

Turns out MediaEngineTabVideoSource is distinct from all other screensharing, that sits in MediaEngineRemoteVideoSource. So you were right that this currently won't work cross-process.

>I tried modifying MediaEngineTabVideoSource to just keep outputting the old mImage 
>if the case from this bug is detected, but *still* the remote ends video stops after some delay.

This didn't work because switching tabs creates a new MediaEngineTabVideoSource object, so the old image wasn't actually available.

I have some hacky code to just output a 640x480 black square and that makes things work.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #22)
> I have some hacky code to just output a 640x480 black square and that makes
> things work.

Well, I'd speculate that this is a perfectly reasonable solution! It's prolly good to document this in the code extensively so that someone else reading it doesn't go like 'muuuup?'.

So yeah, the MediaEngineTabVideoSource sits in the content process because that's where the rendering pipeline is for the document as well. So it doesn't capture the tab viewport and cuts away the chrome; therefore it has the potential to add parts of the document that is out-of-view to the stream, which might be _very_ useful in the future.
FWIW, the reverse behavior also exists: if you start a loop session while a chrome tab is selected, sending chrome works, but content tabs will be inaccessible.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #24)
> FWIW, the reverse behavior also exists: if you start a loop session while a
> chrome tab is selected, sending chrome works, but content tabs will be
> inaccessible.

Thank you for highlighting this, I've filed bug 1257243 to track it.
Comment on attachment 8731301 [details]
MozReview Request: Bug 1254102 - Don't skip Tab sources if the camera is in use. r?jesup

https://reviewboard.mozilla.org/r/40477/#review37015
Attachment #8731301 - Flags: review?(rjesup) → review+
Comment on attachment 8731302 [details]
MozReview Request: Bug 1254102 - Add "blackout" mode. If the window is not legal, output a black square. r?jesup

https://reviewboard.mozilla.org/r/40479/#review37001

I don't see the need (in this patch at least) for mBlackedOutWindow - if (!mWindow) seems to be 100% equivalent to it.  Unless there's a need in some other bug, can we just remove it?  (Maybe add a comment or two if needed.)

::: dom/media/webrtc/MediaEngineTabVideoSource.cpp:329
(Diff revision 1)
>  
>  nsresult
>  MediaEngineTabVideoSource::Stop(mozilla::SourceMediaStream*, mozilla::TrackID)
>  {
> -  if (!mWindow)
> +  if (!mWindow && !mBlackedoutWindow)
>      return NS_OK;

if you're touching that, add braces
Attachment #8731302 - Flags: review?(rjesup)
https://reviewboard.mozilla.org/r/40479/#review37001

The original code has several !mWindow checks that prevent it from starting the periodic callbacks, and generally use it as an indicator that things have not been set up yet. See MediaEngineTabVideoSource::InitRunnable::Run() and Stop().

Hence most new checks are !mWindow && !mBlackedoutWindow, that is where the old code bailed with mWindow unset we now actually continue if we're blacking out.
Comment on attachment 8731302 [details]
MozReview Request: Bug 1254102 - Add "blackout" mode. If the window is not legal, output a black square. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40479/diff/1-2/
Attachment #8731302 - Flags: review?(rjesup)
https://reviewboard.mozilla.org/r/40479/#review37001

Ok; that makes more sense.  Add a comment or two
Comment on attachment 8731302 [details]
MozReview Request: Bug 1254102 - Add "blackout" mode. If the window is not legal, output a black square. r?jesup

https://reviewboard.mozilla.org/r/40479/#review37039
Attachment #8731302 - Flags: review?(rjesup) → review+
Comment on attachment 8731302 [details]
MozReview Request: Bug 1254102 - Add "blackout" mode. If the window is not legal, output a black square. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40479/diff/2-3/
Blocks: 1256357
https://hg.mozilla.org/mozilla-central/rev/a0a16898e8db
https://hg.mozilla.org/mozilla-central/rev/1ffc09b6908f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Blocks: 1257563
Duplicate of this bug: 1252396
gcp: Can we request uplift to aurora on this now please? I'm looking to turn the switch on Hello now.
Flags: needinfo?(gpascutto)
Comment on attachment 8731302 [details]
MozReview Request: Bug 1254102 - Add "blackout" mode. If the window is not legal, output a black square. r?jesup

Approval Request Comment
[Feature/regressing bug #]: Loop enabled in e10s mode
[User impact if declined]: Browser crash if sharing chrome tabs.
[Describe test coverage new/current, TreeHerder]: None, manually tested by several people, on m-c
[Risks and why]: Very low, fallback activates if existing code fails.
[String/UUID change made/needed]: N/A
Flags: needinfo?(gpascutto)
Attachment #8731302 - Flags: approval-mozilla-aurora?
Blocks: 1258865
Comment on attachment 8731302 [details]
MozReview Request: Bug 1254102 - Add "blackout" mode. If the window is not legal, output a black square. r?jesup

Verified manually, baked in Central for 10 days, Aurora47+
Attachment #8731302 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.