Bug 137064

Summary: [GStreamer] Do not use CachedResourceLoader, SecurityOrigin, ResourceBuffer and other WebCore types
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: PlatformAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, darin, dbates, eric.carlson, gns, pnormand, sam, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 21354    
Attachments:
Description Flags
Patch
none
Try to fix the mac build
none
Patch
none
Rebased patch after r174563
pnormand: review+
Patch or landing none

Description Carlos Garcia Campos 2014-09-24 03:35:13 PDT
It's a layering violation.
Comment 1 Carlos Garcia Campos 2014-09-24 04:15:56 PDT
Created attachment 238596 [details]
Patch

This patch is for GStreamer, because it's the only media backend I can actually test, but I added the new classes with the current mac implementation in mind, so they should be ready to be used by the mac port as well. After this patch fixing the mac port should be easy, I can try to do it myself, but it would be like a shot in the dark, and I'll need help to add the new files to the xcode files.
Comment 2 WebKit Commit Bot 2014-09-24 04:17:49 PDT
Attachment 238596 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaElement.cpp:5657:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Carlos Garcia Campos 2014-09-24 04:50:48 PDT
Created attachment 238597 [details]
Try to fix the mac build

The style issue is a false positive I think
Comment 4 WebKit Commit Bot 2014-09-24 04:53:23 PDT
Attachment 238597 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaElement.cpp:5657:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Carlos Garcia Campos 2014-09-24 08:45:07 PDT
I guess the mac build failure is because I have to add a symbol to a file or mark it as public or something like that.
Comment 6 Carlos Garcia Campos 2014-09-27 02:15:21 PDT
(In reply to comment #5)
> I guess the mac build failure is because I have to add a symbol to a file or mark it as public or something like that.

Ah, I see now, I'm using MediaResourceLoader unconditionally in HTMLMediaElement::mediaPlayerCreateResourceLoader(). I can fix the windows build by adding the new files to the build, but I can't fix mac build (don't have a mac to deal with xcode files). I could add #if USE(GSTREAMER) with a FIXME that could be removed when mac switches to implement MediaPlayerResourceLoaderClient
Comment 7 Gustavo Noronha (kov) 2014-09-30 10:35:52 PDT
There was a tool that the google folks created to add files to xcode project files, I used it in a patch I did which moved files used by Mac, but I can't remember its name, find it.
Comment 8 Carlos Garcia Campos 2014-09-30 10:39:14 PDT
(In reply to comment #7)
> There was a tool that the google folks created to add files to xcode project files, I used it in a patch I did which moved files used by Mac, but I can't remember its name, find it.

I know, I tried it a couple of times in the past and never worked for me. Thanks anyway.
Comment 9 Daniel Bates 2014-09-30 12:22:49 PDT
Created attachment 238945 [details]
Patch

Add build files to Xcode and Visual Studio projects and update WebCore change log entry.
Comment 10 WebKit Commit Bot 2014-09-30 12:25:24 PDT
Attachment 238945 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaElement.cpp:5652:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Carlos Garcia Campos 2014-09-30 23:21:43 PDT
(In reply to comment #9)
> Created an attachment (id=238945) [details]
> Patch
> 
> Add build files to Xcode and Visual Studio projects and update WebCore change log entry.

Thank you!
Comment 12 Philippe Normand 2014-10-02 09:57:22 PDT
Comment on attachment 238945 [details]
Patch

The changes to the GStreamer code look good to me.
Comment 13 Philippe Normand 2014-10-02 09:58:28 PDT
Nice clean-up Carlos, thanks :)
Comment 14 Carlos Garcia Campos 2014-10-09 23:59:24 PDT
Created attachment 239602 [details]
Rebased patch after r174563
Comment 15 WebKit Commit Bot 2014-10-10 00:01:20 PDT
Attachment 239602 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaElement.cpp:5656:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Carlos Garcia Campos 2014-10-14 05:00:59 PDT
Darin, Eric, could you please confirm this approach is fine for mac also before I land this patch?
Comment 17 Alexey Proskuryakov 2014-10-14 11:23:21 PDT
Comment on attachment 239602 [details]
Rebased patch after r174563

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

A few random comments below. I can't determine how this approach affects Mac.

> Source/WebCore/loader/MediaResourceLoader.cpp:57
> +    if (m_resource)
> +        return false;

Can the loader be started/stopped multiple times? If not, assertions would be more appropriate than runtime checks.

> Source/WebCore/loader/MediaResourceLoader.cpp:69
> +    // TODO: Decide whether to use preflight mode for cross-origin requests (see http://wkbug.com/131484).

Please use FIXME, not TODO.

However, the answer seems quite obvious, we do not need preflight. JS code does not control request method or headers, so it cannot attack a naive intranet server that does not expect malicious requests of these types. All servers need to expect malicious requests that are GET or POSTs generated by forms, because web pages could always send those - CORS makes it possible to send other types of cross-origin requests, and these need preflight.

I recommend to remove this comment, and resolve the associated bug.

> Source/WebCore/loader/MediaResourceLoader.cpp:94
> +// CachedResourceClient

Please do not add these comments to .cpp files. Most people don't maintain them, so they just become incorrect with time.

> Source/WebCore/loader/MediaResourceLoader.cpp:103
> +        static NeverDestroyed<const String> consoleMessage(ASCIILiteral("Cross-origin media resource load denied by Cross-Origin Resource Sharing policy."));

There is no need to complicate code to optimize for this rare error case. A regular transient string would work perfectly well.

> Source/WebCore/loader/MediaResourceLoader.cpp:106
> +        m_client->accessControlCheckFailed(ResourceError(errorDomainWebKitInternal, 0, response.url().string(), consoleMessage.get()));

Do we get two error lines in Web Inspector console as a result?

> Source/WebCore/loader/MediaResourceLoader.h:40
> +class MediaResourceLoader final : public MediaPlayerResourceLoader, CachedRawResourceClient {

Naming seems unhelpful here. Inheritance means that MediaResourceLoader is a specific type of MediaPlayerResourceLoader, but there is nothing in the name explaining how it is a subtype of it.
Comment 18 Carlos Garcia Campos 2014-10-14 23:16:01 PDT
(In reply to comment #17)
> (From update of attachment 239602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239602&action=review

Thanks for looking at it. Note that this is mostly a refactoring, so most of the comments are about code that I have just moved to a different place.

> A few random comments below. I can't determine how this approach affects Mac.

The idea is that mac switch to use the MediaResourceLoader as well, instead of using the CacheResourceLoader directly, which is a layering violation. So, once this patch lands, WebCoreAVFResourceLoader should turn into a MediaPlayerResourceLoaderClient instead of a CachedRawResourceClient. 

> > Source/WebCore/loader/MediaResourceLoader.cpp:57
> > +    if (m_resource)
> > +        return false;
> 
> Can the loader be started/stopped multiple times? If not, assertions would be more appropriate than runtime checks.

start() is always called once right after the object is created, but stop can be called multiple times, because the destructor call stop, and in most of the cases the loader has already been stopped when it's destroyed.

> > Source/WebCore/loader/MediaResourceLoader.cpp:69
> > +    // TODO: Decide whether to use preflight mode for cross-origin requests (see http://wkbug.com/131484).
> 
> Please use FIXME, not TODO.
> 
> However, the answer seems quite obvious, we do not need preflight. JS code does not control request method or headers, so it cannot attack a naive intranet server that does not expect malicious requests of these types. All servers need to expect malicious requests that are GET or POSTs generated by forms, because web pages could always send those - CORS makes it possible to send other types of cross-origin requests, and these need preflight.
> 
> I recommend to remove this comment, and resolve the associated bug.

Honestly, that comes from WebKitWebSourceGStreamer.cpp, and I don't even know what preflight means. I'll remove the comment anyway.

> > Source/WebCore/loader/MediaResourceLoader.cpp:94
> > +// CachedResourceClient
> 
> Please do not add these comments to .cpp files. Most people don't maintain them, so they just become incorrect with time.

Ok.

> > Source/WebCore/loader/MediaResourceLoader.cpp:103
> > +        static NeverDestroyed<const String> consoleMessage(ASCIILiteral("Cross-origin media resource load denied by Cross-Origin Resource Sharing policy."));
> 
> There is no need to complicate code to optimize for this rare error case. A regular transient string would work perfectly well.

Ok.

> > Source/WebCore/loader/MediaResourceLoader.cpp:106
> > +        m_client->accessControlCheckFailed(ResourceError(errorDomainWebKitInternal, 0, response.url().string(), consoleMessage.get()));
> 
> Do we get two error lines in Web Inspector console as a result?

hmm, I haven't checked the web inspector, but only one message is written to the js console, so I guess we get only one, like in the layout tests results.

> > Source/WebCore/loader/MediaResourceLoader.h:40
> > +class MediaResourceLoader final : public MediaPlayerResourceLoader, CachedRawResourceClient {
> 
> Naming seems unhelpful here. Inheritance means that MediaResourceLoader is a specific type of MediaPlayerResourceLoader, but there is nothing in the name explaining how it is a subtype of it.

What about PlatformMediaResourceLoader?
Comment 19 Carlos Garcia Campos 2014-10-16 05:44:54 PDT
(In reply to comment #18)
> > > Source/WebCore/loader/MediaResourceLoader.h:40
> > > +class MediaResourceLoader final : public MediaPlayerResourceLoader, CachedRawResourceClient {
> > 
> > Naming seems unhelpful here. Inheritance means that MediaResourceLoader is a specific type of MediaPlayerResourceLoader, but there is nothing in the name explaining how it is a subtype of it.
> 
> What about PlatformMediaResourceLoader?

or MediaResourceLoaderBase?
Comment 20 Eric Carlson 2014-10-21 09:32:59 PDT
Comment on attachment 239602 [details]
Rebased patch after r174563

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

This approach seems sensible for the Mac as well. Thanks for taking on this cleanup task!

>>> Source/WebCore/loader/MediaResourceLoader.h:40
>>> +class MediaResourceLoader final : public MediaPlayerResourceLoader, CachedRawResourceClient {
>> 
>> Naming seems unhelpful here. Inheritance means that MediaResourceLoader is a specific type of MediaPlayerResourceLoader, but there is nothing in the name explaining how it is a subtype of it.
> 
> What about PlatformMediaResourceLoader?

If you are suggesting that MediaPlayerResourceLoader be renamed PlatformMediaResourceLoader and this class stays MediaResourceLoader, this seems fine to me.
Comment 21 Carlos Garcia Campos 2014-10-22 06:27:47 PDT
Created attachment 240270 [details]
Patch or landing
Comment 22 WebKit Commit Bot 2014-10-22 06:29:01 PDT
Attachment 240270 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaElement.cpp:5668:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Carlos Garcia Campos 2014-10-22 08:12:14 PDT
Committed r175050: <http://trac.webkit.org/changeset/175050>