Bug 97532 – [WK2][WTR] WTR bundle client loads binary data as text

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Mikhail Pozdnyakov]

Currently InjectedBundlePage::decidePolicyForResponse always returns WKBundlePagePolicyActionUse even for Response MIME types that cannot be shown. This causes failing of http/tests/loading/text-content-type-with-binary-extension.html test, which checks that binary data is not loaded as text.

[Mikhail Pozdnyakov]

Created attachment 165539 [details]
patch

WTR::InjectedBundlePage::decidePolicyForResponse now checks response MIME type and returns WKBundlePagePolicyActionPassThrough (skips for handling by UI process) if it cannot be shown.

[Kenneth Rohde Christiansen]

Comment on attachment 165539 [details]
patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:419
> +// This function is here to keep consistency with WebKit::WebPageProxy::canShowMIMEType().
> +// Actually it does not need anything from page.
> +bool WKBundlePageCanShowMIMEType(WKBundlePageRef, WKStringRef mimeTypeRef)

Isn't that comment better in the changelog? I think so. Also could it potentially need something from page in the future?

[Mikhail Pozdnyakov]

Created attachment 165551 [details]
patch v2

Rebased. Took comments from Kenneth into consideration.

[Mikhail Pozdnyakov]

> Also could it potentially need something from page in the future?
Potentially maybe, but I don't think we will need it in WTR.

[WebKit Review Bot]

Comment on attachment 165551 [details]
patch v2

Clearing flags on attachment: 165551

Committed r129479: <http://trac.webkit.org/changeset/129479>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

Crash in fast/loader/reload-zero-byte-plugin.html on EFL being tracked in bug 97565.

[Raphael Kubo da Costa (:rakuco)]

Could you also please check if the failure in http/tests/security/contentSecurityPolicy/object-src-url-allowed.html is related to this change?

[Benjamin Poulain]

Comment on attachment 165551 [details]
patch v2

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:420
> +bool WKBundlePageCanShowMIMEType(WKBundlePageRef, WKStringRef mimeTypeRef)
> +{
> +    using WebCore::MIMETypeRegistry;

Why do your own thing instead of moving the code of WebPageProxy::canShowMIMEType() in a better place??

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:422
> +    const WTF::String mimeType = toImpl(mimeTypeRef)->string();

This should be toWTFString.
You should not prefix String with WTF::

[Mikhail Pozdnyakov]

(In reply to comment #9)
> (From update of attachment 165551 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165551&action=review
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:420
> > +bool WKBundlePageCanShowMIMEType(WKBundlePageRef, WKStringRef mimeTypeRef)
> > +{
> > +    using WebCore::MIMETypeRegistry;
> 
> Why do your own thing instead of moving the code of WebPageProxy::canShowMIMEType() in a better place??

Actually this code can be shared even better :) please take a look at bug97547.

> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:422
> > +    const WTF::String mimeType = toImpl(mimeTypeRef)->string();
> 
> This should be toWTFString.
As far as I see toWTFString is declared in ./Tools/WebKitTestRunner/StringFunctions.h which is not part of WebKit source.

[Benjamin Poulain]

> > Why do your own thing instead of moving the code of WebPageProxy::canShowMIMEType() in a better place??
> 
> Actually this code can be shared even better :) please take a look at bug97547.

Good.
It would have be nice doing that beforehand.

> > > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:422
> > > +    const WTF::String mimeType = toImpl(mimeTypeRef)->string();
> > 
> > This should be toWTFString.
> As far as I see toWTFString is declared in ./Tools/WebKitTestRunner/StringFunctions.h which is not part of WebKit source.

No.
See WKSharedAPICast.h

[Mikhail Pozdnyakov]

(In reply to comment #11)
> 
> No.
> See WKSharedAPICast.h
Oh, OK. Thanks for pointing this out. Strange that toWTFString is not used anywhere in Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp, 
and I guess it should be fixed everywhere than..

[Mikhail Pozdnyakov]

 Strange that toWTFString is not used anywhere in Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp, 
> and I guess it should be fixed everywhere than..
addressed in https://bugs.webkit.org/show_bug.cgi?id=97766