1050802 - screenshare ui for "Stop sharing" stops both screen and camera

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Philipp Hancke]

Attachment: stopsharing.png

Go to the URL, click on the Window button.
This asks for camera permission and then window permission. Allow both. Both camera and screen videos should be visible now.

Then stop sharing via the URL bar UI shown in the attached stopsharing.png
This stops sharing both the screen and the camera.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Suspect this is due to in invalidating all sharing for the window, as opposed to a specific track or stream.

Comment 2

[Philipp Hancke]

yes... stopping the camera that way seems to stop the screen, too.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

In fact, for getUserMedia:revoke:

      LOG(("Revoking MediaCapture access for window %llu",windowID));
      OnNavigation(windowID);

It only specifies a window to revoke, not a specific track or even stream.  It is currently (for audio/video) described to the user as "You're sharing <...> with site." and the option is "Stop sharing" - no way to stop video and not audio, for example.

There is an open question about the intent of the Chrome UI for this; normally app UI will provide clean "end"/turn-off options, and the Chrome UI is a bit of a backstop and privacy guarantee and notification (since the apps can't be trusted - the user gets feedback through the URLbar icons and the top-of-screen hanger, and gets a UI that can't be spoofed - it's not the normal Stop Capturing UI, though).

To resolve this we'd need to pass a type (since the user isn't exposed to streams or tracks, and wouldn't know what was hooked up to a specific on anyways.) 

If we apply this generally, it means that stopping sharing video would leave audio being captured (and vice versa). And stopping screen sharing would leave all audio and video streams/tracks active.

Another option would be to handle screensharing by type (ends all screensharing/windowsharing) with the site, but leave audio and video tied together for less confusion. This would require no string changes.

The last option would be to leave it as-is, and revise the strings as soon as possible (perhaps uplifting the screensharing string change at least to Aurora), and let app UI handle "I want to stop X but keep sharing Y".


My personal temptation would be to allow ending screen/windowsharing by type, but leave audio and video tied - but (soon) provide Chrome audio and video Mute's, which has always been on the UI wish-list.  I think overall that will cause less user confusion and map better to user interests.  In release terms - modify screensharing disables now and try to uplift to 33 (new feature in 33); and add Mute UI in 34 or 35.

Needinfoing Florian on UI and deveditz since this impacts the Privacy portion of the UI.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Test for revoking only screen/window sharing

Note: must be installed on a webserver so you can whitelist!  (localhost, real server, etc).

Hacked gum_test.html to ask for video:true 10 seconds after asking for a Window share.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Backend to allow stopping sharing for screen/window for a WindowID

works!

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch for stopping only screen/window when asked

Works poorly - it tries to cancel screen/window sharing (if they're active) even if you clicked on the camera and it was saying to you that you're sharing your camera.  It works well enough for me to test the backend, which now works.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8470672 [details] [diff] [review]
WIP patch for stopping only screen/window when asked

florian - can you fix this so we have a viable option for this possible solution?

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8470670 [details] [diff] [review]
Backend to allow stopping sharing for screen/window for a WindowID

Review of attachment 8470670 [details] [diff] [review]:
-----------------------------------------------------------------

I have some questions. Also, this seems to only be half the patch, in that camera access revoke still tears down everything, including screensharing. Is there a plan to fix this similarly?

::: dom/media/MediaManager.cpp
@@ +2005,5 @@
> +      uint64_t windowID = nsString(aData).ToInteger64(&rv);
> +      MOZ_ASSERT(NS_SUCCEEDED(rv));
> +      if (NS_SUCCEEDED(rv)) {
> +        LOG(("Revoking MediaCapture access for window %llu", windowID));
> +        OnNavigation(windowID);

This codepath still kills everything, including screensharing, if *camera* access is revoked.

@@ +2163,5 @@
> +void
> +MediaManager::StopScreensharing(uint64_t aWindowID)
> +{
> +  // We need to stop window/screensharing for all streams in all innerwindows that
> +  // correspond to that outerwindow.

You have this comment twice. This one's probably not needed since this stub merely calls the other function.

@@ +2174,5 @@
> +  StopScreensharing(window);
> +}
> +
> +void
> +MediaManager::StopScreensharing(nsPIDOMWindow *aWindow)

Why have a way to call StopScreensharing with a window pointer and not just id? Is there more code coming?

I ask because this function seems confused about whether it accepts an outerWindow or an InnerWindow. Unless you need both it might be better to define it one way and document it that way.

@@ +2177,5 @@
> +void
> +MediaManager::StopScreensharing(nsPIDOMWindow *aWindow)
> +{
> +  // We need to stop window/screensharing for all streams in all innerwindows that
> +  // correspond to that outerwindow.

This is where I admit I may not understand inner/outerwindows completely. I thought an outer window pretty much had only one active inner window at a time, and that you moved from one to another only though OnNavigation. If that's the case then hunting down multiple windows seems superfluous, since as sharing is killed when OnNavigation happens.

If this is not the case, please point me to a link where I can learn more. :-)

@@ +2184,5 @@
> +  // all the listeners for each one, and tell them to stop
> +  // window/screensharing
> +  nsCOMPtr<nsPIDOMWindow> piWin = do_QueryInterface(aWindow);
> +  if (piWin) {
> +    if (piWin->GetCurrentInnerWindow() || piWin->IsInnerWindow()) {

if (piWin->IsInnerWindow() || piWin->GetCurrentInnerWindow()) seems safer.

@@ +2186,5 @@
> +  nsCOMPtr<nsPIDOMWindow> piWin = do_QueryInterface(aWindow);
> +  if (piWin) {
> +    if (piWin->GetCurrentInnerWindow() || piWin->IsInnerWindow()) {
> +      uint64_t windowID;
> +      if (piWin->GetCurrentInnerWindow()) {

What happens if you call GetCurrentInnerWindow on an inner window?

::: dom/media/MediaManager.h
@@ +98,5 @@
>      }
>      return mStream->AsSourceStream();
>    }
>  
> +  void StopScreenWindowSharing();

StopScreenAndWindowSharing?

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Nm, I see this is similar to existing code in MediaManager.cpp

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8470670 [details] [diff] [review]
Backend to allow stopping sharing for screen/window for a WindowID

Review of attachment 8470670 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits and nullptr fix.

::: dom/media/MediaManager.cpp
@@ +2174,5 @@
> +  StopScreensharing(window);
> +}
> +
> +void
> +MediaManager::StopScreensharing(nsPIDOMWindow *aWindow)

OK I understand the code better now and I see the recursion below (I didn't know we supported screensharing in iframes). However, I'd feel better if you defined StopScreensharing to take an aInnerWindow (and assert that it is), and then make sure to pass in an innerWindow in the recursion step. That should remove the unused outerWindow codepath.

@@ +2195,5 @@
> +      StreamListeners* listeners = GetActiveWindows()->Get(windowID);
> +      if (listeners) {
> +        uint32_t length = listeners->Length();
> +        for (uint32_t i = 0; i < length; ++i) {
> +          nsRefPtr<GetUserMediaCallbackMediaStreamListener> listener =

could use auto& here instead of addref, or just:

listeners->ElementAt(i)->StopScreenWindowSharing();

@@ +2212,5 @@
> +        nsCOMPtr<nsIDocShellTreeItem> item;
> +        docShell->GetChildAt(i, getter_AddRefs(item));
> +        nsCOMPtr<nsPIDOMWindow> win = item ? item->GetWindow() : nullptr;
> +
> +        StopScreensharing(win);

Looks like you could end up passing nullptr to StopScreensharing here which seems bad. Missing if?

@@ +2264,5 @@
> +      new MediaOperationRunnable(MEDIA_STOP,
> +                                 this, nullptr, nullptr,
> +                                 nullptr, mVideoSource,
> +                                 mFinished, mWindowID, nullptr));
> +    mMediaThread->Dispatch(runnable, NS_DISPATCH_NORMAL);

Why not call Invalidate() here?

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #10)
> Comment on attachment 8470670 [details] [diff] [review]
> Backend to allow stopping sharing for screen/window for a WindowID
> 
> Review of attachment 8470670 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits and nullptr fix.
> 
> ::: dom/media/MediaManager.cpp
> @@ +2174,5 @@
> > +  StopScreensharing(window);
> > +}
> > +
> > +void
> > +MediaManager::StopScreensharing(nsPIDOMWindow *aWindow)
> 
> OK I understand the code better now and I see the recursion below (I didn't
> know we supported screensharing in iframes). However, I'd feel better if you
> defined StopScreensharing to take an aInnerWindow (and assert that it is),
> and then make sure to pass in an innerWindow in the recursion step. That
> should remove the unused outerWindow codepath.

I'd rather just merge it later with the what-are-we-sharing code.

> 
> @@ +2195,5 @@
> > +      StreamListeners* listeners = GetActiveWindows()->Get(windowID);
> > +      if (listeners) {
> > +        uint32_t length = listeners->Length();
> > +        for (uint32_t i = 0; i < length; ++i) {
> > +          nsRefPtr<GetUserMediaCallbackMediaStreamListener> listener =
> 
> could use auto& here instead of addref, or just:
> 
> listeners->ElementAt(i)->StopScreenWindowSharing();

That seems simpler.

> 
> @@ +2212,5 @@
> > +        nsCOMPtr<nsIDocShellTreeItem> item;
> > +        docShell->GetChildAt(i, getter_AddRefs(item));
> > +        nsCOMPtr<nsPIDOMWindow> win = item ? item->GetWindow() : nullptr;
> > +
> > +        StopScreensharing(win);
> 
> Looks like you could end up passing nullptr to StopScreensharing here which
> seems bad. Missing if?

Sure.

> 
> @@ +2264,5 @@
> > +      new MediaOperationRunnable(MEDIA_STOP,
> > +                                 this, nullptr, nullptr,
> > +                                 nullptr, mVideoSource,
> > +                                 mFinished, mWindowID, nullptr));
> > +    mMediaThread->Dispatch(runnable, NS_DISPATCH_NORMAL);
> 
> Why not call Invalidate() here?

Invalidate() kills audio as well (if they captured audio+screencapture).  Again, possibility for future refactoring

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Randell Jesup [:jesup] from comment #11)
> I'd rather just merge it later with the what-are-we-sharing code.

Sure, though MediaCaptureWindowStateInternal could benefit from the same treatment imho. i.e. make it take an innerWindow and have the outer MediaCaptureWindowState which you already have be the one that takes outerWindows.

> > Why not call Invalidate() here?
> 
> Invalidate() kills audio as well (if they captured audio+screencapture). 

Ah, missed that. Maybe add a "// leave audio" comment or something to guard that difference from future refactoring.

Comment 13

[Florian Quèze [:florian]]

Will "getUserMedia:revoke" without the 'screen:' prefix still stop both audio/video shares AND the screen share?

Comment 14

[Florian Quèze [:florian]]

Attachment: WIP2 patch for stopping only screen/window when asked

Testing shows the answer to my question in comment 13 is "yes". I'm afraid that's not what I would expect as a user.

The attached patch should work if my guesses about what attachment 8470670 [details] [diff] [review] tries to do are correct, but it doesn't work because of some inconsistencies:
- when sending getUserMedia:revoke without the screen prefix, MediaManagerService.mediaCaptureWindowState returns false immediately for all the values. But when using the screen: prefix, MediaManagerService.mediaCaptureWindowState still returns true for the screen value; I guess it's changed asynchronously.
- The "recording-window-ended" notification is sent when a web page stops a stream, but not when the user revokes permission.
- if the microphone and a screenshare are ongoing but have been requested separately, revoking the screenshare permission stops only the screenshare. But if the microphone and the screenshare were requested in a single gUM call, revoking the permission for the screenshare also stops the microphone.

These 3 issues combined make it challenging (if not impossible) to correctly remove the screen share icon from the UI. Fixing either of these 3 should give me a reasonably clean solution to remove the screenshare icon.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Florian Quèze [:florian] [:flo] from comment #13)
> Will "getUserMedia:revoke" without the 'screen:' prefix still stop both
> audio/video shares AND the screen share?

Yes

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Florian Quèze [:florian] [:flo] from comment #14)
> Created attachment 8472671 [details] [diff] [review]
> WIP2 patch for stopping only screen/window when asked
> 
> Testing shows the answer to my question in comment 13 is "yes". I'm afraid
> that's not what I would expect as a user.

Hmmm.  Does it say "camera, microphone and screen" or equivalent?  I'll note ending everything is it does today.  I can see changing the text (though that's a string/translation change).  I don't think it's *too* confusing, and I can't really see users wanting to end audio/video but not screen often (and I'm fine leaving those usecases to app ui.)

> The attached patch should work if my guesses about what attachment 8470670 [details] [diff] [review]
> [details] [diff] [review] tries to do are correct, but it doesn't work
> because of some inconsistencies:
> - when sending getUserMedia:revoke without the screen prefix,
> MediaManagerService.mediaCaptureWindowState returns false immediately for
> all the values. But when using the screen: prefix,
> MediaManagerService.mediaCaptureWindowState still returns true for the
> screen value; I guess it's changed asynchronously.

Yes, it submits MEDIA_STOP requests.

> - The "recording-window-ended" notification is sent when a web page stops a
> stream, but not when the user revokes permission.

That seems like a bug to me - let's fix that.

> - if the microphone and a screenshare are ongoing but have been requested
> separately, revoking the screenshare permission stops only the screenshare.
> But if the microphone and the screenshare were requested in a single gUM
> call, revoking the permission for the screenshare also stops the microphone.

Hmmm.  That shouldn't be the case, I think.  That would be a bug in this patch (perhaps we're marking the stream as Finished if we do a MEDIA_STOP without all parts being stopped.  I'll check that (I purposely didn't include the audio in the MEDIA_STOP, but I think I didn't check if it sets 'finished'.

> These 3 issues combined make it challenging (if not impossible) to correctly
> remove the screen share icon from the UI. Fixing either of these 3 should
> give me a reasonably clean solution to remove the screenshare icon.

I think two of these are pretty fixable.

Comment 17

[Florian Quèze [:florian]]

(In reply to Randell Jesup [:jesup] from comment #16)
> (In reply to Florian Quèze [:florian] [:flo] from comment #14)
> > Created attachment 8472671 [details] [diff] [review]
> > WIP2 patch for stopping only screen/window when asked
> > 
> > Testing shows the answer to my question in comment 13 is "yes". I'm afraid
> > that's not what I would expect as a user.
> 
> Hmmm.  Does it say "camera, microphone and screen" or equivalent?

No. We have 2 separate icons in the url bar, and they open separate doorhangers. One is about the camera and microphone, the other is about the screen/window sharing.

> I'll note ending everything is it does today.

Yes, I thought we agreed during the discussions before the implementation that ending all streams when clicking "stop sharing" on either of the doorhanger was 'good enough' for 33; I was surprised to see patches in this bug.

> I can see changing the text (though
> that's a string/translation change).

Not for 33. It's already at risk given our late in the cycle we are, but I thought we wanted to redesign this whole UI for 34 (I'm now thinking it's more likely to happen in 35) following the mockups in bug 1037162.

> I don't think it's *too* confusing,
> and I can't really see users wanting to end audio/video but not screen often
> (and I'm fine leaving those usecases to app ui.)

I think the app UI is the UI users should use most of the time to stop a stream, and using the chrome (and currently well hidden) "stop sharing" should only happen when the app is broken or the user no longer trusts it. If the user no longer trusts the app, I don't see a good reason to keep sharing some of the streams, so I think it makes sense to stop everything at once (ie. I would be fine with wontfixing this bug).

> > - The "recording-window-ended" notification is sent when a web page stops a
> > stream, but not when the user revokes permission.
> 
> That seems like a bug to me - let's fix that.

FYI, fixing this will cause some easy-to-fix failures in browser/base/content/test/general/browser_devices_get_user_media.js.

Fixing this should let me remove some existing hacks in the front-end code :-).

Comment 18

[Florian Quèze [:florian]]

(In reply to Randell Jesup [:jesup] from comment #16)

> > - The "recording-window-ended" notification is sent when a web page stops a
> > stream, but not when the user revokes permission.
> 
> That seems like a bug to me - let's fix that.

Filed bug 1056172 for this.

Comment 19

[Florian Quèze [:florian]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #8)

> @@ +2186,5 @@
> > +  nsCOMPtr<nsPIDOMWindow> piWin = do_QueryInterface(aWindow);
> > +  if (piWin) {
> > +    if (piWin->GetCurrentInnerWindow() || piWin->IsInnerWindow()) {
> > +      uint64_t windowID;
> > +      if (piWin->GetCurrentInnerWindow()) {
> 
> What happens if you call GetCurrentInnerWindow on an inner window?

Apparently on a debug build you crash with:
Assertion failure: IsOuterWindow(), at ../../../dist/include/nsPIDOMWindow.h:297

The stack is https://pastebin.mozilla.org/6082898

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Backend to allow stopping sharing for screen/window for a WindowID

Unbitrot due to bug 951991 landing

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8476687 [details] [diff] [review]
Backend to allow stopping sharing for screen/window for a WindowID

Carry forward r+=jib

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Randell Jesup [:jesup] from comment #21)
> Carry forward r+=jib

I'm unsure about my r+ given comment 19. I would like to see StopScreensharing refactored to take an aInnerWindow like I suggested in comment 10.

Comment 23

[Florian Quèze [:florian]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #22)
> (In reply to Randell Jesup [:jesup] from comment #21)
> > Carry forward r+=jib
> 
> I'm unsure about my r+ given comment 19.

comment 19 has been addressed, the crash I was seeing was indeed caused by the bitrot from bug 951991; we just discussed this in #media.

Comment 24

[Florian Quèze [:florian]]

Attachment: UI changes v3

Gijs, the removed code is a hack we no longer need now that we have a fix for bug 1056172.

I believe this is a correct browser/ patch, but the behavior is still not correct when both the screen and the microphone have been requested in a single gUM call and the user stops the screen share.
With the patches currently in the bug, both streams are stopped, but the microphone icon stays in the URL bar.
You said before (comment 16) that stopping both streams in that case was a bug.
The microphone icon not being removed is because the recording-window-ended notification is never fired when using the stop screen sharing code path. This also seems like something that should be fixed.

Comment 25

[:Gijs (he/him)]

Comment on attachment 8476714 [details] [diff] [review]
UI changes v3

Review of attachment 8476714 [details] [diff] [review]:
-----------------------------------------------------------------

r=me assuming the remaining issues wrt the firing of the notification and/or the closing of the microphone access when sending "screen:<id>" get resolved.

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Stop only screenshare if it's in a combined stream with audio

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8476806 [details] [diff] [review]
Stop only screenshare if it's in a combined stream with audio

Works; I noticed that if I stop only screen/window sharing that works and the shutdown message is sent (with isAudio=false, isVideo=true), but the screenshare icon doesn't go away (at top or in urlbar).  If I think end audio (which also ends screenshare if still active), the audio icon goes away but the screenshare (already stuck there) remains in the urlbar.  Note the top-of-screen indicator does go away at this point

Comment 28

[Florian Quèze [:florian]]

Adding here what I said on IRC so that people reading the bug can follow...

(In reply to Randell Jesup [:jesup] from comment #27)
> Comment on attachment 8476806 [details] [diff] [review]
> Stop only screenshare if it's in a combined stream with audio
> 
> Works; I noticed that if I stop only screen/window sharing that works and
> the shutdown message is sent (with isAudio=false, isVideo=true), but the
> screenshare icon doesn't go away (at top or in urlbar).

It's actually removed and re-added, because when the "recording-device-events" notification is fired, MediaManagerService.mediaCaptureWindowState still gives true for the screen.

> If I think end
> audio (which also ends screenshare if still active), the audio icon goes
> away but the screenshare (already stuck there) remains in the urlbar.  Note
> the top-of-screen indicator does go away at this point

Probably just that you need to apply locally the patch from bug 1056172.

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Stop only screenshare if it's in a combined stream with audio

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8476843 [details] [diff] [review]
Stop only screenshare if it's in a combined stream with audio

Now works (when combined with the patch on the other bug) in all situations.

Comment 31

[Florian Quèze [:florian]]

Comment on attachment 8476843 [details] [diff] [review]
Stop only screenshare if it's in a combined stream with audio

Review of attachment 8476843 [details] [diff] [review]:
-----------------------------------------------------------------

I can confirm that this patch works for me locally. You probably want someone more familiar with this code to have another look.

The issue of not sending the "recording-window-ended" notification when stopping the screen share and it was the last stream of the page remains, but it no longer has any user-visible effect, so we can address this in a follow-up.

Comment 32

[Daniel Veditz [:dveditz]]

I'm not sure what I'm being asked in the needinfo. From a privacy POV I don't mind this bug: if you say Stop Sharing that's what we do; there's no possibility of "failing open".

Combining the doorhangers as in bug 1037162 is a definite improvement. If we're looking for an interim state with the two separate doorhangers I don't mind stopping the screen sharing leaving the camera/mic shared (the UI on that is OK) but I do worry if stopping the camera/mic leave the screen shared because I don't think people expect that if they think that stopping the camera/mic is like hanging up the phone.

Comment 33

[Daniel Veditz [:dveditz]]

(In reply to Daniel Veditz [:dveditz] from comment #32)
> From a privacy POV I don't mind this bug

I do recognize that it IS a bug and sometimes does what people don't want. Just worried about trying to "fix" it without actually creating the decent UI we've planned elsewhere.

Comment 34

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8476843 [details] [diff] [review]
Stop only screenshare if it's in a combined stream with audio

Review of attachment 8476843 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nit.

::: dom/media/MediaManager.cpp
@@ +2260,5 @@
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
>    if (mVideoSource && !mStopped &&
>        (mVideoSource->GetMediaSource() == MediaSourceType::Screen ||
>         mVideoSource->GetMediaSource() == MediaSourceType::Window)) {

Don't you need && !mVideoSource->IsAvailable() here?

::: dom/media/MediaManager.h
@@ +120,5 @@
>    }
>    bool CapturingScreen()
>    {
>      NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
> +    return mVideoSource && !mStopped && !mVideoSource->IsAvailable() &&

Just so I understand, if mVideoSource is available i.e not busy, it means it has stopped?

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #34)
> Comment on attachment 8476843 [details] [diff] [review]
> Stop only screenshare if it's in a combined stream with audio
> 
> Review of attachment 8476843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nit.
> 
> ::: dom/media/MediaManager.cpp
> @@ +2260,5 @@
> >  {
> >    NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
> >    if (mVideoSource && !mStopped &&
> >        (mVideoSource->GetMediaSource() == MediaSourceType::Screen ||
> >         mVideoSource->GetMediaSource() == MediaSourceType::Window)) {
> 
> Don't you need && !mVideoSource->IsAvailable() here?

I'm concerned there might be race conditions (hasn't yet started because start request is in the queue), so shutting down anyways is safe and not a problem.

> 
> ::: dom/media/MediaManager.h
> @@ +120,5 @@
> >    }
> >    bool CapturingScreen()
> >    {
> >      NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
> > +    return mVideoSource && !mStopped && !mVideoSource->IsAvailable() &&
> 
> Just so I understand, if mVideoSource is available i.e not busy, it means it
> has stopped?

It means it's not currently in a running state (i.e. we've processed a STOP_TRACK command, not just asked for it).

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e35629e7cdb
https://hg.mozilla.org/integration/mozilla-inbound/rev/c365e93b8e42

ni? mreavy on uplift

Comment 37

[Randell Jesup [:jesup] (needinfo me)]

nigel backed out for Windows M1 dt1/dt2 X oranges (perhaps because I forgot to also land Bug 1056172)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2e87c61067a

Comment 38

[Randell Jesup [:jesup] (needinfo me)]

relanded by sheriffs because the oranges weren't ours:
https://hg.mozilla.org/integration/mozilla-inbound/rev/727f3b86e5e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa81a4c7381b

Comment 39

[Randell Jesup [:jesup] (needinfo me)]

We'll want this for 33

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/727f3b86e5e1
https://hg.mozilla.org/mozilla-central/rev/aa81a4c7381b

Comment 41

[Florian Quèze [:florian]]

Comment on attachment 8476687 [details] [diff] [review]
Backend to allow stopping sharing for screen/window for a WindowID

Approval Request Comment
[Feature/regressing bug #]: screensharing, which landed in 33. Per comment 39, this is a fix we would like to uplift.
[User impact if declined]: Clicking "stop sharing" in the screensharing doorhanger will also stop the camera and microphone streams.
[Describe test coverage new/current, TBPL]: The fix has backed on m-c for a week.
[Risks and why]: acceptable for aurora. Note: we need to uplift bug 1056172 at the same time.
[String/UUID change made/needed]: none.

Comment 42

[Florian Quèze [:florian]]

Note: My approval request in comment 41 is actually for the 2 changesets landed in comment 40; not for attachment 8476687 [details] [diff] [review] specifically.

Comment 43

[Florian Quèze [:florian]]

Attachment: attachment 8476687 and attachment 8476843, as landed on m-c

Approval Request Comment
See comment 41.

Comment 44

[Florian Quèze [:florian]]

Comment on attachment 8476714 [details] [diff] [review]
UI changes v3

Approval Request Comment
See comment 41.

Comment 45

[Florian Quèze [:florian]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3746bf3edf57
https://hg.mozilla.org/releases/mozilla-aurora/rev/651f5e00e21c

Comment 46

[Bogdan Maris, Desktop QA]

Testing on latest Nightly and latest Aurora, Ubuntu 12.04 32bit, Mac OS X 10.9.4 and Windows 7 64bit. Closing sharing does not close the camera but closing camera will close the sharing as well. Is this gonna be fixed in another bug or should I log a new bug?

Comment 47

[Randell Jesup [:jesup] (needinfo me)]

bogdan: that's the behavior we agreed on for 33 & 34.  The potential for user confusion with the current UI if the user stops audio but leaves screensharing active is too large.  The major UI redesign being worked on will change all these issues.

Comment 48

[Bogdan Maris, Desktop QA]

(In reply to Randell Jesup [:jesup] from comment #47)
> bogdan: that's the behavior we agreed on for 33 & 34.  The potential for
> user confusion with the current UI if the user stops audio but leaves
> screensharing active is too large.  The major UI redesign being worked on
> will change all these issues.

Thanks Randell.
Then I will close this issue as verified since I verified that closing screensharing does not affect video/audio.