974017 - [webvtt] Limit the line cue setting to the range of -1000~1000

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

I've reopened the spec bug (https://www.w3.org/Bugs/Public/show_bug.cgi?id=22708) for this since having a very large line setting impacts the step algorithm making it very, very slow. I'm taking steps to do this in vtt.js right now and not waiting for the spec to catch up, since WebVTT is set to have its pref flipped in Firefox.

Maybe it should be even lower.

Comment 1

[Daniel Veditz [:dveditz]]

Other than making the implementation slow is there anything beyond a denial of service attack here? Doesn't seem like it needs to be a hidden bug if that's all it is.

Comment 2

[Rick Eyre (:reyre)]

Yeah, it will just make it slow, possibly very slow, depending on if the line setting is large enough.

Comment 3

[Ralph Giles (:rillian)]

Very slow at the request of the page author barring XSS attacks. I'm fine with opening this bug.

Comment 4

[Rick Eyre (:reyre)]

The specification community group has asked me to see if I can optimize this algorithm instead of limiting the value. So I'll try to do that first. I think there shouldn't be a problem with it.

Comment 5

[Rick Eyre (:reyre)]

Attachment: Update vtt.js r=rillian

This implements an optimization for large line settings instead of clamping the line value at some arbitrary integer.

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8404892 [details] [diff] [review]
Update vtt.js r=rillian

Review of attachment 8404892 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like an improvement to me! Do we have any tests for this?

::: content/media/webvtt/vtt.jsm
@@ +129,5 @@
>      // Accept a setting if its a valid (signed) integer.
>      integer: function(k, v) {
>        if (/^-?\d+$/.test(v)) { // integer
>          // Only take values in the range of -1000 ~ 1000
> +        this.set(k, parseInt(v, 10));

Comment needs updating too, no? The limit no longer happens here.

@@ +856,5 @@
> +      var step = this.lineHeight,
> +          maxSteps = Math.floor((options.container[axes.ref] / step) + 1);
> +      if (maxSteps < (toMove / step)) {
> +        toMove = maxSteps * step;
> +      }

toMove and container are both in px rather than steps, so it might be cleaner to use maxMove instead of maxSteps here instead of converting to steps and then back.

Comment 7

[Rick Eyre (:reyre)]

Thanks for the quick review.

(In reply to Ralph Giles (:rillian) from comment #6)

> Looks like an improvement to me! Do we have any tests for this?

We have a few tests in vtt.js that test that the result is still the same as the regular algorithm, but we don't have any tests to make sure the optimization is actually happening. I've verified that manually for now. I'm not sure how we would be able to accomplish that since this code isn't really exposed to our test suite. We might be able to run some kind of time based test and make sure it's not over a certain amount... possibly. I'll open a bug for that on the GitHub repo.

> ::: content/media/webvtt/vtt.jsm
> @@ +129,5 @@
> >      // Accept a setting if its a valid (signed) integer.
> >      integer: function(k, v) {
> >        if (/^-?\d+$/.test(v)) { // integer
> >          // Only take values in the range of -1000 ~ 1000
> > +        this.set(k, parseInt(v, 10));
> 
> Comment needs updating too, no? The limit no longer happens here.

Good catch, I'll update.

> @@ +856,5 @@
> > +      var step = this.lineHeight,
> > +          maxSteps = Math.floor((options.container[axes.ref] / step) + 1);
> > +      if (maxSteps < (toMove / step)) {
> > +        toMove = maxSteps * step;
> > +      }
> 
> toMove and container are both in px rather than steps, so it might be
> cleaner to use maxMove instead of maxSteps here instead of converting to
> steps and then back.

Makes sense. I'll update.

Comment 8

[Rick Eyre (:reyre)]

Attachment: v2: Update vtt.js r=rillian.

Thanks for review Ralph.

Carrying forward r=rillian.

Comment 9

[Rick Eyre (:reyre)]

I've filed the bug to test line setting optimization as https://github.com/mozilla/vtt.js/issues/308.

Comment 10

[Rick Eyre (:reyre)]

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=c4d7acd983e0

Comment 11

[Carsten Book [:Tomcat]]

landed on m-i as https://hg.mozilla.org/integration/mozilla-inbound/rev/3abaf19dcbff

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3abaf19dcbff