1056213 - Window title in "windows or screen to share" list is not updated when navigating to another page

Status: VERIFIED FIXED

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

Description

[Philipp Hancke]

steps to reproduce:
1) navigate to https://fippo.github.io/webrtc-landing/gum_test_fippo3.html
2) share a window (queries camera first)
3) navigate to https://talky.io, join a room
4) click "share screen"
5) note the "GUM Test Page - Nightly" in the "Window or screen to share:" list. This is from step 1 and not reflecting the current page title.

This looks like the list is generated during step 1 and cached.

Comment 1

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

The full list isn't cached (open a new window after 3 and before 4, and you'll see it in the list), but it appears it doesn't update the titles of existing windows when it Rescan()s.

Comment 2

[Maire Reavy [:mreavy]]

Roman, Gian-Carlo, or Matt -- Would any of your feel comfortable taking this bug?  It's a follow on to the rescan bug (Bug 1041369) which we uplifted to Fx33.  I'd love to get this fixed within the next week (or sooner) and uplifted before we go to Beta, if possible.

Comment 3

[Matthew Miller [:m_and_m]]

I can take a look at it next week.

Comment 4

[Maire Reavy [:mreavy]]

Thanks, Matt!

Comment 5

[Matthew Miller [:m_and_m]]

Attachment: Window title in "windows or screen to share" list is not updated when navigating to another page

Comment 6

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

Comment on attachment 8480140 [details] [diff] [review]
Window title in "windows or screen to share" list is not updated when navigating to another page

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

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +703,5 @@
> +void MediaEngineWebRTCVideoSource::Refresh(int aIndex) {
> +#ifdef MOZ_B2G_CAMERA
> +  mCaptureIndex = aIndex;
> +#else
> +  mCaptureIndex = aIndex;

a) hoist this outside the #ifdef
b) would this actually ever be different?  If it can't, then remove the parameter; if you're worried it might but shouldn't, MOZ_ASSERT or NS_WARN_IF

@@ +705,5 @@
> +  mCaptureIndex = aIndex;
> +#else
> +  mCaptureIndex = aIndex;
> +
> +  // assume the uniqueId hasn't changed ...

"Caller ensures this is the same UniqueId" or "Caller looked up this captureIndex by uniqueID, so it shouldn't change"

Comment 7

[Matthew Miller [:m_and_m]]

(In reply to Randell Jesup [:jesup] from comment #6)
> Comment on attachment 8480140 [details] [diff] [review]
> Window title in "windows or screen to share" list is not updated when
> navigating to another page
> 
> Review of attachment 8480140 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
> @@ +703,5 @@
> > +void MediaEngineWebRTCVideoSource::Refresh(int aIndex) {
> > +#ifdef MOZ_B2G_CAMERA
> > +  mCaptureIndex = aIndex;
> > +#else
> > +  mCaptureIndex = aIndex;
> 
> a) hoist this outside the #ifdef
> b) would this actually ever be different?  If it can't, then remove the
> parameter; if you're worried it might but shouldn't, MOZ_ASSERT or NS_WARN_IF
> 

The index actually does change, at least for Mac when enumerating windows.

Comment 8

[Matthew Miller [:m_and_m]]

Attachment: Window title in "windows or screen to share" list is not updated when navigating to another page

Comment 9

[Matthew Miller [:m_and_m]]

Attachment: 8480175: Window title in "windows or screen to share" list is not updated when navigating to another page

The previous patch had a stupid typo (#ifndef vs. #ifdef) that I thought hadn't been committed, but was.

My apologies for the noise /-:

Comment 10

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

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

Comment 11

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

Backed out for various M3 failures (semi-random, UAF or leaving something running/leaked).
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ef2bf9308ed7

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fb2483f4928

Comment 12

[Matthew Miller [:m_and_m]]

Attachment: v4 - Window title in "windows or screen to share" list is not updated when navigating to another page

Patch to address Mochitest-3 failures.  mCaptureIndex is changed when a device is allocated, meaning the enumeration index can be different.  However, the enumeration index is what is used to get the current device name.

Try of the interdiff: < https://tbpl.mozilla.org/?tree=Try&rev=3e0291fd3379 >
Failures in that run appear to be unrelated...

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9024c845a9

Comment 14

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

Comment on attachment 8481881 [details] [diff] [review]
v4 - Window title in "windows or screen to share" list is not updated when navigating to another page

Approval Request Comment
[Feature/regressing bug #]: screensharing

[User impact if declined]: Wrong (old) window title shown

[Describe test coverage new/current, TBPL]: manual testing.  Green on inbound, works in manual testing.  Easily tested by hand

[Risks and why]: fairly simple patch.  Causes use confusion.

[String/UUID change made/needed]: none

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8c9024c845a9

Comment 16

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

DOn't uplift until bug 1061475 is fiixed and included, please.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd1c98e99569

Comment 18

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue on old Nightly (2014-08-20), verified that the issue is fixed on Windows 7 64bit, Ubuntu 14.04 64bit and Mac OS X 10.9.4 using latest Aurora and Firefox 33 beta 2.