1010707 - [WebVTT] hang with VTTCue and canvas

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (hangs Firefox)

Comment 1

[Jesse Ruderman]

Attachment: sample

Comment 2

[Jesse Ruderman]

Attachment: a stack (C++ and JS)

I got this by disabling the JITs and pressing ^C during the hang.

Comment 3

[Jesse Ruderman]

Attachment: a stack (C++ and JS)

Comment 4

[Alastor Wu [:alwu]]

This issue is due to we're in the infinite while-loop [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/webvtt/vtt.jsm#1006

Comment 5

[Alastor Wu [:alwu]]

Still debugging, don't find the root cause..

Now I observed that the CueStyleBox doesn't be updated correctly after [1], so we can't move a StyleBox to the specific position.

I also found that the test won't be crashed if we do one of following modifications,
(1) remove |canvas.getContext("2d").measureText("Z")| 
(2) add |video.controls = true| before |video.pause()|
(3) pause video on |video.onplay = function() { ... }|

The possible reason might be related with canvas reflowing process, but I don't know what is the exactly root cause now...

Comment 6

[Alastor Wu [:alwu]]

Hi, Ralph,
Sorry to bother you,
Could you give me any suggestion about this issue?

Now, the problem I found is that we're in the infinite while-loop [1] when we want to move element to specific position. In the BoxPosition::move, if the |toMove| is not defined, then we would use its line height [2] as moving offset. In this case, because the line height [3] is ZERO, we can never move the box to the right place, so we're stuck in that while-loop.

I have some possible ways to solve that problem, but I don't sure which one is the better method.
More important is that I don't know whether these modification would cause other side effect, because I don't familiar with this field. 

So I need your help to see which following option is a better choice.
(1) we should avoid ZERO line height in BoxPosition ctor
(2) in BoxPosition::move, when |toMove| is not defined and line height is ZERO, we gave it another default value
(3) maybe the computing condition [4] of |lh| is incorrect and we need to modify it?
(4) the line height shouldn't be ZERO, it might be a bug in the reflow process and we need to figure it out

Very appreciate your help!

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/webvtt/vtt.jsm#1004
[2] https://dxr.mozilla.org/mozilla-central/source/dom/media/webvtt/vtt.jsm#880
[3] https://dxr.mozilla.org/mozilla-central/source/dom/media/webvtt/vtt.jsm#869
[4] https://dxr.mozilla.org/mozilla-central/source/dom/media/webvtt/vtt.jsm#859

Comment 7

[Ralph Giles (:rillian)]

CC ruxton who's also looked at this code recently.

Comment 8

[Ralph Giles (:rillian)]

Sorry, Alastor, I don't have any great insight. Let's try to compare with the spec and see if there's an more correct behaviour here. In any case we should make sure it can't hang!

Comment 9

[Alastor Wu [:alwu]]

After study the spec, the solution can be found in the part "Processing model". [1]
In addition, I observed that the css setting flow is not consistent with spec, so I would work for the solution to follow the spec and fix the hanging problem. 

[1] https://w3c.github.io/webvtt/#processing-model

Comment 10

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1010707 - don't adjust position when line-height is zero.

Review commit: https://reviewboard.mozilla.org/r/54576/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54576/

Comment 11

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/54576/#review51448

There are 15 steps in https://w3c.github.io/webvtt/#processing-model. Could you add comments(step1 to  step 15) in vtt.jsm to show me which step is wrong?

Comment 12

[Alastor Wu [:alwu]]

The error is in the step 11-5 of "apply WebVTT cue settings" [1] which is in the step 14-2-1 of "rules for updating the display of WebVTT text tracks". [2]

I think it's not very useful if just add a comment for this step, I would prefer to re-factor the whole flow to make all steps clear because there are still some steps we don't have the implementation, eg. for "apply WebVTT cue settings", the step 2~8 (setting the CSS attributes).

[1] https://w3c.github.io/webvtt/#apply-webvtt-cue-settings
[2] https://w3c.github.io/webvtt/#processing-model

Comment 13

[Alastor Wu [:alwu]]

Hi, Ralph,

Could you help me review this patch?
The changing is based on spec,
> "if step is zero, then jump to the step labeled done positioning below" 

You can see more details in the comment12.
Thanks!

Comment 14

[Ralph Giles (:rillian)]

Comment on attachment 8755373 [details]
MozReview Request: Bug 1010707 - don't adjust position when line-height is zero.

https://reviewboard.mozilla.org/r/54576/#review52976

Works for me! Thanks.

Comment 15

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f65c40a32a92

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/109250561b77

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/109250561b77