Bug 137064 – [GStreamer] Do not use CachedResourceLoader, SecurityOrigin, ResourceBuffer and other WebCore types

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Carlos Garcia Campos]

It's a layering violation.

[Carlos Garcia Campos]

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.

[WebKit Commit Bot]

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.

[Carlos Garcia Campos]

Created attachment 238597 [details]
Try to fix the mac build

The style issue is a false positive I think

[WebKit Commit Bot]

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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

(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

[Gustavo Noronha (kov)]

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.

[Carlos Garcia Campos]

(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.

[Daniel Bates]

Created attachment 238945 [details]
Patch

Add build files to Xcode and Visual Studio projects and update WebCore change log entry.

[WebKit Commit Bot]

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.

[Carlos Garcia Campos]

(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!

[Philippe Normand]

Comment on attachment 238945 [details]
Patch

The changes to the GStreamer code look good to me.

[Philippe Normand]

Nice clean-up Carlos, thanks :)

[Carlos Garcia Campos]

Created attachment 239602 [details]
Rebased patch after r174563

[WebKit Commit Bot]

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.

[Carlos Garcia Campos]

Darin, Eric, could you please confirm this approach is fine for mac also before I land this patch?

[Alexey Proskuryakov]

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.

[Carlos Garcia Campos]

(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?

[Carlos Garcia Campos]

(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?

[Eric Carlson]

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.

[Carlos Garcia Campos]

Created attachment 240270 [details]
Patch or landing

[WebKit Commit Bot]

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.

[Carlos Garcia Campos]

Committed r175050: <http://trac.webkit.org/changeset/175050>