Bug 260409

Summary:	Empty files should not succeed in loading (as WebVTT)
Product:	WebKit	Reporter:	Ahmad Saleem <ahmad.saleem792>
Component:	Media	Assignee:	Nobody <webkit-unassigned>
Status:	RESOLVED FIXED
Severity:	Normal	CC:	annevk, destra, eric.carlson, jer.noble, karlcow, kohei.ueno119, webkit-bug-importer
Priority:	P2	Keywords:	BrowserCompat, InRadar, WPTImpact
Version:	WebKit Nightly Build
Hardware:	Unspecified
OS:	Unspecified
URL:	http://wpt.live/webvtt/parsing/file-parsing/signature-invalid.html
Bug Depends on:	179370
Bug Blocks:

Description Ahmad Saleem 2023-08-18 14:43:09 PDT

Hi Team,

While going through Blink's commit, I came across another potential merge:

Blink Commit: https://chromium.googlesource.com/chromium/src.git/+/0983d4b64cb5f519684f773daa27b2ac9b8f8688

WebKit Source: https://searchfox.org/wubkat/source/Source/WebCore/loader/TextTrackLoader.cpp#120

WPT Test Case: https://wpt.fyi/results/webvtt/parsing/file-parsing/signature-invalid.html?label=master&label=experimental&aligned=&q=signature

WPT Test Case Live Link: http://wpt.live/webvtt/parsing/file-parsing/signature-invalid.html

^ It is about first failure - 'signature, empty'.

Just wanted to raise, so we can fix it. Adding "WPTImpact" and "BrowserCompat" tags.

Thanks!

Comment 1 Radar WebKit Bug Importer 2023-08-18 18:34:04 PDT

<rdar://problem/114118292>

Comment 2 Ahmad Saleem 2023-08-24 15:45:07 PDT

NOTE - Doing 1-1 Blink Merge leads to two WPT regressions:

html/semantics/embedded-content/media-elements/track/track-element
/track-add-remove-cue.html

and

html/semantics/embedded-content/media-elements/track/track-element/track-text-track-cue-list.html

___

PR attempt: https://github.com/WebKit/WebKit/pull/16863

____

NOTE: We should sync 'webvtt' and look into why the tests are timing out and fix it.

Still investigating but closing my PR for time being.

Comment 3 Anne van Kesteren 2023-09-07 23:26:14 PDT

I was thinking of fixing this in TextTrackLoader::notifyFinished by changing

if (m_cueParser)
    m_cueParser->fileFinished();

to

if (m_cueParser)
    m_cueParser->fileFinished();
else
    m_state = Failed;

but I'm not sure if that would similarly regress tests.

However, looking closely at your commit it seems that WebKit would still invoke processNewCueData for the non-Failed states whereas Chromium would not (at least not at that point in time).

Syncing webvtt WPT is probably a good next step here. I can maybe do that today between meetings.

Comment 4 Kueno 2023-10-21 06:25:16 PDT

@Ahmad Saleem @Anne van Kesteren

Are you working on this issue? If not, I'd be happy to take it over.

Comment 5 Ahmad Saleem 2023-10-21 06:26:32 PDT

(In reply to Kueno from comment #4)
> @Ahmad Saleem @Anne van Kesteren
> 
> Are you working on this issue? If not, I'd be happy to take it over.

I am not. Go ahead and take it. You are doing amazing job!! We all really appreciate it.

Comment 6 Kueno 2023-10-22 06:13:08 PDT

Pull request: https://github.com/WebKit/WebKit/pull/19401

Comment 7 EWS 2023-10-23 02:47:58 PDT

Committed 269646@main (d40154a4fe8f): <https://commits.webkit.org/269646@main>

Reviewed commits have been landed. Closing PR #19401 and removing active labels.