Bug 118483 - WebVTTParser's identifier buffering can ignore subsequent lines
Summary: WebVTTParser's identifier buffering can ignore subsequent lines
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-08 14:16 PDT by Brendan Long
Modified: 2013-07-24 08:43 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.76 KB, patch)
2013-07-08 14:21 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Fix changelog to have reviewer section (1.78 KB, patch)
2013-07-16 09:20 PDT, Brendan Long
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brendan Long 2013-07-08 14:16:58 PDT
In WebVTTParser::parseBytes, we do this:


    String line = collectNextLine(data, length, &position);

    switch (m_state) {
    case Initial:
        // Buffer up at least 9 bytes before proceeding with checking for the file identifier.
        m_identifierData.append(data, length);
        if (m_identifierData.size() < bomLength + fileIdentifierLength)
            return;

But it doesn't work in this situation:

    parser->parseBytes("WEBVTT\n\n", 8);
    parser->parseBytes("00:00:00.000 --> 00:00:10.000\nCue text\n\n", strlen("00:00:00.000 --> 00:00:10.000\nCue text\n\n");

The first parseBytes call has less than 9 bytes, so it does nothing. The second parseBytes call collects the current line ("00:00:00.000 --> 00:00:10.000"), but since the state is `Initial`, it just checks the identifier and throws away the current line. Now it collects the next line ("Cue text"), and because we're in state `Header`, it treats it as a comment and throws it away too.
Comment 1 Brendan Long 2013-07-08 14:21:09 PDT
Created attachment 206267 [details]
Patch
Comment 2 Brendan Long 2013-07-08 14:27:28 PDT
I guess I should explain the patch: If `position == line.sizeInBytes()`, then we haven't parsed a line ending yet, so it's safe to continue buffering. If `position > line.sizeInBytes()`, then we parsed the entire line and should check it now.
Comment 3 WebKit Commit Bot 2013-07-16 09:06:53 PDT
Comment on attachment 206267 [details]
Patch

Rejecting attachment 206267 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 206267, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/1090259
Comment 4 Brendan Long 2013-07-16 09:20:18 PDT
Created attachment 206788 [details]
Fix changelog to have reviewer section
Comment 5 Brendan Long 2013-07-23 10:12:07 PDT
Eric, can you review this again? All I changed was adding the "Reviewed by nobody (Oops)" line back so the commit queue would like it.
Comment 6 WebKit Commit Bot 2013-07-24 08:43:34 PDT
Comment on attachment 206788 [details]
Fix changelog to have reviewer section

Clearing flags on attachment: 206788

Committed r153086: <http://trac.webkit.org/changeset/153086>
Comment 7 WebKit Commit Bot 2013-07-24 08:43:36 PDT
All reviewed patches have been landed.  Closing bug.