Closed Bug 1098437 Opened 10 years ago Closed 9 years ago

The audio & video are still shared to the caller, even after receiver clicks on the "stop sharing" button

Categories

(Hello (Loop) :: Client, defect, P1)

x86_64
Windows 7
defect

Tracking

(firefox34 unaffected, firefox35+ verified, firefox36+ verified, firefox37+ verified)

VERIFIED FIXED
mozilla37
Tracking Status
firefox34 --- unaffected
firefox35 + verified
firefox36 + verified
firefox37 + verified
backlog Fx35+

People

(Reporter: pauly, Assigned: mixedpuppy)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

STR:
1. Click on "Start a conversation" icon and copy a call URL
2. Share the URL with caller and have them start the call
3. Accept the incoming call
4. Click the camera icon in the conversation title bar
5. Click on the down arrow at the "continue sharing" button and click on "stop sharing"

Expected Results:
The call should be interrupted (audio/video should stop sharing) at the other end.

Actual Results:
The audio & video are still shared to the caller.
It seems to be a regression, works fine in FF 34b8 but NOT in 36.0a1 (2014-11-13), 35.0a2 (2014-11-13)
Keywords: regression
OS: All → Windows 7
Hardware: All → x86_64
[Tracking Requested - why for this release]:
This is a privacy issue (video/audio continues to be shared after the user has requested to stop sharing).

I just repro'd myself on Nightly.  

Florian, Mark -- This is specific to Loop/Hello.  I just did a talky.io call and the "stop sharing" button works fine there.  The Loop UI seems to believe we have stopped sharing, but the video/audio continues to go out on the wire.
backlog: --- → Fx35+
Flags: needinfo?(standard8)
Flags: needinfo?(florian)
Priority: -- → P1
Mozregression says:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5bd6e09f074e&tochange=1abe619810bb

The most obvious thing that stands out there is:

Bug 973001 - getUserMedia UI doesn't work with e10s - fix the urlbar indicators and show the global indicators

which touched webrtcUI.jsm which is where this is likely to be.

Possible affecting changesets:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba660b435bb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba660b435bb5

I could probably take a look sometime next week.
Blocks: 973001
Flags: needinfo?(standard8)
Attached patch WIP (obsolete) — Splinter Review
The problem is that for chat windows, nothing is listening to the webrtc:StopSharing messages sent by webrtcUI.jsm.

The attached patch loads a simple frame script into chat window browsers to setup the listeners.

The patch works for floating chat windows, but doesn't work anymore if one detaches the chat window to a standalone window. After reattaching the chat window, things still don't work, so I suspect the problem is with handling of framescript while swapping the docshells rather than a problem related to the code of the standalone chat window.
Flags: needinfo?(florian)
Attachment #8524614 - Flags: feedback?(mixedpuppy)
Florian -- Can I assign this to you?
Flags: needinfo?(florian)
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #5)
> Florian -- Can I assign this to you?

Yes, if you don't mind potentially waiting a few more days for a full fix, or using only the partial fix here and having us handle the detached chat window case in a follow-up.
Assignee: nobody → florian
Flags: needinfo?(florian)
Comment on attachment 8524614 [details] [diff] [review]
WIP

I think we may need to load content.js.  eg. content.js has handlers for full screen as well as webrtc.  Can you look into that?
Attachment #8524614 - Flags: feedback?(mixedpuppy) → feedback+
(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> Created attachment 8524614 [details] [diff] [review]
> WIP
> 
> The problem is that for chat windows, nothing is listening to the
> webrtc:StopSharing messages sent by webrtcUI.jsm.
> 
> The attached patch loads a simple frame script into chat window browsers to
> setup the listeners.
> 
> The patch works for floating chat windows, but doesn't work anymore if one
> detaches the chat window to a standalone window. After reattaching the chat
> window, things still don't work, so I suspect the problem is with handling
> of framescript while swapping the docshells rather than a problem related to
> the code of the standalone chat window.

Just noticed your comments...I found (in bug 1095457) that I had to load the content script in the LOAD event, not earlier.
[Tracking Requested - why for this release]: this should be tracked since it's a regression and carries privacy implications.
Depends on: 1101074
Hi Shane -- Do you have any bandwidth to finish off Florian's WIP patch?   I need it for Fx35.  Florian is currently swamped with other work.
Flags: needinfo?(mixedpuppy)
I've taken a look at this, it's not as trivial as I initially thought, but I'll spend a bit more time on it today and tomorrow, see if I can get it resolved.  The initial patch fixes one aspect of the problem but does not fix the issue when attaching/detaching the chat window.  

I also notice another problem (with patch applied) which is that the camera button goes away after you "stop sharing", but the chat window remains.  You can still see the other person, but you have no way to reactivate sharing of the camera/mic.  You then have to close the window.  Someone will need to look into the UX on that, and either it's a bug (ie. the icon should remain and you can restart sharing) with the permissions prompt or a bug in UX (eg. should the window close and call end if you stop sharing).  

@mreavy someone will need to decide what the proper behavior is.  In either case, I'm not sure if I can address it in time before uplift.  This is of course, less critical than making sure "stop sharing" works.
Flags: needinfo?(mixedpuppy) → needinfo?(mreavy)
(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> You
> can still see the other person, but you have no way to reactivate sharing of
> the camera/mic.  You then have to close the window.

This is a Loop bug, not bug of SocialAPI or the WebRTC UI.

> @mreavy someone will need to decide what the proper behavior is.

The icon disappearing when you are no longer sharing is the expected behavior.

In the future we'll enhance the doorhanger to also allow muting (which will keep the icon visible).
(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #11)

> The icon disappearing when you are no longer sharing is the expected
> behavior.

That is a incredibly awkward UX, it leaves the user in a "what do I do now?" state which is never good.
Attached patch loop camera permissions (obsolete) — Splinter Review
This uses content.js in the chat window rather than a custom content script, since content.js will support other things such as fullscreen notifications.  It also fixes stopping sharing from a detached (and re-attached) window.
Flags: needinfo?(mreavy)
Attachment #8529237 - Flags: feedback?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> This is a Loop bug, not bug of SocialAPI or the WebRTC UI.

Agreed -- however, I'll note that allowing the session to be reestablished after the camera and/or microphone are revoked will require support from the OpenTok SDK. It's not clear whether we'll get an "accessDenied" event in these circumstances; the SDK documentation is a bit unclear on this point (see <https://tokbox.com/opentok/libraries/client/js/reference/Publisher.html#events>).
(In reply to Adam Roach [:abr] from comment #15)
> (In reply to Florian Quèze [:florian] [:flo] from comment #12)
> > This is a Loop bug, not bug of SocialAPI or the WebRTC UI.
> 
> Agreed -- however, I'll note that allowing the session to be reestablished
> after the camera and/or microphone are revoked will require support from the
> OpenTok SDK. 

IMO just closing the window is acceptable, if you cannot re-establish access to the hardware.
Hi Shane, are you going to be able to finish this off this week so we can uplift early in beta?  Please work with Maire if not.
Severity: normal → critical
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy)
Attachment #8529237 - Flags: review?(mhammond)
Comment on attachment 8529237 [details] [diff] [review]
loop camera permissions

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

LGTM if Florian rubber-stamps it.
Attachment #8529237 - Flags: review?(mhammond)
Attachment #8529237 - Flags: review?(florian)
Attachment #8529237 - Flags: review+
Attachment #8529237 - Flags: feedback?(florian)
Depends on: 1107967
Comment on attachment 8529237 [details] [diff] [review]
loop camera permissions

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

::: browser/base/content/socialchat.xml
@@ +58,5 @@
> +        this.addEventListener("load", function loaded(event) {
> +          this.removeEventListener("load", loaded, true);
> +          let mm = this.content.messageManager;
> +          mm.loadFrameScript("chrome://browser/content/content.js", true);
> +        }, true);

This part looks good to me and fixes the bug here.

@@ +153,5 @@
>            aTarget.content.popupnotificationanchor.className = this.content.popupnotificationanchor.className;
>            this.content.swapDocShells(aTarget.content);
> +
> +          // make sure the webrtc camera icon is updated properly for the new window
> +          Services.obs.notifyObservers(null, "recording-device-events", null);

I've just verified that "Stop sharing" on a teared off regular tab is similarly broken, so we shouldn't add a workaround in the SocialAPI code. I filed bug 1107967 for this regression, and expect to take care of it next week.
Attachment #8529237 - Flags: review?(florian) → review-
Comment on attachment 8529237 [details] [diff] [review]
loop camera permissions

Apparently my r- here was confusing to Maire. r=me for the first part of the patch, as long as the |Services.obs.notifyObservers(null, "recording-device-events", ...| workaround doesn't land.
Attachment #8529237 - Flags: review- → review+
Shane -- Thanks for fixing this.  Can you land it and ask for uplift approval?  (I want to get this fix into Fx35 and Fx36, Beta and Aurora respectively.)
Assignee: florian → mixedpuppy
Flags: needinfo?(mixedpuppy)
revised based on last comment, carry forward r+

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7ad4e1e78e8f
Attachment #8524614 - Attachment is obsolete: true
Attachment #8529237 - Attachment is obsolete: true
Flags: needinfo?(mixedpuppy)
Attachment #8533856 - Flags: review+
Comment on attachment 8533856 [details] [diff] [review]
loop camera permissions

Approval Request Comment
[Feature/regressing bug #]: loop
[User impact if declined]: privacy issue, camera/mic continue to work after disabling them if the chat window has had a docshell swap.
[Describe test coverage new/current, TBPL]: tbpl
[Risks and why]: low, loading content.js enables some stuff that is necessary at the window level (webrtc, full screen, etc)
[String/UUID change made/needed]: none
Attachment #8533856 - Flags: approval-mozilla-beta?
Attachment #8533856 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d8acfeb6f12c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attachment #8533856 - Flags: approval-mozilla-beta?
Attachment #8533856 - Flags: approval-mozilla-beta+
Attachment #8533856 - Flags: approval-mozilla-aurora?
Attachment #8533856 - Flags: approval-mozilla-aurora+
Verified fixed FF 36.0a2 (2014-12-15), 37.0a1 (2014-12-14) Win 7
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Verified fixed FF 35b4 Win 7
You need to log in before you can comment on or make changes to this bug.