[Webkit-unassigned] [Bug 135379] iOS 8 / OSX 10.10 WebGL - using a video from an other domain fails (CORS bug)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=135379

Jer Noble <jer.noble at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #266976|                            |review-
              Flags|                            |

--- Comment #20 from Jer Noble <jer.noble at apple.com> ---
Comment on attachment 266976
  --> https://bugs.webkit.org/attachment.cgi?id=266976
Proof of concept patch

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

Thanks for the patch. We've previously looked into this mechanism for custom data loading of media resources, and our experiments revealed a number of problems with this approach.  Namely:

This will only work for initial requests.  Subsequent requests (for additional byte ranges, for sub-resources, or due to HTTP redirects) will not come through this path, and will thus we will not get a chance to do a CORS check on those requests.  We have to assume that these requests would not pass CORS, or we would risk opening up a hole in CORS support.  Additionally, this path is only triggered for the HLS manifest load, but requests for HLS media segments does not come through this API.

I am going to give this a r- due to the above, but only due to the shortcomings of the approach in the patch, not because of the patch itself. In addition, there are a few things in your patch I'd like to call out that would need to be fixed in the hypothetical case that the approach was valid.

> avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:412
> +    int m_didPassCORSAccessCheck;

This should be defined in terms of an enum, with defined states. Such as:

enum CORSAccessCheckResult {
    CORSAccessUnknown,
    CORSAccessDenied,
    CORSAccessAllowed,
};
CORSAccessCheckResult m_didPassCORSAccessCheck { CORSAccessUnknown };

> avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:514
> +    , m_didPassCORSAccessCheck(0)

With C++11, this style of initialization can be put in the header.

> avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:839
> +    if (parsedURL.protocol().lower() == "http") {
> +        parsedURL.setProtocol("mock-http");
> +    } else if (parsedURL.protocol().lower() == "https") {
> +        parsedURL.setProtocol("mock-https");
> +    }

WebKit coding style guidelines <https://webkit.org/code-style-guidelines/> specify dropping braces on single-line control statements.

> avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2222
> +bool MediaPlayerPrivateAVFoundationObjC::didPassCORSAccessCheck() const
> +{
> +    //ASSERT(m_didPassCORSAccessCheck != 0);
> +    return m_didPassCORSAccessCheck == 2;

These should use the enum values i mentioned above.  So this would become:

return m_didPassCORSAccessCheck == CORSAccessAllowed;

> avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2227
> +void MediaPlayerPrivateAVFoundationObjC::didFinishCORSAccessCheck(bool passed) {
> +    m_didPassCORSAccessCheck = passed ? 2 : 1;
> +}

And this would become:

m_didPassCORSAccessCheck = passed ? CORSAccessCheckAllowed : CORSAccessCheckDenied;

> avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3445
> +    URL url([m_avAsset resolvedURL]);
> +    if (url.protocol() == "mock-http") {
> +        url.setProtocol("http");
> +    } else if (url.protocol() == "mock-https") {
> +        url.setProtocol("https");
> +    }
> +    return url;

Ditto about the WebKit coding style guidelines.

> avfoundation/objc/WebCoreAVFResourceLoader.mm:75
> +    if (requestURL.protocol() == "mock-http") {
> +        requestURL.setProtocol("http");
> +    } else if (requestURL.protocol() == "mock-https") {
> +        requestURL.setProtocol("https");
> +    }

Ditto.

> avfoundation/objc/WebCoreAVFResourceLoader.mm:83
> +    m_origin = loader->document() ? loader->document()->securityOrigin() : nullptr;
> +    ASSERT(m_origin.get());

No need to add .get() to a RefPtr or RetainPtr when testing nullity.  Those classes provide a convenience bool operator.

> avfoundation/objc/WebCoreAVFResourceLoader.mm:117
> +    bool didPassCORSAccessCheck = resource->passesAccessControlCheck(*m_origin.get());

Also, they override operator*(), so no need to add a .get() before dereferencing.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151210/2bbaf1c6/attachment.html>