videocontrols.xml binding is loaded twice in VideoDocuments (volume unresponsive until fullscreen used)

RESOLVED INACTIVE

Status

()

Toolkit
Video/Audio Controls
RESOLVED INACTIVE
3 years ago
2 hours ago

People

(Reporter: santiago.garcia, Unassigned)

Tracking

39 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.89 Safari/537.36

Steps to reproduce:

I have the following document:

<!DOCTYPE html>
<head>
<link href="any.css" rel="stylesheet">
</head>
<body>
<video controls preload="none">
</video>
</body>

The document where I initially encountered the bug was a fair bit more complex, and it also pointed to a real CSS as well as a real video file, but this is what I narrowed it down to.




Actual results:

Clicking on the volume slider does not update the volume slider. Setting the video to fullscreen resolves the problem. So does commenting out the <link> tag (obviously not a real option)


Expected results:

Volume slider should work and update correctly in all circumstances.

Updated

3 years ago
Component: Untriaged → Video/Audio
Product: Firefox → Core
Component: Audio/Video → Audio/Video: Playback
Component: Audio/Video: Playback → Video/Audio Controls
Product: Core → Toolkit
I can't reproduce this, and I would be surprised that "setting the video to fullscreen resolves the problem" since bug 962560 (present in Firefox 39) means that the volume slider in fullscreen is initially out of sync with the actual volume of the video.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 2

2 years ago
I can still reproduce the problem with Firefox 46.0.1. The volume slider doesn't update at all until I switch the video to fullscreen once. It does seem to "work" in the sense that the video becomes louder / softer, it just doesn't update.

I will attach a short screen capture which shows the problem.
(Reporter)

Comment 3

2 years ago
Created attachment 8750219 [details]
ScreenCapture_09.05.2016 10.02.51.wmv

Brief screen capture showing the problem.

Comment 4

2 years ago
Created attachment 8750279 [details]
Converted_file_e445ed56.webm

Comment 5

2 years ago
We need a testcase, please.
Flags: needinfo?(santiago.garcia)
(Reporter)

Comment 6

2 years ago
Created attachment 8750281 [details]
Test case

This is the document I am testing with.
Okay, I can reproduce it when visiting the attachment. I had originally just created a data URI of the markup that was given in comment #0 and couldn't reproduce with the data URI but I can with the attachment. 

I will look more in to this.
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(santiago.garcia)
Resolution: WORKSFORME → ---
This is actually exposing a deeper issue.

In your attached testcase, the binding for the videocontrols is loaded once. During this load the width of the controls are computed as 0px each. This is presumably because the binding is loaded and evaluated before layout has completed?

If you compare this with viewing a video directly (a synthetic VideoDocument), the binding is actually ran twice. The first time the controls are computed as 0px each. Then after or around the time of the "loadstart" event, the videocontrols binding is loaded again and the controls have their correct width calculated.

I confirmed that removing the <link> from your testcase does indeed make the bug go away.

We have two possible work-arounds:
1) Hard-code the widths of the controls instead of dynamically calculating them.
2) Calculate the widths of the controls on the next tick after the binding has finished initializing (via setTimeout(..., 0);).

Neil or Gijs, do you have any idea why this binding would be loading twice?
Status: REOPENED → NEW
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(enndeakin)
Summary: Volume slider in HTML5 video does not update → videocontrols.xml binding is loaded twice in VideoDocuments
I should have said, "do you have any idea why this binding is loaded twice, or why the first time it runs the widths are computed as 0?"
I don't know toooo much about the innards of XBL bindings. I can tell you that the initial attach seems to happen with this stack:

>	xul.dll!nsXBLBinding::ExecuteAttachedHandler() Line 613	C++
 	xul.dll!nsBindingManager::ProcessAttachedQueue(unsigned int aSkipSize=6) Line 423	C++
 	xul.dll!nsBindingManager::EndOutermostUpdate() Line 1030	C++
 	xul.dll!nsRunnableMethodImpl<enum nsresult (__thiscall mozilla::dom::FetchDriver::*)(void),1,0>::Run() Line 743	C++
 	xul.dll!nsContentUtils::RemoveScriptBlocker() Line 4984	C++
 	xul.dll!PresShell::DidCauseReflow() Line 9152	C++
 	xul.dll!PresShell::Initialize(int aWidth, int aHeight) Line 1701	C++
 	xul.dll!mozilla::dom::MediaDocument::StartLayout() Line 267	C++
 	xul.dll!mozilla::dom::MediaDocumentStreamListener::OnStartRequest(nsIRequest * request=0x0c23f830, nsISupports * ctxt=0x00000000) Line 58	C++
 	xul.dll!nsDocumentOpenInfo::OnStartRequest(nsIRequest * request=0x0c23f830, nsISupports * aCtxt=0x00000000) Line 269	C++
 	xul.dll!mozilla::net::nsHttpChannel::CallOnStartRequest() Line 1032	C++
 	xul.dll!mozilla::net::nsHttpChannel::ContinueOnStartRequest3(nsresult result=NS_OK) Line 6010	C++
 	xul.dll!mozilla::net::nsHttpChannel::ContinueOnStartRequest2(nsresult result=NS_BINDING_FAILED) Line 6001	C++
 	xul.dll!mozilla::net::nsHttpChannel::ContinueOnStartRequest1(nsresult result=NS_BINDING_FAILED) Line 5978	C++
 	xul.dll!mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest * request=0x0832eb20, nsISupports * ctxt=0x00000000) Line 5949	C++
 	xul.dll!nsInputStreamPump::OnStateStart() Line 526	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream=0x080d53d0) Line 442	C++
 	xul.dll!nsInputStreamReadyEvent::Run() Line 97	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait=false, bool * aResult=0x006cf583) Line 991	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait=false) Line 290	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate=0x00a47180) Line 98	C++
 	xul.dll!MessageLoop::RunHandler() Line 227	C++
 	xul.dll!MessageLoop::Run() Line 207	C++
 	xul.dll!nsBaseAppShell::Run() Line 158	C++
 	xul.dll!nsAppShell::Run() Line 264	C++
 	xul.dll!nsAppStartup::Run() Line 285	C++
 	xul.dll!XREMain::XRE_mainRun() Line 4368	C++
 	xul.dll!NS_TableDrivenQI(void * aThis=0x006cf931, const nsID & aIID={...}, void * * aInstancePtr=0x006cf7e8, const QITableEntry * aEntries=0x10ee3da0) Line 18	C++
 	xul.dll!nsComponentManagerImpl::QueryInterface(const nsID & aIID={...}, void * * aInstancePtr=0x006cf7e8) Line 946	C++
 	xul.dll!nsQueryInterface::operator()(const nsID & aIID, void * * aAnswer=0x006cf7e8) Line 19	C++
 	xul.dll!nsCOMPtr_base::assign_from_qi(const nsQueryInterface aQI={...}, const nsID & aIID) Line 62	C++


And the second one looks like this:

>	xul.dll!nsXBLBinding::ExecuteAttachedHandler() Line 613	C++
 	xul.dll!nsBindingManager::ProcessAttachedQueue(unsigned int aSkipSize=0) Line 423	C++
 	xul.dll!PresShell::FlushPendingNotifications(mozilla::ChangesToFlush aFlush={...}) Line 4083	C++
 	xul.dll!nsRefreshDriver::Tick(__int64 aNowEpoch, mozilla::TimeStamp aNowTime={...}) Line 1746	C++
 	xul.dll!mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver * driver=0x0808d400, __int64 jsnow=1462893317793379, mozilla::TimeStamp now={...}) Line 275	C++
 	xul.dll!mozilla::RefreshDriverTimer::TickRefreshDrivers(__int64 aJsNow=1462893317793379, mozilla::TimeStamp aNow={...}, nsTArray<RefPtr<nsRefreshDriver> > & aDrivers) Line 246	C++
 	xul.dll!mozilla::RefreshDriverTimer::Tick(__int64 jsnow, mozilla::TimeStamp now={...}) Line 265	C++
 	xul.dll!mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp aTimeStamp={...}) Line 589	C++
 	xul.dll!mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp aVsyncTimestamp={...}) Line 510	C++
 	xul.dll!nsRunnableMethodImpl<void (__thiscall mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp),1,0,mozilla::TimeStamp>::Run() Line 744	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait=true, bool * aResult=0x006cf32b) Line 991	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait=true) Line 290	C++
 	xul.dll!nsThread::Shutdown() Line 807	C++
 	xul.dll!mozilla::LazyIdleThread::ShutdownThread() Line 314	C++
 	xul.dll!mozilla::LazyIdleThread::Notify(nsITimer * aTimer=0x08669350) Line 515	C++
 	xul.dll!nsTimerImpl::Fire() Line 527	C++
 	xul.dll!nsTimerEvent::Run() Line 290	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait=false, bool * aResult=0x006cf583) Line 991	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait=false) Line 290	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate=0x00a47180) Line 98	C++
 	xul.dll!MessageLoop::RunHandler() Line 227	C++
 	xul.dll!MessageLoop::Run() Line 207	C++
 	xul.dll!nsBaseAppShell::Run() Line 158	C++
 	xul.dll!nsAppShell::Run() Line 264	C++
 	xul.dll!nsAppStartup::Run() Line 285	C++
 	xul.dll!XREMain::XRE_mainRun() Line 4368	C++
 	xul.dll!NS_TableDrivenQI(void * aThis=0x006cf931, const nsID & aIID={...}, void * * aInstancePtr=0x006cf7e8, const QITableEntry * aEntries=0x10ee3da0) Line 18	C++
 	xul.dll!nsComponentManagerImpl::QueryInterface(const nsID & aIID={...}, void * * aInstancePtr=0x006cf7e8) Line 946	C++
 	xul.dll!nsQueryInterface::operator()(const nsID & aIID, void * * aAnswer=0x006cf7e8) Line 19	C++
 	xul.dll!nsCOMPtr_base::assign_from_qi(const nsQueryInterface aQI={...}, const nsID & aIID) Line 62	C++

I would *assume* that the second stack here is because now that we're flushing layout based on vsync, we know the size of the item and so we're (re)attaching the binding. I don't know why the first thing happens before any layout has happened... hopefully Neil can help somewhat more quickly with these stacks here?
Flags: needinfo?(gijskruitbosch+bugs)
Why does adding the dummy <link> cause the RefreshDriver to not flush pending notifications? Maybe this is two bugs in one?

Comment 12

2 years ago
Created attachment 8750946 [details] [diff] [review]
Check flags before loading bindings in frame constructor

I encountered an issue like this in another bug, caused due to some codepaths that call LoadBindings not checking if a binding was already applied.

Can you check if this patch fixes the bug? I don't know if this a suitable fix as there may already be an assumption that LoadBindings should already not be called in this case.

My debugging shows that the binding for #videocontrols is only being loaded once, but the binding for the inner elements are in fact being loaded up to three times.
Flags: needinfo?(enndeakin) → needinfo?(jaws)
Flags: needinfo?(jaws)
Summary: videocontrols.xml binding is loaded twice in VideoDocuments → videocontrols.xml binding is loaded twice in VideoDocuments (volume unresponsive until fullscreen used)
Flags: needinfo?(jaws)
The patch doesn't fix the issue for me. I get the same behavior that comment #0 describes.
Flags: needinfo?(jaws)

Comment 14

2 hours ago
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Last Resolved: 2 years ago2 hours ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.