Open Bug 1187296 Opened 9 years ago Updated 2 years ago

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

Categories

(Toolkit :: Video/Audio Controls, defect)

Product:
Toolkit
Component:
Video/Audio Controls
Version:
39 Branch
Type:
defect

Tracking

()

People

(Reporter: santiago.garcia, Unassigned)

Details

Attachments

(4 files)

ScreenCapture_09.05.2016 10.02.51.wmv
(deleted), video/x-ms-wmv
Details
Converted_file_e445ed56.webm
(deleted), video/webm
Details
Test case
(deleted), text/html
Details
Check flags before loading bindings in frame constructor
(deleted), patch
Details | Diff | Splinter Review
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.
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
Closed: 9 years ago
Resolution: --- → WORKSFORME
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.
Attached video ScreenCapture_09.05.2016 10.02.51.wmv (deleted) —
Brief screen capture showing the problem.
Attached video Converted_file_e445ed56.webm (deleted) —
We need a testcase, please.
Flags: needinfo?(santiago.garcia)
Attached file Test case (deleted) —
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?
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)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: