Bug 68071 – Rename FrameLoaderClient::allowImages to FrameLoaderClient::allowImage and include the image URL as parameter

• Component: New Bugs
• Status: RESOLVED FIXED

Include the image URL in the call to FrameLoaderClient::allowImages

Created attachment 107312 [details]
Patch

[WebKit Review Bot]

Comment on attachment 107312 [details]
Patch

Attachment 107312 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9661453

[Alexey Proskuryakov]

Comment on attachment 107312 [details]
Patch

Could you please provide rationale for this change?

The patch is not acceptable as is because "allowImages" function name becomes incorrect, but let's take a step back to figure out whether this is something that should be in WebKit, and whether there is already a way to achieve what you need.

(In reply to comment #3)
> (From update of attachment 107312 [details])
> Could you please provide rationale for this change?

sorry for not including that info in the first place.

I want to be able to block images from certain security origins. A user might want to allow images from the first party origin only, or always block images from a certain origin.

> 
> The patch is not acceptable as is because "allowImages" function name becomes incorrect, but let's take a step back to figure out whether this is something that should be in WebKit, and whether there is already a way to achieve what you need.

That's a good point. The least would be to rename allowImages to allowImage, as it would now talk about a specific image.

An alternative would be to fail the network requests for the unwanted images, however, this would display the "broken image" icons instead of just nothing.

[Adam Barth]

Comment on attachment 107312 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        Include the image URL in the call to FrameLoaderClient::allowImages
> +        https://bugs.webkit.org/show_bug.cgi?id=68071
> +
> +        Reviewed by NOBODY (OOPS!).
> +

One thing that might be helpful is to add more information to the ChangeLog explaining why you're interested in making this change.  The ChangeLog entry is an important part of communicating the "why" behind a change.

[Alexey Proskuryakov]

> An alternative would be to fail the network requests for the unwanted images, however, this would display the "broken image" icons instead of just nothing.

WebCore asks whether to display it via shouldPaintBrokenImage() FrameLoaderClient call.

[Adam Barth]

Failing the request at the network level doesn't seem like the best approach.  Whether to load an image seems like a policy decision and policy decisions should be made by the Client not the Platform.

Created attachment 107373 [details]
Patch

Comment on attachment 107373 [details]
Patch

I've changed allowImages to allowImage and added more information to the changelog entry in WebCore/

[Adam Barth]

Comment on attachment 107373 [details]
Patch

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

This is looking good, but we probably should iterate another time.

> Source/WebCore/loader/FrameLoaderClient.h:307
>          virtual bool allowJavaScript(bool enabledPerSettings) { return enabledPerSettings; }
>          virtual bool allowPlugins(bool enabledPerSettings) { return enabledPerSettings; }
> -        virtual bool allowImages(bool enabledPerSettings) { return enabledPerSettings; }
> +        virtual bool allowImage(const KURL& imageURL, bool enabledPerSettings) { return enabledPerSettings; }
>          virtual bool allowDisplayingInsecureContent(bool enabledPerSettings, SecurityOrigin*, const KURL&) { return enabledPerSettings; }
>          virtual bool allowRunningInsecureContent(bool enabledPerSettings, SecurityOrigin*, const KURL&) { return enabledPerSettings; }

To make these consistent, perhaps the URL should be the last parameter?  Do we also need a didNotAllowImage to parallel didNotAllowPlugins?  Perhaps there isn't any code that needs to know whether images are allowed without being about to load an image.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:131
> +        KURL requestURL = request.url();
> +        if (!f->loader()->client()->allowImage(requestURL, !settings || settings->areImagesEnabled()))

I see that the loading-from-the-MemoryCache case is handled here.

> Tools/DumpRenderTree/chromium/WebPermissions.h:42
> -    virtual bool allowImages(WebKit::WebFrame*, bool enabledPerSettings);
> +    virtual bool allowImage(WebKit::WebFrame*, const WebKit::WebURL& imageURL, bool enabledPerSettings);

It would be ideal to add a LayoutTest to exercise this new policy lever.

(In reply to comment #10)
> (From update of attachment 107373 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107373&action=review
> 
> This is looking good, but we probably should iterate another time.
> 
> > Source/WebCore/loader/FrameLoaderClient.h:307
> >          virtual bool allowJavaScript(bool enabledPerSettings) { return enabledPerSettings; }
> >          virtual bool allowPlugins(bool enabledPerSettings) { return enabledPerSettings; }
> > -        virtual bool allowImages(bool enabledPerSettings) { return enabledPerSettings; }
> > +        virtual bool allowImage(const KURL& imageURL, bool enabledPerSettings) { return enabledPerSettings; }
> >          virtual bool allowDisplayingInsecureContent(bool enabledPerSettings, SecurityOrigin*, const KURL&) { return enabledPerSettings; }
> >          virtual bool allowRunningInsecureContent(bool enabledPerSettings, SecurityOrigin*, const KURL&) { return enabledPerSettings; }
> 
> To make these consistent, perhaps the URL should be the last parameter?  Do we also need a didNotAllowImage to parallel didNotAllowPlugins?  Perhaps there isn't any code that needs to know whether images are allowed without being about to load an image.

Right, I didn't see a place where we'd just check the setting.

> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:131
> > +        KURL requestURL = request.url();
> > +        if (!f->loader()->client()->allowImage(requestURL, !settings || settings->areImagesEnabled()))
> 
> I see that the loading-from-the-MemoryCache case is handled here.
> 
> > Tools/DumpRenderTree/chromium/WebPermissions.h:42
> > -    virtual bool allowImages(WebKit::WebFrame*, bool enabledPerSettings);
> > +    virtual bool allowImage(WebKit::WebFrame*, const WebKit::WebURL& imageURL, bool enabledPerSettings);
> 
> It would be ideal to add a LayoutTest to exercise this new policy lever.

I have a patch that adds layoutTestController.dumpPermissionClientCallbacks() and it would then print the URL passed the allowImage(). However, the layout test is run from file: so the URL depends on the location of your checkout.

A possible solution was to run this test over http. Or to strip everything before LayoutTests/ if the URL is a file: URL.

wdyt?

[Alexey Proskuryakov]

I just noticed that this functionality is Chromium only.

I guess one thing for you to consider in review is whether you want to make policy decision based on original or final URL in case of redirect.

(In reply to comment #12)
> I just noticed that this functionality is Chromium only.
> 
> I guess one thing for you to consider in review is whether you want to make policy decision based on original or final URL in case of redirect.

Good point. We should probably check the policy for both URLs (actually all URLs if it's a redirect chain, but I guess that's not yet possible).

Adam, what do you think?

[Adam Barth]

> I have a patch that adds layoutTestController.dumpPermissionClientCallbacks() and it would then print the URL passed the allowImage(). However, the layout test is run from file: so the URL depends on the location of your checkout.
> 
> A possible solution was to run this test over http. Or to strip everything before LayoutTests/ if the URL is a file: URL.

This issue comes up in a bunch of places.  DumpRenderTree should already have code to normalize file URLs.  I know that happens for console messages, for example.

> > I guess one thing for you to consider in review is whether you want to make policy decision based on original or final URL in case of redirect.
> 
> Good point. We should probably check the policy for both URLs (actually all URLs if it's a redirect chain, but I guess that's not yet possible).
> 
> Adam, what do you think?

I would go with the final URL.  This is also an important case to test.  Basically, the question boils down to whether you want to control which sorts of images are displayed or whether you want to control which sorts of network requests the user agent makes.  In this case, it seems clear we're talking about which sorts of images are displayed, so, like canvas taint, we want to use the final URL.

(In reply to comment #14)
> > I have a patch that adds layoutTestController.dumpPermissionClientCallbacks() and it would then print the URL passed the allowImage(). However, the layout test is run from file: so the URL depends on the location of your checkout.
> > 
> > A possible solution was to run this test over http. Or to strip everything before LayoutTests/ if the URL is a file: URL.
> 
> This issue comes up in a bunch of places.  DumpRenderTree should already have code to normalize file URLs.  I know that happens for console messages, for example.
> 
> > > I guess one thing for you to consider in review is whether you want to make policy decision based on original or final URL in case of redirect.
> > 
> > Good point. We should probably check the policy for both URLs (actually all URLs if it's a redirect chain, but I guess that's not yet possible).
> > 
> > Adam, what do you think?
> 
> I would go with the final URL.  This is also an important case to test.  Basically, the question boils down to whether you want to control which sorts of images are displayed or whether you want to control which sorts of network requests the user agent makes.  In this case, it seems clear we're talking about which sorts of images are displayed, so, like canvas taint, we want to use the final URL.

I think the first URL is as import, as we can here still avoid any network traffic. If the user uses this option to save network traffic, we should also check the policy before starting the request at all

[Adam Barth]

Comment on attachment 107373 [details]
Patch

Ok.  The safest thing is to put the check in CachedResourceLoader::canRequest, which gets called for the first and final URLs.  It's supposed to get called for each redirect too, but I think that doesn't work quite right in 100% of cases.

Created attachment 107643 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 107643 [details]
Patch

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

> Source/WebCore/loader/FrameLoaderClient.h:305
> +        virtual bool allowImage(bool enabledPerSettings, const KURL& imageURL) { return enabledPerSettings; }

Seems like this will break some builds (unused argument).

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:295
> +        if (Frame* f = frame()) {

Please don't abbr.

[Adam Barth]

Comment on attachment 107643 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:295
>> +        if (Frame* f = frame()) {
> 
> Please don't abbr.

We do occasionally abbreviate Frame as f if the current object has a method called frame() to avoid shadowing.  You can see some other examples of that in this file.

[Adam Barth]

Comment on attachment 107643 [details]
Patch

Once we have a test for the redirect case (and the build fix), this is probably ready to go.

Created attachment 107682 [details]
Patch

[Alexey Proskuryakov]

> We do occasionally abbreviate Frame as f if the current object has a method called frame() to avoid shadowing.  You can see some other examples of that in this file.

Yes, it's visible even in patch context. I agree with Darin when he tells to not copy such style issues to new code even if they are present elsewhere in old code.

[Adam Barth]

> Yes, it's visible even in patch context. I agree with Darin when he tells to not copy such style issues to new code even if they are present elsewhere in old code.

Is there a better way to handle this situation?  Shadowing the frame() member function seems suboptimal.  I guess we could use a name other than "f" and other than "frame".

(In reply to comment #23)
> > Yes, it's visible even in patch context. I agree with Darin when he tells to not copy such style issues to new code even if they are present elsewhere in old code.
> 
> Is there a better way to handle this situation?  Shadowing the frame() member function seems suboptimal.  I guess we could use a name other than "f" and other than "frame".

I could just invoke frame() everytime. I'd expect the compiler to optimize this away

Created attachment 107719 [details]
Patch

[Adam Barth]

Comment on attachment 107719 [details]
Patch

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

> Source/WebKit/chromium/public/WebPermissionClient.h:52
> +    virtual bool allowImage(WebFrame* frame, bool enabledPerSettings, const WebURL& imageURL) { return allowImages(frame, enabledPerSettings); }
> +    // FIXME: remove once chromium side has landed.
>      virtual bool allowImages(WebFrame*, bool enabledPerSettings) { return enabledPerSettings; }

Technically we need fishd to review this part of the change, but I think this case is ok.

[Adam Barth]

@fishd: There's a tiny WebKit API change in this patch, which you might want to look at.

[Darin Fisher (:fishd, Google)]

Comment on attachment 107719 [details]
Patch

API changes LGTM

[WebKit Review Bot]

Comment on attachment 107719 [details]
Patch

Clearing flags on attachment: 107719

Committed r95369: <http://trac.webkit.org/changeset/95369>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.