Closed Bug 1580015 Opened 3 months ago Closed 3 months ago

[webvtt] Video with min-width AND captions locks up browser tab

Categories

(Core :: Audio/Video: Playback, defect, P2)

69 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox-esr68 --- unaffected
firefox69 --- wontfix
firefox70 --- verified
firefox71 --- verified

People

(Reporter: dstanich, Assigned: alwu)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:69.0) Gecko/20100101 Firefox/69.0

Steps to reproduce:

This is a problem with the <video> tag locking up the browser tab and spiking the CPU.

Steps in the HTML:

  1. Created a parent container with a defined height/width
  2. Apply a min-width CSS property to the video tag
  3. Provide a caption track for the video file for closed captioning
  4. Attempt to view the video with closed captioning enabled

You can recreate this by:

  1. Clone this repo: https://github.com/dstanich/ff-video-caption-bug
  2. npm install
  3. npm start
  4. Go to http://localhost:8080 and attempt to watch the video

Actual results:

As of Firefox 69, if BOTH min-width and closed captioning is enabled on the video element, then the browser tab will lock up and requires it to sometimes be force quit.

I tested in the latest FF ESR 68.1.0 to compare and this works fine here.

Expected results:

Video should have been properly positioned with the min-width and the video should have played showing the captioning as it played.

Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core

I see from the profile [1] that most of the time is spent in the WeVTT.processCues which is called repeatedly. Alastor can you please have a look and redirect accordingly.

[1] https://perfht.ml/2PYXvkP

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(alwu)
Priority: -- → P2

Will take a look.

Assignee: nobody → alwu
Flags: needinfo?(alwu)
Blocks: webvtt
Summary: Video with min-width AND captions locks up browser tab → [webvtt] Video with min-width AND captions locks up browser tab

Accessing offsetXXX attributes of element, it would trigger reflow, and sometime it would result in a re-entry of this function because we would call processCue() when resizecaption occurs.

One specifical case is that, assigning CSS properties min-width and max-width at the same video element, in thise case, when we re-enter the 'processCues()' and access offsetXXX. It seems that the layout system would reprocess those properites again and again and dispatch resizecaption when we access offsetXXX, which causes an infinite loop, starting from receving resizecaption, then calling processCue(), accessingoffSetXXX, triggering reflow, sendingresizecaption` event again.

Therefore, we should stop revisiting processCues() if we're already running a processing, we should run a processing once at a time.

Regressed by: 1541452
Attachment #9091861 - Attachment description: Bug 1580015 - do not reentry function 'processCues()'. → Bug 1580015 - part1 : do not reentry function 'processCues()'.

Add NI as a reminder to uplift the change to beta.

Flags: needinfo?(alwu)
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf45c1ec28a2
part1 : do not reentry function 'processCues()'. r=heycam
https://hg.mozilla.org/integration/autoland/rev/6bd2baa12b99
part2 : add test 'test_webvtt_infinite_processing_loop.html'. r=heycam
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Comment on attachment 9091861 [details]
Bug 1580015 - part1 : do not reentry function 'processCues()'.

Beta/Release Uplift Approval Request

  • User impact if declined: Page will freeze forever and reach a CPU spike.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See comment0
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): In this patch, we prevent to run the same task again while there is another task running, we didn't change or add any feature or behavior changes.
  • String changes made/needed: no
Flags: needinfo?(alwu)
Attachment #9091861 - Flags: approval-mozilla-beta?
Attachment #9092156 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Comment on attachment 9091861 [details]
Bug 1580015 - part1 : do not reentry function 'processCues()'.

Fix for regression from 69, avoids CPU freeze when video captions are on.
Let's take this for beta 7.

Attachment #9091861 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9092156 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: in-testsuite+

This issue is verified as fixed on our latest Nightly build as well as beta 70.0b7 on Windows 10, Mac 10.14 and Ubuntu 18.04

QA Whiteboard: [qa-triaged]
Flags: qe-verify+

Since this is a Wont Fix for 69 I will go ahead and change the resolution for it to Verified.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.