Bug 161792 - TextTrackLoader should use FetchOptions::mode according its crossOrigin attribute
Summary: TextTrackLoader should use FetchOptions::mode according its crossOrigin attri...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-09 05:14 PDT by youenn fablet
Modified: 2016-09-09 10:27 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.82 KB, patch)
2016-09-09 05:30 PDT, youenn fablet
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (791.24 KB, application/zip)
2016-09-09 06:26 PDT, Build Bot
no flags Details
Rebasing failing tests (20.52 KB, patch)
2016-09-09 08:07 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-09-09 05:14:03 PDT
TextTrackLoader should use FetchOptions::mode according its crossOrigin attribute
Comment 1 youenn fablet 2016-09-09 05:30:48 PDT
Created attachment 288397 [details]
Patch
Comment 2 youenn fablet 2016-09-09 05:48:08 PDT
(In reply to comment #1)
> Created attachment 288397 [details]
> Patch

Patch is changing the behavior in the case of no crossOrigin attribute and URL is cross-origin. Before the patch, access was refused.
With the patch, access is ok.
This seems to align with https://html.spec.whatwg.org/multipage/embedded-content.html#sourcing-out-of-band-text-tracks

Also, this check is not all that useful as it could be bypassed by a same-origin URL that would redirect to a cross-origin one.

I hesitated to keep or not that check.
Eric, do you know the rationale behind it?
Comment 3 Build Bot 2016-09-09 06:26:49 PDT
Comment on attachment 288397 [details]
Patch

Attachment 288397 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2041158

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrack/addCue.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/id.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/endTime.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/pauseOnExit.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/startTime.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/track.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrack/removeCue.html
Comment 4 Build Bot 2016-09-09 06:26:53 PDT
Created attachment 288398 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 5 youenn fablet 2016-09-09 08:07:03 PDT
Created attachment 288404 [details]
Rebasing failing tests
Comment 6 youenn fablet 2016-09-09 08:09:53 PDT
(In reply to comment #3)
> Comment on attachment 288397 [details]
> Patch
> 
> Attachment 288397 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/2041158
> 
> New failing tests:
> imported/w3c/web-platform-tests/html/semantics/embedded-content/media-
> elements/interfaces/TextTrack/addCue.html
> imported/w3c/web-platform-tests/html/semantics/embedded-content/media-
> elements/interfaces/TextTrackCue/id.html
> imported/w3c/web-platform-tests/html/semantics/embedded-content/media-
> elements/interfaces/TextTrackCue/endTime.html
> imported/w3c/web-platform-tests/html/semantics/embedded-content/media-
> elements/interfaces/TextTrackCue/pauseOnExit.html
> imported/w3c/web-platform-tests/html/semantics/embedded-content/media-
> elements/interfaces/TextTrackCue/startTime.html
> imported/w3c/web-platform-tests/html/semantics/embedded-content/media-
> elements/interfaces/TextTrackCue/track.html
> imported/w3c/web-platform-tests/html/semantics/embedded-content/media-
> elements/interfaces/TextTrack/removeCue.html

They were failing as the src was data URLs.
This allows passing some more tests.

The change of behavior is due to the removed check.
In terms of interoperability, this allows aligning with Firefox and the spec.
But this is breaking with Chromium and past WebKit behavior.
Comment 7 Alex Christensen 2016-09-09 08:24:03 PDT
Comment on attachment 288404 [details]
Rebasing failing tests

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

> Source/WebCore/ChangeLog:15
> +        Also, this check could be bypassed in the case of a same-origin URL redirecting to a cross-origin one.

You mean it could before this patch but not after this patch, right?
Comment 8 youenn fablet 2016-09-09 08:26:50 PDT
(In reply to comment #7)
> Comment on attachment 288404 [details]
> Rebasing failing tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288404&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        Also, this check could be bypassed in the case of a same-origin URL redirecting to a cross-origin one.
> 
> You mean it could before this patch but not after this patch, right?

Yes, if we really wanted to enforce this check for redirections, we should set fetch mode to same-origin.
Comment 9 Eric Carlson 2016-09-09 09:17:51 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > Created attachment 288397 [details]
> > Patch
> 
> Patch is changing the behavior in the case of no crossOrigin attribute and
> URL is cross-origin. Before the patch, access was refused.
> With the patch, access is ok.
> This seems to align with
> https://html.spec.whatwg.org/multipage/embedded-content.html#sourcing-out-of-
> band-text-tracks
> 
> Also, this check is not all that useful as it could be bypassed by a
> same-origin URL that would redirect to a cross-origin one.
> 
> I hesitated to keep or not that check.
> Eric, do you know the rationale behind it?

The code was added for the original spec text, see bug 73184.
Comment 10 WebKit Commit Bot 2016-09-09 10:27:00 PDT
Comment on attachment 288404 [details]
Rebasing failing tests

Clearing flags on attachment: 288404

Committed r205750: <http://trac.webkit.org/changeset/205750>
Comment 11 WebKit Commit Bot 2016-09-09 10:27:05 PDT
All reviewed patches have been landed.  Closing bug.