Bug 97532 - [WK2][WTR] WTR bundle client loads binary data as text
Summary: [WK2][WTR] WTR bundle client loads binary data as text
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-25 00:06 PDT by Mikhail Pozdnyakov
Modified: 2012-09-27 01:18 PDT (History)
5 users (show)

See Also:


Attachments
patch (7.60 KB, patch)
2012-09-25 00:29 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v2 (7.78 KB, patch)
2012-09-25 01:23 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2012-09-25 00:06:20 PDT
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.
Comment 1 Mikhail Pozdnyakov 2012-09-25 00:29:29 PDT
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.
Comment 2 Kenneth Rohde Christiansen 2012-09-25 00:43:36 PDT
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?
Comment 3 Mikhail Pozdnyakov 2012-09-25 01:23:03 PDT
Created attachment 165551 [details]
patch v2

Rebased. Took comments from Kenneth into consideration.
Comment 4 Mikhail Pozdnyakov 2012-09-25 01:27:38 PDT
> Also could it potentially need something from page in the future?
Potentially maybe, but I don't think we will need it in WTR.
Comment 5 WebKit Review Bot 2012-09-25 02:19:38 PDT
Comment on attachment 165551 [details]
patch v2

Clearing flags on attachment: 165551

Committed r129479: <http://trac.webkit.org/changeset/129479>
Comment 6 WebKit Review Bot 2012-09-25 02:19:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Raphael Kubo da Costa (:rakuco) 2012-09-25 07:16:44 PDT
Crash in fast/loader/reload-zero-byte-plugin.html on EFL being tracked in bug 97565.
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-09-25 08:11:40 PDT
Could you also please check if the failure in http/tests/security/contentSecurityPolicy/object-src-url-allowed.html is related to this change?
Comment 9 Benjamin Poulain 2012-09-25 12:28:15 PDT
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::
Comment 10 Mikhail Pozdnyakov 2012-09-25 23:49:09 PDT
(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.
Comment 11 Benjamin Poulain 2012-09-26 11:29:04 PDT
> > 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
Comment 12 Mikhail Pozdnyakov 2012-09-26 13:34:51 PDT
(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..
Comment 13 Mikhail Pozdnyakov 2012-09-27 01:18:31 PDT
 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