Bug 99037 - [GStreamer] No CORS support for media elements
Summary: [GStreamer] No CORS support for media elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: Gtk, LayoutTestFailure
Depends on: 131516
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-11 06:06 PDT by Zan Dobersek
Modified: 2014-04-13 02:34 PDT (History)
19 users (show)

See Also:


Attachments
Patch (13.72 KB, patch)
2014-03-18 04:06 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (13.66 KB, patch)
2014-03-18 08:14 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Disable access to CORS failing streams when crossOrigin attribute is set (19.71 KB, patch)
2014-03-20 02:05 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
fixed nullptr style issue (20.32 KB, patch)
2014-03-20 04:12 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Updating after second reviews (20.33 KB, patch)
2014-04-10 07:23 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing crashes (20.36 KB, patch)
2014-04-11 06:04 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 Zan Dobersek 2012-10-11 06:06:51 PDT
There is currently no support for media elements in non-Chromium ports. This bug covers adding this for the GTK port.
Covered by http/tests/security/video-cross-origin-readback.html.
Comment 1 youenn fablet 2014-03-18 04:06:59 PDT
Created attachment 227027 [details]
Patch
Comment 2 WebKit Commit Bot 2014-03-18 04:08:19 PDT
Attachment 227027 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1026:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Philippe Normand 2014-03-18 04:23:12 PDT
Nice patch! Is the test also failing on EFL? I suppose it can also be unskipped for that port.
Comment 4 Jinwoo Song 2014-03-18 04:45:14 PDT
(In reply to comment #3)
> Nice patch! Is the test also failing on EFL? I suppose it can also be unskipped for that port.

Yes, EFL port may unskip the test case with this patch!
Comment 5 Xabier Rodríguez Calvar 2014-03-18 04:53:58 PDT
Comment on attachment 227027 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:456
> +    priv->passedCORSChecks = FALSE;

Should be false?
Comment 6 youenn fablet 2014-03-18 06:00:04 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Nice patch! Is the test also failing on EFL? I suppose it can also be unskipped for that port.
> 
> Yes, EFL port may unskip the test case with this patch!

I will unskip it and test EFL accordingly
Comment 7 youenn fablet 2014-03-18 06:09:55 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Nice patch! Is the test also failing on EFL? I suppose it can also be unskipped for that port.
> > 
> > Yes, EFL port may unskip the test case with this patch!
> 
> I will unskip it and test EFL accordingly

(In reply to comment #5)
> (From update of attachment 227027 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227027&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:456
> > +    priv->passedCORSChecks = FALSE;
> 
> Should be false?

I do not precisely know the expected coding rule.
passedCORSChecks is currently defined as a gboolean, hence FALSE.
But bool/false is fine by me if that is the preferred way.
Comment 8 Philippe Normand 2014-03-18 06:33:11 PDT
We indeed usually prefer non-GLib types wherever possible.
Comment 9 Martin Robinson 2014-03-18 06:44:40 PDT
Comment on attachment 227027 [details]
Patch

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

Nice fix. Some style nits:

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:62
> +        void handleResponseReceived(const ResourceResponse&, bool);

You should use an enum for this parameter.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:140
> +    gboolean passedCORSChecks;

bool is the right type here.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1020
> +    RequestOriginPolicy corsPolicy = (corsMode != MediaPlayerClient::Unspecified) ? PotentiallyCrossOriginEnabled : UseDefaultOriginRestrictionsForType;
> +    StoredCredentials allowCredentials = (corsMode == MediaPlayerClient::UseCredentials) ? AllowStoredCredentials : DoNotAllowStoredCredentials;

Nit: the parenthesis are unnecessary here.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1026
> +        m_origin = resourceLoader->document() ? resourceLoader->document()->securityOrigin() : NULL;

You should use nullptr instead of NULL.
Comment 10 Eric Carlson 2014-03-18 07:22:54 PDT
Comment on attachment 227027 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1230
> +Document* MediaPlayer::document()
> +{
> +    if (!m_mediaPlayerClient)
> +        return 0;
> +
> +    return m_mediaPlayerClient->mediaPlayerOwningDocument();
> +}

This is a layering violation, but it doesn't look like it is used. Why is this necessary?
Comment 11 youenn fablet 2014-03-18 07:28:14 PDT
(In reply to comment #10)
> (From update of attachment 227027 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227027&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:1230
> > +Document* MediaPlayer::document()
> > +{
> > +    if (!m_mediaPlayerClient)
> > +        return 0;
> > +
> > +    return m_mediaPlayerClient->mediaPlayerOwningDocument();
> > +}
> 
> This is a layering violation, but it doesn't look like it is used. Why is this necessary?

The goal is to get the SecurityOrigin and check CORS against it.
Would it be better to add a SecurityOrigin getter instead?
Comment 12 youenn fablet 2014-03-18 07:30:54 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 227027 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=227027&action=review
> > 
> > > Source/WebCore/platform/graphics/MediaPlayer.cpp:1230
> > > +Document* MediaPlayer::document()
> > > +{
> > > +    if (!m_mediaPlayerClient)
> > > +        return 0;
> > > +
> > > +    return m_mediaPlayerClient->mediaPlayerOwningDocument();
> > > +}
> > 
> > This is a layering violation, but it doesn't look like it is used. Why is this necessary?
> 
> The goal is to get the SecurityOrigin and check CORS against it.
> Would it be better to add a SecurityOrigin getter instead?

Woups, I was too quick...
Right, this function is no longer used.
The security origin is retrieved from CachedResourceLoader->document().
I will remove this function
Comment 13 youenn fablet 2014-03-18 08:14:23 PDT
Created attachment 227051 [details]
Patch

Fixed style issues, removed MediaPlayer::document(), Updated EFL TestExpectations
Comment 14 youenn fablet 2014-03-20 02:05:22 PDT
Created attachment 227274 [details]
Disable access to CORS failing streams when crossOrigin attribute is set
Comment 15 youenn fablet 2014-03-20 02:17:46 PDT
Last uploaded patch ensures that a stream that fails COR check is not played at all when crossOrigin attribute is set.
This is consistent with other loaders.

It seems to me though that these checks should best be handled at ResourceLoader level in the future.
That would allow to put in common all these CORS checks that already appear at various places in WebKit. 
That would simplify future support of CORS in other places, like in the Mac counterpart of GStreamer.
Comment 16 Xabier Rodríguez Calvar 2014-03-20 02:55:28 PDT
Comment on attachment 227274 [details]
Disable access to CORS failing streams when crossOrigin attribute is set

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:843
> +            GST_ELEMENT_ERROR(src, RESOURCE, READ, ("%s", "Cross-origin stream load denied by Cross-Origin Resource Sharing policy."), (0));
> +        else
> +            GST_ELEMENT_ERROR(src, RESOURCE, READ, ("Received %d HTTP error code", response.httpStatusCode()), (0));

(0) -> (nullptr)
Comment 17 youenn fablet 2014-03-20 04:12:19 PDT
Created attachment 227281 [details]
fixed nullptr style issue
Comment 18 youenn fablet 2014-04-09 06:49:27 PDT
Is there any other improvement I should bring to the patch?
Comment 19 Xabier Rodríguez Calvar 2014-04-09 07:43:59 PDT
Comment on attachment 227281 [details]
fixed nullptr style issue

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

Again my informal review: I have only some nits in the changelogs that you can fix when just before landing. Anyway as I am not reviewer, somebody should do a proper one and r+.

> Source/WebCore/ChangeLog:27
> +        Related test is http/tests/security/video-cross-origin-readback.html
> +        Disabled access to cross-origin streams that fail CORS check when crossorigin attribute is set.
> +        Related test is http/tests/security/video-cross-origin-accessfailure.html
> +
> +        Tests: http/tests/security/video-cross-origin-accessfailure.html
> +               http/tests/security/video-cross-origin-accesssameorigin.html
> +
> +        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
> +        (WebCore::MediaPlayerPrivateGStreamer::didPassCORSAccessCheck): Return whether media is cross-origin (tainted) or not by querying the gstreamer source layer
> +        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: Added MediaPlayerPrivateGStreamer::didPassCORSAccessCheck declaration
> +        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
> +        (webKitWebSrcStart): Passed CORS mode parameter to the streaming client. In case of CORS check failure, stop the resource loading
> +        (webKitSrcPassedCORSAccessCheck): Return whether CORS access control check was done and successful.
> +        (StreamingClient::handleResponseReceived): Take a parameter to assign the CORS access control check result
> +        (CachedResourceStreamingClient::CachedResourceStreamingClient): Updated setting of the ResourceLoaderOptions according CORS mode
> +        (CachedResourceStreamingClient::responseReceived): Check CORS
> +        (ResourceHandleStreamingClient::didReceiveResponse): No CORS check
> +        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.h: Added webKitSrcPassedCORSAccessCheck declaration

Nit: at the end of some sentences there is a missing ".". I say it because of coherence.

> LayoutTests/ChangeLog:17
> +        * platform/efl/TestExpectations: Enabled http/tests/security/video-cross-origin-readback.html

Nit: Missing .

> LayoutTests/ChangeLog:18
> +        * platform/gtk/TestExpectations: ditto

Nit: "Ditto."

> LayoutTests/ChangeLog:19
> +        * platform/mac/TestExpectations: Disabled http/tests/security/video-cross-origin-accessfailure.html

Nit: Missing .
Comment 20 youenn fablet 2014-04-09 07:50:54 PDT
(In reply to comment #19)
> (From update of attachment 227281 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227281&action=review
> 
> Again my informal review: I have only some nits in the changelogs that you can fix when just before landing. Anyway as I am not reviewer, somebody should do a proper one and r+.


Thanks for the review.
I will update the patch accordingly, then set r? and cq?.
Comment 21 Xabier Rodríguez Calvar 2014-04-09 08:56:47 PDT
(In reply to comment #20)
> Thanks for the review.
> I will update the patch accordingly, then set r? and cq?.

Eric, Martin or Phil, who had commented on this, could do it.
Comment 22 Philippe Normand 2014-04-09 23:28:05 PDT
Comment on attachment 227281 [details]
fixed nullptr style issue

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

Looks quite good, thanks! We just need a final iteration I think :)

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:841
> +            GST_ELEMENT_ERROR(src, RESOURCE, READ, ("%s", "Cross-origin stream load denied by Cross-Origin Resource Sharing policy."), (nullptr));

What is the %s for there?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1040
> +    // FIXME: In case of PotentiallyCrossOriginEnabled policy, preflight mode should sometimes be used (not simple request headers).
> +    // But CachedResourceLoader does not support it. Use DocumentThreadableLoader instead?

Can you please file a new bug for this and mention it in the comment?
Comment 23 youenn fablet 2014-04-10 01:26:48 PDT
(In reply to comment #22)
> (From update of attachment 227281 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227281&action=review
> 
> Looks quite good, thanks! We just need a final iteration I think :)
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:841
> > +            GST_ELEMENT_ERROR(src, RESOURCE, READ, ("%s", "Cross-origin stream load denied by Cross-Origin Resource Sharing policy."), (nullptr));
> 
> What is the %s for there?
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1040
> > +    // FIXME: In case of PotentiallyCrossOriginEnabled policy, preflight mode should sometimes be used (not simple request headers).
> > +    // But CachedResourceLoader does not support it. Use DocumentThreadableLoader instead?
> 
> Can you please file a new bug for this and mention it in the comment?

I filed bug 131484.
Checking what others are doing, Blink decided to not use preflight (https://code.google.com/p/chromium/issues/detail?id=173727), which seems to make sense.
I have not found information for Mozilla.
Comment 24 youenn fablet 2014-04-10 07:23:12 PDT
Created attachment 229048 [details]
Updating after second reviews
Comment 25 WebKit Commit Bot 2014-04-10 08:46:36 PDT
Comment on attachment 229048 [details]
Updating after second reviews

Clearing flags on attachment: 229048

Committed r167073: <http://trac.webkit.org/changeset/167073>
Comment 26 WebKit Commit Bot 2014-04-10 08:46:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 WebKit Commit Bot 2014-04-10 17:26:06 PDT
Re-opened since this is blocked by bug 131516
Comment 28 Martin Robinson 2014-04-10 18:02:46 PDT
Tests that crashed (5): flag all

+media/video-canvas-drawing.html
+media/video-canvas-drawing-output.html
+media/W3C/audio/paused/paused_false_during_play.html
+media/video-canvas-source.html
+media/video-canvas-alpha.html
Comment 29 youenn fablet 2014-04-11 00:06:13 PDT
(In reply to comment #28)
> Tests that crashed (5): flag all
> 
> +media/video-canvas-drawing.html
> +media/video-canvas-drawing-output.html
> +media/W3C/audio/paused/paused_false_during_play.html
> +media/video-canvas-source.html
> +media/video-canvas-alpha.html

Thanks for unrolling the patch.

For those 5 tests, WebKitWebSrc->priv is null when calling webKitSrcPassedCORSAccessCheck, leading to the crashes.
When running equivalent tests served through http, everything is fine.
Comment 30 Philippe Normand 2014-04-11 05:44:50 PDT
Will you provide an updated patch Youenn?
Comment 31 youenn fablet 2014-04-11 06:04:38 PDT
Created attachment 229130 [details]
Fixing crashes
Comment 32 youenn fablet 2014-04-11 06:07:48 PDT
(In reply to comment #30)
> Will you provide an updated patch Youenn?

Patch is uploaded, which fixes MediaPlayerPrivateGStreamer::didPassCORSAccessCheck().
It is working fine with EFL-WK1 but I have a problem with my set-up and cannot test it right now for GTK-WK2.
Once double checked, I will put it on review.
Comment 33 Philippe Normand 2014-04-11 06:15:17 PDT
Awesome thank for the follow-up :)
Comment 34 youenn fablet 2014-04-13 00:15:17 PDT
(In reply to comment #31)
> Created an attachment (id=229130) [details]
> Fixing crashes

The identified crashes are gone with the latest patch on GTK-WK2.
Comment 35 Xabier Rodríguez Calvar 2014-04-13 00:29:48 PDT
(In reply to comment #34)
> (In reply to comment #31)
> > Created an attachment (id=229130) [details] [details]
> > Fixing crashes
> 
> The identified crashes are gone with the latest patch on GTK-WK2.

Probably bug 131488.
Comment 36 WebKit Commit Bot 2014-04-13 02:33:57 PDT
Comment on attachment 229130 [details]
Fixing crashes

Clearing flags on attachment: 229130

Committed r167193: <http://trac.webkit.org/changeset/167193>
Comment 37 WebKit Commit Bot 2014-04-13 02:34:07 PDT
All reviewed patches have been landed.  Closing bug.