Bug 70274 – WebVTT parser should not allow timestamps that have no spaces

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Anna Cavender]

Timestamps like this should not be allowed:

00:04:01.000-->00:03:00.500

They should instead be required to have at least one whitespace character between the startTime/endTime and the arrow:

00:04:01.000 --> 00:03:00.500

[Anna Cavender]

Created attachment 111332 [details]
Patch

[WebKit Review Bot]

Attachment 111332 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
From git://git.webkit.org/WebKit
   c0d4f37..d10d208  master     -> origin/master
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/html/HTMLKeygenElement.h
	M	Source/WebCore/html/HTMLKeygenElement.cpp
r97658 = c627b94a38756dd0cde4d64c2288f36bff87ce20 (refs/remotes/trunk)
	A	LayoutTests/fast/mutation/observe-attributes-expected.txt
	A	LayoutTests/fast/mutation/observe-attributes.html
	M	LayoutTests/ChangeLog
	M	Source/WebCore/dom/MutationRecord.idl
	M	Source/WebCore/dom/MutationRecord.cpp
	M	Source/WebCore/dom/Element.cpp
	M	Source/WebCore/dom/Node.h
	M	Source/WebCore/dom/WebKitMutationObserver.h
	M	Source/WebCore/dom/NodeRareData.h
	M	Source/WebCore/dom/MutationRecord.h
	M	Source/WebCore/dom/Node.cpp
	M	Source/WebCore/dom/WebKitMutationObserver.cpp
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/bindings/v8/V8Proxy.cpp
	M	Source/WebCore/WebCore.xcodeproj/project.pbxproj
r97659 = d10d208ef8dc738eb8d45c331840c61c2c779498 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Silvia Pfeiffer]

Hmm, the spec at http://www.whatwg.org/specs/web-apps/current-work/webvtt.html currently states only the U+0020 SPACE character as a valid separator between the "-->" string and the timestamps. This accepts other white space as separators, too, in particular isASpace() accepts also U+0009 CHARACTER TABULATION (tab), U+000A LINE FEED (LF), U+000C FORM FEED (FF), and U+000D CARRIAGE RETURN.

I've just registered a bug on the spec to allow the TAB character as a separator, see http://www.w3.org/Bugs/Public/show_bug.cgi?id=14487. But I do not believe the newline characters should be allowed.

[Anna Cavender]

I agree line terminators should not be allowed, but collectTimingsAndSettings() is only ever handed a single line (so it will not have any line terminators).  Perhaps the use of isASpace() is confusing here, would it be more clear to just check for ' ' and/or '\t' instead of using isASpace()?

[Anna Cavender]

Created attachment 111538 [details]
Patch

How about we make sure that at least one space separates the time from the --> for now?  We can always update if more whitespace characters are allowed in the future.

[Silvia Pfeiffer]

Works for me. :-)

[Anna Cavender]

Created attachment 111543 [details]
adding check for tabs

Ha, one of our tests (tc008-timings-no-hour-errors.vtt) checks for tabs, so at least WE think tabs should be allowed.  Adding that here.

[WebKit Review Bot]

Comment on attachment 111543 [details]
adding check for tabs

Clearing flags on attachment: 111543

Committed r97883: <http://trac.webkit.org/changeset/97883>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Silvia Pfeiffer]

As of a few hours ago, the spec now also tolerates tabs, see http://www.w3.org/Bugs/Public/show_bug.cgi?id=14487 . We should add this.

[Anna Cavender]

Great!  We too allow both spaces and tabs, so looks like we are good to go.

A small note: the parser in the spec currently allows *0 or more* space or tab characters, not *1 or more* space or tab characters as specified in Ian's change.  Our implementation now enforces at least 1 space or tab character (so that [time]-->[time] without any spaces or tabs is not allowed).