Closed Bug 1182040 Opened 4 years ago Closed 4 years ago

Tab sharing no longer changes tab when the user switches tabs

Categories

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

defect
Points:
1

Tracking

(firefox39 unaffected, firefox40 unaffected, firefox41+ verified, firefox42+ fixed)

RESOLVED FIXED
mozilla42
Iteration:
42.2 - Jul 27
Tracking Status
firefox39 --- unaffected
firefox40 --- unaffected
firefox41 + verified
firefox42 + fixed

People

(Reporter: standard8, Assigned: mikedeboer)

Details

(Keywords: regression, Whiteboard: [sharing bug])

Attachments

(3 files)

[Tracking Requested - why for this release]:

We've noticed that tab sharing has stopped updating the remote user when the local user switches tabs.

STR:

1) Set your browser into non-e10s mode (e10s tab sharing isn't enabled currently).
2) Set up a Hello conversation
3) Open several tabs
4) Share your tabs
5) Switch your active tab

Expected Results:

- The remote person sees the tab changes.

Actual Results:

- Sharing stays on the same view and doesn't recover.
- There's this message on the browser console for the person doing the sharing:

TypeError: sender.switchTracks is not a function
I've tracked the regression range to in-between 40 and 41.

Michael, we changed the sdk from 2.5.0 to 2.5.1 between those versions. From looking at the code, I think this could be an issue within the sdk, but I'm not 100% sure.

Please could you take a look?
Flags: needinfo?(msander)
[Tracking Requested - why for this release]: Regression in functionality of tab sharing - no longer updates after a user switches tabs.
Rank: 18
Tracking because regression.
(In reply to Mark Banner (:standard8) from comment #1)
> I've tracked the regression range to in-between 40 and 41.
> 
> Michael, we changed the sdk from 2.5.0 to 2.5.1 between those versions. From
> looking at the code, I think this could be an issue within the sdk, but I'm
> not 100% sure.
> 
> Please could you take a look?

I can reproduce this issue and have filed an internal bug. I'll give a further update soon and ETA for a fix.
Flags: needinfo?(msander)
Should be fixed in this beta version of the SDK, which will be released next week. We are still finishing up some regression testing and bug verifications so please do not add this to your source tree.

I wasn't able to test with tab switching however, so please let me know if there are still problems.
(In reply to msander from comment #5)
> Should be fixed in this beta version of the SDK, which will be released next
> week. We are still finishing up some regression testing and bug
> verifications so please do not add this to your source tree.
> 
> I wasn't able to test with tab switching however, so please let me know if
> there are still problems.

I've just tested this against the latest nightly and it seems to work fine. I did a few other verifications but will do a few more next week when we know what all the changes to the sdk are.
Flags: qe-verify+
Flags: firefox-backlog+
Whiteboard: [sharing bug]
Michael, is there a simple patch that we can use that just fixes the stream switching issue on 2.5.1? This'd make it a whole lot more easy to uplift all the things to Fx 40!
Flags: needinfo?(msander)
Yes, I'll get one to you by the end of day tomorrow.
Flags: needinfo?(msander)
(In reply to msander from comment #8)
> Yes, I'll get one to you by the end of day tomorrow.

\o/ Awesome!
Attached file webrtc-js-2.5.2.tgz
Really the only change is

@@ -25970,9 +25970,9 @@
               var peerConnection = _peerConnections[connectionId];
               peerConnection.getSenders().forEach(function(sender) {
                 if (sender.track.kind === 'audio' && newStream.getAudioTracks().length) {
-                  replacePromises.push(sender.replaceTrack(newStream.getAudioTracks()[0]));
+                  replacePromises.push(sender.switchTracks(newStream.getAudioTracks()[0]));
                 } else if (sender.track.kind === 'video' && newStream.getVideoTracks().length) {
-                  replacePromises.push(sender.replaceTrack(newStream.getVideoTracks()[0]));
+                  replacePromises.push(sender.switchTracks(newStream.getVideoTracks()[0]));
                 }
               });
             });
Diff is backwards but you get the idea :)
There is some code that moved around but did not change.
The only real change is what Michael mentioned in comment 10.

Thanks Michael!
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Attachment #8634014 - Flags: review?(dmose)
Iteration: --- → 42.2 - Jul 27
Points: --- → 1
Comment on attachment 8634014 [details] [diff] [review]
Patch v1: upgrade TokBox SDK to 2.5.2

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

I haven't tested this, but based on Mike's and Michael's comments, it seems low risk to me.  rs=dmose
Attachment #8634014 - Flags: review?(dmose) → review+
https://hg.mozilla.org/mozilla-central/rev/f4c1152a490a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
QA Contact: bogdan.maris
Comment on attachment 8634014 [details] [diff] [review]
Patch v1: upgrade TokBox SDK to 2.5.2

Approval Request Comment
[Feature/regressing bug #]: bug 	1108892, which introduced text chat in conversations, based on peer-to-peer data-channels. The TokBox SDK that was updated to support this contained a regression.
[User impact if declined]: Whilst sharing browser tabs during a conversation, the stream will not follow the active tab - as would be expected.
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: minor, very small change to the SDK.
[String/UUID change made/needed]: n/a.
Attachment #8634014 - Flags: approval-mozilla-aurora?
Comment on attachment 8634014 [details] [diff] [review]
Patch v1: upgrade TokBox SDK to 2.5.2

Even though the patch has a lot of change, most of it is due to the SDK update. Mike has confirmed. Let's land this on Aurora.
Attachment #8634014 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reproduced the initial issue using old Nightly (from 2015-07-09), verified that using latest Aurora the issue does not reproduce anymore across platforms (Windows 10 32-bit, Ubuntu 14.04 32-bit and Mac OS X 10.10.4).
Verification on Aurora 41 should be enough here I think.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.