1371000 - Firefox not seeing all windows available to share

Status: VERIFIED FIXED

(Core :: WebRTC, defect, P1)

Description

[johan.claudebreuninger]

Attachment: Screenshot_20170607_204301.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170520072017

Steps to reproduce:

Tried to share a window from my computer using WebRTC but the list is incomplete, and only shows Mozilla software.

Tested in a Jitsi instance and with the gUM Test Page


Actual results:

Windows not listed.

Look at the attached screeshot. A lot of windows are open in the task bar but do not show in Firefox's list.


Expected results:

Full list of available windows.

Comment 1

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

There is no filter to show only Mozilla software. Can you do us a favor and type up a list of the windows shown and a list of windows that are not shown? It's hard to tell from the screenshot. Thanks.

Comment 2

[johan.claudebreuninger]

Hello Jan,

Sorry for not providing enough details. In the example from my screenshot I have following windows/programs opened:
-Dolphin
-Firefox
-Kate
-KCalc
-Konsole
-Thunderbird

The only shown windows are Firefox and Thunderbird.

Comment 3

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

Thanks! Is there anyone with linux who can reproduce this?

It would also help to know if this is a regression. Do we know if this worked in earlier versions of Firefox on linux? Window sharing has been implemented for a long time, going back to version 33, though it required a whitelist before 52. 

If this used to work, the next step is to establish a regression-range with https://mozilla.github.io/mozregression/

Comment 4

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

Michael, I see you verified window sharing on linux in bug 1336723, could you see if you can reproduce this?

Comment 5

[johan.claudebreuninger]

I just tested on TrueOS and I am not seeing all the windows either.

Comment 6

[Michael Froman [:mjf]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #4)
> Michael, I see you verified window sharing on linux in bug 1336723, could
> you see if you can reproduce this?

My linux box is temporarily offline, but I'll bring it up as soon as possible and check.  Leaving needinfo on for me as a reminder.  Should be later today or tomorrow.

Comment 7

[Michael Froman [:mjf]]

I'm on Ubuntu 16.04, Firefox 53.0.3.  Using the gUM Test Page I see non-Mozilla windows in the list.  However, in order to get the list to update, I had to restart Firefox.  Simply opening a new tab, or a new window did not cause the list of windows to update.  I'll attach a screen shot to show my window list.

Comment 8

[Michael Froman [:mjf]]

Attachment: screen shot showing non-Mozilla windows in share window list

Here is the screen shot showing non-Mozilla windows in the list

Comment 9

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

Johan, does comment 7 explain your symptom? Did you open the other windows after Firefox, and do you see them in the list if you open them before opening Firefox?

Note I'm not saying this is ok, it just helps triage this.

Comment 10

[Nico Grunbaum [:ng, @chew:mozilla.org]]

I attempted to repro this in on Ubuntu 16.04.2 LTS with Firefox 53.03, and found the same behavior with the added caveat that closed windows still appeared in the list. Selecting a closed window failed to capture.

Comment 11

[Nico Grunbaum [:ng, @chew:mozilla.org]]

The same behavior exists in the webrtc.org 57 merge Firefox branch.

Comment 12

[johan.claudebreuninger]

Hello Jan,

We are on a way to a solution here. While restarting Firefox helps a lot - and shows non Mozilla windows - I'm still missing a few.

I am approximately missing a third of the windows.

Comment 13

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

Are the missing windows minimized by any chance?

Comment 14

[johan.claudebreuninger]

Minimized windows don't seem to have an impact. I have been making some tests yesterday with very weird results. If I close and open Firefox without even touching my windows, some are not shown anymore, and some others appear.

Comment 15

[Nico Grunbaum [:ng, @chew:mozilla.org]]

I checked on my laptop and this behavior also occurs on OS X. I suspect it is platform independent.

Comment 16

[johan.claudebreuninger]

Could it be X11 related? Last time I used OS X it was still the default server.

Comment 17

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1371000 - add expiration to noncamera device info;

Review commit: https://reviewboard.mozilla.org/r/148748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148748/

Comment 18

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

Comment on attachment 8877362 [details]
Bug 1371000 - add expiration to noncamera device info;

https://reviewboard.mozilla.org/r/148748/#review153188

Not the best design perhaps to have 184 IPC calls on a single gUM call, but until we change it, the design very much relies on this being cached.

::: dom/media/systemservices/VideoEngine.cpp:75
(Diff revision 1)
>  std::shared_ptr<webrtc::VideoCaptureModule::DeviceInfo>
>  VideoEngine::GetOrCreateVideoCaptureDeviceInfo() {
> +  switch (mCaptureDevInfo.type) {
> +    case webrtc::CaptureDeviceType::Camera: {
> -  if (mDeviceInfo) {
> +      if (mDeviceInfo) {
> -    return mDeviceInfo;
> +        return mDeviceInfo;
> -  }
> +      }

GetOrCreateVideoCaptureDeviceInfo() is called from RecvGetCaptureCapability() which is called 184 times on a single gUM call.

Comment 19

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

Oh wait, I tested camera, not screenshare, so it may not be a problem after all, but I'd still like to have a better understanding on how often this is called for screenshare.

Comment 20

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

It's called a lot less for screensharing, ~10 times, but that's still too high. Basically it's called for every IPC method to CamerasParent. Someday we should rethink that design.

Comment 21

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

I tested on Fedora 25 with nightly from ~ 5 days ago.  All windows, mozilla and non-mozilla, are shown.  Opening a new non-mozilla window and trying Window on the gum_test page again shows the new window (and sharing it works).

I rebuilt inbound on 6/13, and I see the problem - new windows don't show up.

Comment 22

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

Downloading a m-c build from 6-5 shows the problem too.  Regression-range says between 2016-12-17 and 2017-1-5,which points strongly to the v49 landing.  Note sure why it worked for me earlier

Comment 23

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8877362 [details]
Bug 1371000 - add expiration to noncamera device info;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148748/diff/1-2/

Comment 24

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

Randell, are you able to get a narrower range with mozregression?

Comment 25

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

Comment on attachment 8877362 [details]
Bug 1371000 - add expiration to noncamera device info;

https://reviewboard.mozilla.org/r/148748/#review154002

This could work, though I have a question about the cache window logic, as it seems more elaborate than necessary to me, though I may be missing something.

::: dom/media/systemservices/VideoEngine.h:30
(Diff revision 2)
> +  // Note because cameras use HW plug event detection, this
> +  //   only applies to screen based modes.

nit: odd indent

::: dom/media/systemservices/VideoEngine.h:34
(Diff revision 2)
> +  // Base cache validity period
> +  // Note because cameras use HW plug event detection, this
> +  //   only applies to screen based modes.
> +  static const int64_t kBaseExpiryPeriodMs = 500;
> +  // The number of times the expiry window can slide before forced expiration
> +  static const int kMaxEpiryExtensions = 10;

typo

::: dom/media/systemservices/VideoEngine.h:53
(Diff revision 2)
> -  /** Returns or creates a new new DeviceInfo.
> -  *   It is cached to prevent repeated lengthy polling for "realness"
> -  *   of the hardware devices.  This could be handled in a more elegant
> -  *   way in the future.
> +  /** Returns an existing or creates a new new DeviceInfo.
> +  *   Camera info is cached to prevent repeated lengthy polling for "realness"
> +  *   of the hardware devices.  Other types of capture, e.g. screen share info,
> +  *   is not cached. This could be handled in a more elegant way in the future.

update "not cached" language

::: dom/media/systemservices/VideoEngine.cpp:89
(Diff revision 2)
> +    // Screen sharing cache is invalidated by expiry over a sliding window
> +    // with an enforced maximum size.
> +    currentTime = webrtc::Clock::GetRealTimeClock()->TimeInMilliseconds();
> +    if (currentTime <= mExpiryTimeInMs) {
> +      if (++mExpiryExtensions <= kMaxEpiryExtensions) {
> +        mExpiryTimeInMs += kExpiryExtentionMs;
> +      }

From what I can tell, this extends the "caching" window ("expiry window" seems like a misnomer) from 500 ms to 1000 ms if I call the method 10 or more times within the first 500 ms, which we know is typical.

What problem does this logic solve over a simpler 1 second max?

Comment 26

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #24)
> Randell, are you able to get a narrower range with mozregression?

12/27 works
v49 landed 12/28
12/29 fails

Comment 27

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8877362 [details]
Bug 1371000 - add expiration to noncamera device info;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148748/diff/2-3/

Comment 28

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

Comment on attachment 8877362 [details]
Bug 1371000 - add expiration to noncamera device info;

https://reviewboard.mozilla.org/r/148748/#review154640

lgtm.

::: dom/media/systemservices/VideoEngine.cpp:89
(Diff revision 3)
> +    // Screen sharing cache is invalidated by expiry over a sliding window
> +    // with an enforced maximum size.

window is no longer sliding.

Comment 29

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8877362 [details]
Bug 1371000 - add expiration to noncamera device info;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148748/diff/3-4/

Comment 30

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8877362 [details]
Bug 1371000 - add expiration to noncamera device info;

https://reviewboard.mozilla.org/r/148748/#review155306

Comment 31

[ychoo]

I'm seeing similar problem with Windows 7 and Windows 10. Window sharing list is much shorter than application sharing.

Comment 32

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

Nico/jib: can we land this?  We've missed possible uplift to 55.

Comment 33

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Yes, I have it running on try now.

Comment 34

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0738a483dee8043e7ef7eb1fb42a21f69f13817

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b3d54ec727b
add expiration to noncamera device info;r=jib

Comment 36

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

https://hg.mozilla.org/mozilla-central/rev/3b3d54ec727b

Comment 37

[Ryan VanderMeulen [:RyanVM]]

(In reply to Randell Jesup [:jesup] from comment #32)
> Nico/jib: can we land this?  We've missed possible uplift to 55.

There's still two more betas in this cycle (one more week). Is this too risky for uplift or?

Comment 38

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

question for jib as well. Not a couple-liner, though pretty straightforward if it's careful about holding refs needed by the timer and there aren't any thread races, and I think there aren't.  I'd be ok with this perhaps with some manual checks.

Comment 39

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

Looks ok to me. No threads or timer references here since it just checks against previous GetRealTimeClock() values. Provided those counters are never reset for any reason, we should be safe.

The change seems well-contained to screensharing, as the more common camera case bypasses the new logic, and works as before.

Definitely needs manual testing, since there's no automated coverage.

TESTING TIPS:

Set MOZ_LOG="timestamp:1,VideoEngine:4" and verify log messages from the patch in the log.

We should test both camera and screensharing (multiple types) with e.g. http://mozilla.github.io/webrtc-landing/

In short, since this low-level code is called several times during a single request, we pretty much want the cache to work all the time for cameras, and within the same request for screensharing (but not across multiple screensharing requests).

Ensure:
 1. the cache is never reset for cameras,
 2. the cache is never reset within a single screensharing request (<1s)
 3. the cache is always reset between individual screensharing requests (>1s)
 4. User sees new windows in drop down
 5. camera capture is not sluggish

Comment 40

[Nico Grunbaum [:ng, @chew:mozilla.org]]

I am running through the "Ensure" list above using https://mozilla.github.io/webrtc-landing/gum_test.html

1. Tested by selecting the "video" option on the "getUserMedia Test Page", allowing access to the camera, then refreshing and repeating. In 14 consecutive tries only the first resulted in a new device info being created.

2. & 3. Tested by running the "window" test on the "getUserMedia Test Page", then hitting refresh and running the "window" test again.  Afterwards I inspected the logs. This appears to be working as intended.*

4. Tested by closing most windows on the system, triggering the window sharing gUM dialog by selecting "window", and checking the drop down list. Then I opened a number of new windows and refreshed the "getUserMedia Test Page", selected "window" again, and checked the drop down list. The VideoEngine logs show a device info creation event followed by a number of cache hits. Then some seconds later another device info creation event occurs followed a larger number of cache hits than the first.

5. I noticed no unexpected sluggishness when testing #1

* Something I did not expect is that a device info is being created right before a VideoEngine is deleted. I don't have any evidence that this is related to the patch, I will investigate further tomorrow.

Comment 41

[Nico Grunbaum [:ng, @chew:mozilla.org]]

The device info creation before deletion of the VideoEngine is done so that any event handlers can be removed. It is not related to this patch.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Was someone going to request Beta uplift on this patch?

Comment 43

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8877362 [details]
Bug 1371000 - add expiration to noncamera device info;

Approval Request Comment
[Feature/Bug causing the regression]: bug 1371000
[User impact if declined]: Users will have to restart the browser to refresh the windows available in gUM (getUserMedia) dialogs
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: I conducted manual testing as outlined in detail in comment 40
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: medium risk
[Why is the change risky/not risky?]: it is not a trival patch, but it is well contained. It could cause regressions in gUM behavior.
[String changes made/needed]: none

Comment 44

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8877362 [details]
Bug 1371000 - add expiration to noncamera device info;

Seems too last minute for 55, we're building release candidates today

Comment 47

[Bogdan Maris, Desktop QA]

I reproduced this issue on Ubuntu 16.04 32bit using old Nightly from 2017-07-20, verified that the newly opened windows (or recently closed don't) appear in latest Firefox 56 beta 11 after reloading the website or if clicking for the first time on window sharing after first opening Fx and then opening other apps.