SMTC interface doesn't be reset completely (show firefox.exe)

👁 Image	Rares Doghi, Desktop QA Reporter
	Description • 5 years ago

[Affected versions]:
Beta 81.0b5

[Affected platforms]:
Platforms: Windows 10

Steps :

1. Launch the Firefox browser and reach https://www.youtube.com/watch?v=dl6pWnbUWCY
2. Start the video and use loop in order for the short video to keep playing over and over.
3. Open a new video in a new tab and use the Media control key to Pause it.
4. Open a third video in a new tab and use the Media control key to pause it.
5. Close the two separate tabs until only the tab with the short video remains.
6. Use the Media control key in order to pause / play the short video.

Expected Results :
The Virtual control interface should not be displayed since the short video cannot be controlled from the Media key.

Actual Results :
The Virtual Control interface is displayed with greyed out buttons and the firefox.exe text displayed.

👁 Image	Rares Doghi, Desktop QA Reporter
	Comment 1 • 5 years ago

I think this is an S3 severity issue, maybe even an S4 since it only happens in specific conditions and it will not affect the users experience.

👁 Image	Rares Doghi, Desktop QA Reporter
	Comment 2 • 5 years ago

Attached some log files hoping it helps.

👁 Image	Rares Doghi, Desktop QA Reporter
	Comment 3 • 5 years ago

I also have a different scenario for this exact same issue but please let me know if you would like me to log a separate issue for it.

1. Start the Firefox browser and play a Youtube video with sound.
2. Use the Media control key from the keyboard a few times.
3. Reach Youtube in a private window.
4. Mute the video before starting it.
5. Hit the Media control button in order to start/pause the video from the normal window.
6. Close the Normal window.
7. Hit the Media control key in order to start/pause the muted video.

Expected results:
The Virtual control interface should not be displayed at all since the user cannot control the muted video.

Actual Results:
The Virtual control interface is displayed with the firefox.exe information displayed and greyed out buttons.

👁 Image	C.M.Chang[:chunmin]
	Comment 4 • 5 years ago • Edited

Thanks for the log you provided!

I can reproduce this bug now. I'll let you know if I need your help.

In log.txt.child-4.moz_log.0, I do find Not listening because media's duration 2.981000 is too short, which indicates that short video is not being controlled. It seems the virtual control interface stays on the screen even no media is being controlled. I can reproduce this bug without opening a short-duration video by the following steps

1. Play an audible video whose duration is at least 3 seconds in a tab
2. Open an empty tab
3. Go back to the tab contains video
4. Press any media key (e.g. volume+/-) that can prompt the virtual control interface
5. While the virtual control interface stays, close the tab playing video
6. Press volume+/- key

Now the virtual control interface stays with a title firefox.exe. There is no any video now. However, if volume+/- key is pressed again, the virtual control interface pop up again, with the title firefox.exe.

I can reproduce this on Windows 10, but not on Ubuntu 18.04. I suspect it's a windows-only issue. I'll investigate what happens.

👁 Image	Jon Bauman [:jbauman:]
	Updated • 5 years ago

👁 Image	Alastor Wu [:alwu] Assignee
	Comment 5 • 5 years ago

I can reproduce this issue, but it's still no clear what happened to me. If I start normal videos first, and start short duration video, then the interface would be reset completely. Or if you don't pause normal video, and close them directly, this issue also doesn't happen.

I guess we didn't reset SMTC's interface correctly under that situation, I've tried to call ClearAll() [1] when we close the SMTC, but it didn't work. I would continue to check how we can fix that.

[1] https://docs.microsoft.com/en-us/previous-versions/windows/desktop/mediatransport/isystemmediatransportcontrolsdisplayupdater-clearall

👁 Image	C.M.Chang[:chunmin]
	Comment 6 • 5 years ago

Chrome uses ISystemMediaTransportControls::put_IsEnabled [1] to close the SystemMediaTransportControls (or SMTC) panel [2], but somehow it doesn't work for us. I tried to destroy the window-handle bound to SMTC [3] when there is no more media elements in Firefox but it didn't work either. One difference is that chrome uses the existing singleton window-handle instead of creating a new one [4].

Molly, do you have idea what's going on? I wonder if we do something wrong when creating a window for SMTC [5], since the SMTC panel can still pop up even it's bound window-handle is destroyed.

Does Firefox has a singleton window-handle used in parent process? Do you happen to know how to get that window handle? I'd like to try using a similar way chrome applied to see if the SMTC panel will be disappeared or not. Or it's ok to create a new window-handle in this case?

[1] https://docs.microsoft.com/en-us/previous-versions/windows/desktop/mediatransport/isystemmediatransportcontrols-put-isenabled?redirectedfrom=MSDN
[2] https://source.chromium.org/chromium/chromium/src/+/master:components/system_media_controls/win/system_media_controls_win.cc;l=283-286;drc=aeb70b905ac38796b9801ec6a0230e71005c8b6b;bpv=1;bpt=0
[3] https://searchfox.org/mozilla-central/rev/62c443a7c801ba9672de34c2867ec1665a4bbe67/widget/windows/WindowsSMTCProvider.cpp#116
[4] https://source.chromium.org/chromium/chromium/src/+/master:components/system_media_controls/win/system_media_controls_win.cc;l=79;drc=fd31128cb7b8cee546f71e7051c7cd81ff7acd9b
[5] https://searchfox.org/mozilla-central/rev/62c443a7c801ba9672de34c2867ec1665a4bbe67/widget/windows/WindowsSMTCProvider.cpp#102-133

👁 Image	C.M.Chang[:chunmin]
	Comment 7 • 5 years ago • Edited

Another interesting thing I found is that if the enabled states of the media-key buttons are never changed once they are set, then we are free from this problem (e.g., enable all the buttons when opening SMTC, do nothing in SetSupportedMediaKeys, disable all the buttons when closing SMTC). That's weird.

👁 Image	Marc Streckfuß [:MeFisto94]
	Comment 8 • 5 years ago

Note that I saw this kind of panel during development as I only had registered the interface and called put_IsEnabled(true).
So it's like neither UpdateButtons, SetPlaybackState etc are ever called (for that instance or maybe after this has been reset by another media being done).

Could this be a "race condition", as in Open -> Close -> Set Metadata (where the Close is called by a previous media ending).
At least I think it's not about the panel creation in itself, but rather that there is a panel, whose state/metadata is not set correctly, right?
This would on one hand match to the strange conditions under which this happens, but I'm just guessing there.

It never happens for a single video at a time, right?

👁 Image	C.M.Chang[:chunmin]
	Comment 9 • 5 years ago • Edited

(In reply to Marc Streckfuß [:MeFisto94] from comment #8)

It never happens for a single video at a time, right?

This could be reproduced by a single video. See #comment 4. But Set Metadata could be called several times indeed. We will set some empty data when there is no more media.

Open , Close, SetPlaybackState, SetMediaMetadata, SetSupportedMediaKeys are all on the main thread, so data racing won't happen in the Firefox end.

👁 Image	C.M.Chang[:chunmin]
	Comment 10 • 5 years ago • Edited

I've worked out a workaround here. It seems we can avoid this problem if put_IsEnabled(false) is called at the same time when all buttons on the SMTC panel is disabled via put_Is*Enabled. It seems to me this is a Windows bug.

Here is a straightforward workaround patch and here is an alternative that looks slightly better.

It works on my Windows 10 machine but it's better to confirm that works on other machines.

👁 Image	Rares Doghi, Desktop QA Reporter
	Comment 11 • 5 years ago

Hi, we are testing this feature in Beta now and after we control any video with sound using the media key, load a different video in a new tab but muted and close the other tab, every time we hit the Pause/Play media key on the keyboard the Virtual Control interface is displayed with the firefox.exe text. I dont think this is a new issue I think its related to this one but please let me know if we should log a separate issue for it.

1. Launch the Firefox browser and start a video on youtube with sound.
2. Use the Pause / Play media key in order to control the video.
3. Load a separate video in a new tab and mute it before starting the video.
4. Close the first tab.
5. Hit the media keys on the keyboard in order to control the muted video.

Expected Results:
The Virtual control interface should not be displayed at all since the video cannot be controlled from the media keys.

@Alastor , please let me know if we should log a separate issue for this.

👁 Image	Rares Doghi, Desktop QA Reporter
	Comment 12 • 5 years ago

The Virtual Control interface is also displayed on Gifs if the user controlled any video before reaching the page :
https://giphy.com/gifs/series-canal-killing-eve-canalplus-sries-H3GjxERjsR4YXIihsM

I can log a separate issue for this as well but it does seem its the same issue.

👁 Image	Alastor Wu [:alwu] Assignee
	Comment 13 • 5 years ago

Yes, they look like same issue for me. I guess the issue is we didn't reset the interface, so it keeps displaying. I will start debugging this bug today, to see if we have other way to sove it completely. Thank you.

👁 Image	Alastor Wu [:alwu] Assignee
	Updated • 5 years ago

👁 Image	Alastor Wu [:alwu] Assignee
	Comment 14 • 5 years ago

👁 Image	Alastor Wu [:alwu] Assignee
	Comment 15 • 5 years ago

👁 Image	Alastor Wu [:alwu] Assignee
	Comment 16 • 5 years ago

👁 Image	Alastor Wu [:alwu] Assignee
	Comment 17 • 5 years ago

👁 Image	Pulsebot
	Comment 18 • 5 years ago

Pushed by alwu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1f6e8e343055 part1 : remove unused function 'SetControlledTabBrowsingContextId()'. r=chunmin https://hg.mozilla.org/integration/autoland/rev/6791eacd4868 part2 : let MediaControlService determine when we should open/close the event source. r=chunmin https://hg.mozilla.org/integration/autoland/rev/0ea9bcad0ff8 part3 : the event source should do the cleanup when it closes. r=chunmin https://hg.mozilla.org/integration/autoland/rev/c9bd215fdafa part4 : enable SMTC after finishing other initializations. r=chunmin

👁 Image	Razvan Maries
	Comment 19 • 5 years ago

Backed out for perma failures.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=SFADUJpeRDW1C8Aunj_O8A.0&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=windows%2C7%2Copt%2Cmochitests%2Ctest-windows7-32%2Fopt-mochitest-browser-chrome-e10s%2Cbc4&revision=c9bd215fdafad0d46b91b1de0f3f031f1e7579d4

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=317861579&repo=autoland&lineNumber=5515

Backout: https://hg.mozilla.org/integration/autoland/rev/961cfd1bea4d6ec21a671263370590bbcaa79810

👁 Image	Alastor Wu [:alwu] Assignee
	Updated • 5 years ago

👁 Image	Pulsebot
	Comment 20 • 5 years ago

Pushed by alwu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/92a26a171220 part1 : remove unused function 'SetControlledTabBrowsingContextId()'. r=chunmin https://hg.mozilla.org/integration/autoland/rev/00ff29a35291 part2 : let MediaControlService determine when we should open/close the event source. r=chunmin https://hg.mozilla.org/integration/autoland/rev/571dc734bba1 part3 : the event source should do the cleanup when it closes. r=chunmin https://hg.mozilla.org/integration/autoland/rev/4bda24c82a0b part4 : enable SMTC after finishing other initializations. r=chunmin

👁 Image	Narcis Beleuzu [:NarcisB]
	Comment 21 • 5 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/92a26a171220
https://hg.mozilla.org/mozilla-central/rev/00ff29a35291
https://hg.mozilla.org/mozilla-central/rev/571dc734bba1
https://hg.mozilla.org/mozilla-central/rev/4bda24c82a0b

👁 Image	Alastor Wu [:alwu] Assignee
	Comment 22 • 5 years ago • Edited

Comment on attachment 9179076 [details]
Bug 1663128 - part1 : remove unused function 'SetControlledTabBrowsingContextId()'.

Beta/Release Uplift Approval Request

• User impact if declined: On Windows, when users play media, it would show a virtual control interface to allow them to control media via its virtual button. That should be removed when user closes the tab which was playing audio.

But currently that wasn't removed correctly and would display firefox.exe, which can only be removed after users restart Firefox.

• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Follow comment0 or comment4.
• List of other uplifts needed: All patches
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It doesn't introduce behavior change, but only perform a correct steps to remove the virtual interface.
• String changes made/needed:

👁 Image	Alastor Wu [:alwu] Assignee
	Updated • 5 years ago

👁 Image	Julien Cristau [:jcristau]
	Comment 23 • 5 years ago

We're about to build the last beta for 82, and this affects 81 already, so I'd prefer to let it ride the trains.

👁 Image	Julien Cristau [:jcristau]
	Updated • 5 years ago

👁 Image	Julien Cristau [:jcristau]
	Updated • 5 years ago

👁 Image	Julien Cristau [:jcristau]
	Updated • 5 years ago

👁 Image	Julien Cristau [:jcristau]
	Updated • 5 years ago

👁 Image	Brindusa Tot, DTE
	Updated • 5 years ago

👁 Image	Rares Doghi, Desktop QA Reporter
	Comment 24 • 5 years ago

This issue is Verified as Fixed in our latest Nightly build 83.0a1 (2020-10-08) on Windows 10 and Ubuntu.