Bug 72279 – media/track/track-webvtt-tc004-magic-header.html flakily times out

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tony Gentilcore]

media/track/track-webvtt-tc004-magic-header.html has been flakily timing out since r100085.

Flakiness dashboard:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=media%2Ftrack%2Ftrack-webvtt-tc004-magic-header.html

I'm updating the expectations now.

[Tony Gentilcore]

Created attachment 114957 [details]
Patch

[Tony Gentilcore]

Committed r100149: <http://trac.webkit.org/changeset/100149>

[Vsevolod Vlasov]

This test is failing all the time on win and not so often on linux.

Added TEXT to expectations.
http://trac.webkit.org/changeset/101776

[Zan Dobersek]

The problem occurs when parsing the third, erroneous track is being parsed. Currently a parser is automatically created if the response in TextTrackLoader::processNewCueData has text/vtt as mime type. This also happens with the mentioned track, causing the start of parsing. The parsing does not get over the Initial state in WebVTTParser::parseBytes, as the identifier could not be found, but the parser doesn't register this as an error and doesn't notify the client.

Patch incoming.

[Zan Dobersek]

Created attachment 134522 [details]
Patch

[Zan Dobersek]

Comment on attachment 134522 [details]
Patch

Also setting the cq flag.

[Eric Carlson]

Comment on attachment 134522 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134522&action=review

This is close, but I would prefer to have the cleanup noted. Thanks for identifying the issue and taking it on!

> Source/WebCore/html/track/WebVTTParser.cpp:141
>          case Initial:
>              // 4-12 - Collect the first line and check for "WEBVTT".
> -            if (!hasRequiredFileIdentifier(data, length))
> +            if (!hasRequiredFileIdentifier(data, length)) {
> +                if (m_client)
> +                    m_client->fileUnsuccessfullyParsed();
>                  return;
> +            }

"fileUnsuccessfullyParsed" is awkward, how about something like "fileFailedToParse", or "invalidFile" instead?

As long as you are fixing this, I think it would be a good idea to move *all* of the logic about the file identifier into the parser. IOW, allow the client to call parseBytes() immediately instead of waiting until it knows that there is enough data loaded to check the file identifier. This will require the parser stay in the "Initial" state until can check for the identifier, but that will only require us to buffer up to 6 or 9 bytes before checking for the identifier.

> Source/WebCore/loader/TextTrackLoader.cpp:195
> +

Please include the error LOG(...) here.

[Zan Dobersek]

Comment on attachment 134522 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134522&action=review

>> Source/WebCore/html/track/WebVTTParser.cpp:141
>> +            }
> 
> "fileUnsuccessfullyParsed" is awkward, how about something like "fileFailedToParse", or "invalidFile" instead?
> 
> As long as you are fixing this, I think it would be a good idea to move *all* of the logic about the file identifier into the parser. IOW, allow the client to call parseBytes() immediately instead of waiting until it knows that there is enough data loaded to check the file identifier. This will require the parser stay in the "Initial" state until can check for the identifier, but that will only require us to buffer up to 6 or 9 bytes before checking for the identifier.

Should I then make WebVTTParser operate directly on the SharedBuffer that's provided by CachedResource in TextTrackLoader::processNewCueData? Progressing this idea, would it be safe to pass over the pointer to SharedBuffer already when constructing the parser? Current parsing offset would the move to be stored in the parser, making TextTrackLoader pretty much free of parser-related bloat.

Noted and agreed on the name.

>> Source/WebCore/loader/TextTrackLoader.cpp:195
>> +
> 
> Please include the error LOG(...) here.

Through a quick look I only found LOG macros used in track-related files on two occasions. I agree logging should be done, not only here though but rather throughout track-related operations. Because of that I recommend populating the code with logging info for another patch. I can commit to creating a bug, CC-ing people and creating the patch in near-future.

[Eric Carlson]

(In reply to comment #8)
> (From update of attachment 134522 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134522&action=review
> 
> >> Source/WebCore/html/track/WebVTTParser.cpp:141
> >> +            }
> > 
> > "fileUnsuccessfullyParsed" is awkward, how about something like "fileFailedToParse", or "invalidFile" instead?
> > 
> > As long as you are fixing this, I think it would be a good idea to move *all* of the logic about the file identifier into the parser. IOW, allow the client to call parseBytes() immediately instead of waiting until it knows that there is enough data loaded to check the file identifier. This will require the parser stay in the "Initial" state until can check for the identifier, but that will only require us to buffer up to 6 or 9 bytes before checking for the identifier.
> 
> Should I then make WebVTTParser operate directly on the SharedBuffer that's provided by CachedResource in TextTrackLoader::processNewCueData? Progressing this idea, would it be safe to pass over the pointer to SharedBuffer already when constructing the parser? Current parsing offset would the move to be stored in the parser, making TextTrackLoader pretty much free of parser-related bloat.
> 

It already does operate on it in as much as it is passed a pointer to the data contained in the shared buffer. I do not think it make sense to change the API to always take a SharedBuffer instead of a const char *. This is not how any of the other WebKit parsers work, and it make the lifetime management of the buffer much more complicated.

> 
> >> Source/WebCore/loader/TextTrackLoader.cpp:195
> >> +
> > 
> > Please include the error LOG(...) here.
> 
> Through a quick look I only found LOG macros used in track-related files on two occasions. I agree logging should be done, not only here though but rather throughout track-related operations. Because of that I recommend populating the code with logging info for another patch. I can commit to creating a bug, CC-ing people and creating the patch in near-future.

I definitely agree that we should have more consistent logging in other track related functions, but I don't think this is a reason to remove the error logging here.

[Zan Dobersek]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 134522 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=134522&action=review
> > 
> > >> Source/WebCore/html/track/WebVTTParser.cpp:141
> > >> +            }
> > > 
> > > "fileUnsuccessfullyParsed" is awkward, how about something like "fileFailedToParse", or "invalidFile" instead?
> > > 
> > > As long as you are fixing this, I think it would be a good idea to move *all* of the logic about the file identifier into the parser. IOW, allow the client to call parseBytes() immediately instead of waiting until it knows that there is enough data loaded to check the file identifier. This will require the parser stay in the "Initial" state until can check for the identifier, but that will only require us to buffer up to 6 or 9 bytes before checking for the identifier.
> > 
> > Should I then make WebVTTParser operate directly on the SharedBuffer that's provided by CachedResource in TextTrackLoader::processNewCueData? Progressing this idea, would it be safe to pass over the pointer to SharedBuffer already when constructing the parser? Current parsing offset would the move to be stored in the parser, making TextTrackLoader pretty much free of parser-related bloat.
> > 
> 
> It already does operate on it in as much as it is passed a pointer to the data contained in the shared buffer. I do not think it make sense to change the API to always take a SharedBuffer instead of a const char *. This is not how any of the other WebKit parsers work, and it make the lifetime management of the buffer much more complicated.
> 

Ok, noted.

> > 
> > >> Source/WebCore/loader/TextTrackLoader.cpp:195
> > >> +
> > > 
> > > Please include the error LOG(...) here.
> > 
> > Through a quick look I only found LOG macros used in track-related files on two occasions. I agree logging should be done, not only here though but rather throughout track-related operations. Because of that I recommend populating the code with logging info for another patch. I can commit to creating a bug, CC-ing people and creating the patch in near-future.
> 
> I definitely agree that we should have more consistent logging in other track related functions, but I don't think this is a reason to remove the error logging here.

Right then, might as well start logging here.

[Zan Dobersek]

Created attachment 134747 [details]
Patch

Also fixes a typo throughout WebVTTParser.

[Eric Carlson]

Comment on attachment 134747 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134747&action=review

> Source/WebCore/html/track/WebVTTParser.cpp:133
> +            if (length < bomLength + fileIdentifierLength)
> +                return;

The parser only sees each byte once, so won't we now report failure if the first call to parseBytes() passes less than 9 bytes? I think you need to accumulate the first 9 bytes into a member variable and test that.

[Zan Dobersek]

Created attachment 135819 [details]
Patch

Store identifier data until there's enough of it.

[Eric Carlson]

Comment on attachment 135819 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135819&action=review

Nice fix, thanks! Marking r+ but cq- because of the extraneous fprintf().

> Source/WebCore/html/track/WebVTTParser.cpp:152
> +    if (m_identifierData.size() >= bomLength && m_identifierData[0] == '\xEF'
> +        && m_identifierData[1] == '\xBB' && m_identifierData[2] == '\xBF')
> +        position += bomLength;

Nit: I don't think breaking the "if" line helps readability as it makes it harder to see which line is executed when the condition is true.

> Source/WebCore/html/track/WebVTTParser.cpp:154
> +    fprintf(stderr, "WebVTTParser::hasRequiredFileIdentifier\n%s\n", line.utf8().data());

Oops, looks like you forgot to remove some debugging code.

[Zan Dobersek]

Created attachment 135845 [details]
Updated reviewed patch

Thanks for the reviews! Still asking for a cq+ though, thanks in advance.

[Radar WebKit Bug Importer]

<rdar://problem/11192490>

[WebKit Review Bot]

Comment on attachment 135845 [details]
Updated reviewed patch

Clearing flags on attachment: 135845

Committed r113343: <http://trac.webkit.org/changeset/113343>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.