881475 - [webvtt] Discard a cue if non-fatal error is returned

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

libwebvtt currently returns cues that are expected to be passed by the WEBVTT parsing algorithm. In these cases it also calls the error call back to notify the user, in this case Gecko, that the cue isn't malformed in some way.

We need a fix so that the WebVTTLoadListener will discard or keep cues based on the WEBVTT parsing algorithm. This basically just means discarding cues if any kind of error is returned.

Comment 1

[Caitlin Potter (:caitp)]

It is literally as simple as

```
int WEBVTT_CALLBACK
 WebVTTLoadListener::OnReportErrorWebVTTCallBack(void* aUserData, uint32_t aLine,
                                                 uint32_t aCol,
                                                 webvtt_error aError)
 {
   WebVTTLoadListener* self = static_cast<WebVTTLoadListener*>(aUserData);
   self->OnReportError(aLine, aCol, aError);
+  if( aError == WEBVTT_MALFORMED_TIMESTAMP ) {
+    return -1;
+  }
   return WEBVTT_SUCCESS;
 }

```

Comment 2

[Caitlin Potter (:caitp)]

Also, why is this not being delegating to the non-static method? The static callback is really just to make that machinery work. The return code should ideally come from the non-static method, not the static callback.

Comment 3

[Rick Eyre (:reyre)]

Yeah, that's a good idea. We can switch to doing that as well.

This hasn't been addressed currently, so we should be looking at that in this bug.

Comment 4

[Caitlin Potter (:caitp)]

Attachment: Drop cues when start/end timestamps are MALFORMED.

Hey, I haven't tested this build locally -- It's possibly that there is a typo in the error code or something, but I don't think so.

I will build over night just to double check this, but I'm 99% sure this is a workable solution.

Of course I'll test this manually with some bad data too.

Comment 5

[Caitlin Potter (:caitp)]

My tests are currently not working, I'm segfaulting during creation of the track element.

```
###!!! ASSERTION: Context should be a DOM node or a DOM window!: '!requestingContext || node || window', file /home/caitp/hg/mozilla-central/content/base/src/nsContentPolicy.cpp, line 92
Assertion failure: !mStringBuffer (We already have a stringbuffer?), at ../../../dist/include/mozilla/dom/BindingDeclarations.h:139

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff0839ebb in mozilla::dom::DOMString::AsAString (this=0x7fffffffc320)
    at ../../../dist/include/mozilla/dom/BindingDeclarations.h:139
139	    MOZ_ASSERT(!mStringBuffer, "We already have a stringbuffer?");

```

(This is using the html5-track-test-suite file). Something is wrong here, but I am not totally sure what :>

Comment 6

[Rick Eyre (:reyre)]

This is a known issue that we're just about to land behind 833382. You have to apply the patch here https://bugzilla.mozilla.org/show_bug.cgi?id=875169 for it to not crash.

Comment 7

[Caitlin Potter (:caitp)]

Alright cool, it's probably good then... re-test

Comment 8

[Caitlin Potter (:caitp)]

Tested with bug833382v2.patch patch, cue with single-digit minute is discarded. And it builds successfully, do me a favour and push to try once the strerror and other DOM fixes land.

Comment 9

[Rick Eyre (:reyre)]

https://tbpl.mozilla.org/?tree=Try&rev=8f4168e81afc

Comment 10

[Caitlin Potter (:caitp)]

unrelated test failures in osx10.6

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 760673 [details] [diff] [review]
Drop cues when start/end timestamps are MALFORMED.

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

No '#' before the bug number in the commit message. Also please add some subsequent lines about about why the change was made. Otherwise looks good, thanks.

Comment 12

[Caitlin Potter (:caitp)]

Attachment: Drop cues when start/end timestamps are MALFORMED. v2

Updated commit message as requested by :rillian.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e78d02f5e0f6

Should we have a test for this?

Comment 14

[Caitlin Potter (:caitp)]

Probably, I can add another mochitest to check it if needed

Comment 15

[Rick Eyre (:reyre)]

Makes sense to me to have one. If for some reason this slips by in the future and users start using this it might cause problems.

Comment 16

[Caitlin Potter (:caitp)]

Well if we're honest we're more likely to make users happy if we DON'T discard their cues, but if they're going to be unhappy they need to be uniformly unhappy ;)

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e78d02f5e0f6