Closed Bug 881475 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: reyre, Assigned: caitlin.potter)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → rick.eyre
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;
 }

```
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.
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.
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.
Assignee: rick.eyre → caitlin.potter
Status: NEW → ASSIGNED
Attachment #760673 - Flags: review?(giles)
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 :>
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.
Alright cool, it's probably good then... re-test
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.
unrelated test failures in osx10.6
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.
Attachment #760673 - Flags: review?(giles) → review+
Updated commit message as requested by :rillian.
Attachment #760673 - Attachment is obsolete: true
Attachment #761059 - Flags: review?(giles)
Attachment #761059 - Flags: checkin?(giles)
Attachment #761059 - Flags: review?(giles) → review+
Keywords: checkin-needed
Blocks: 881978
Attachment #761059 - Flags: checkin?(giles) → checkin+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e78d02f5e0f6

Should we have a test for this?
Flags: in-testsuite?
Keywords: checkin-needed
Probably, I can add another mochitest to check it if needed
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.
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 ;)
https://hg.mozilla.org/mozilla-central/rev/e78d02f5e0f6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.