Bug 68071

Summary: Rename FrameLoaderClient::allowImages to FrameLoaderClient::allowImage and include the image URL as parameter
Product: WebKit Reporter: jochen
Component: New BugsAssignee: jochen
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, fishd, marja, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description jochen 2011-09-14 02:39:23 PDT
Include the image URL in the call to FrameLoaderClient::allowImages
Comment 1 jochen 2011-09-14 02:41:03 PDT
Created attachment 107312 [details]
Patch
Comment 2 WebKit Review Bot 2011-09-14 04:13:00 PDT
Comment on attachment 107312 [details]
Patch

Attachment 107312 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9661453
Comment 3 Alexey Proskuryakov 2011-09-14 08:52:37 PDT
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.
Comment 4 jochen 2011-09-14 11:50:38 PDT
(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.
Comment 5 Adam Barth 2011-09-14 11:51:18 PDT
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.
Comment 6 Alexey Proskuryakov 2011-09-14 12:08:48 PDT
> 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.
Comment 7 Adam Barth 2011-09-14 12:11:46 PDT
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.
Comment 8 jochen 2011-09-14 12:27:35 PDT
Created attachment 107373 [details]
Patch
Comment 9 jochen 2011-09-14 12:28:38 PDT
Comment on attachment 107373 [details]
Patch

I've changed allowImages to allowImage and added more information to the changelog entry in WebCore/
Comment 10 Adam Barth 2011-09-14 12:34:10 PDT
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.
Comment 11 jochen 2011-09-14 12:40:30 PDT
(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?
Comment 12 Alexey Proskuryakov 2011-09-14 12:43:17 PDT
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.
Comment 13 jochen 2011-09-14 12:46:09 PDT
(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?
Comment 14 Adam Barth 2011-09-14 12:57:21 PDT
> 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.
Comment 15 jochen 2011-09-15 09:13:40 PDT
(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
Comment 16 Adam Barth 2011-09-15 11:34:37 PDT
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.
Comment 17 jochen 2011-09-16 07:05:56 PDT
Created attachment 107643 [details]
Patch
Comment 18 Alexey Proskuryakov 2011-09-16 08:45:13 PDT
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.
Comment 19 Adam Barth 2011-09-16 10:36:08 PDT
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.
Comment 20 Adam Barth 2011-09-16 10:38:38 PDT
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.
Comment 21 jochen 2011-09-16 10:39:08 PDT
Created attachment 107682 [details]
Patch
Comment 22 Alexey Proskuryakov 2011-09-16 10:54:30 PDT
> 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.
Comment 23 Adam Barth 2011-09-16 11:06:27 PDT
> 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".
Comment 24 jochen 2011-09-16 11:51:24 PDT
(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
Comment 25 jochen 2011-09-16 13:47:25 PDT
Created attachment 107719 [details]
Patch
Comment 26 Adam Barth 2011-09-16 13:55:38 PDT
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.
Comment 27 Adam Barth 2011-09-16 13:56:14 PDT
@fishd: There's a tiny WebKit API change in this patch, which you might want to look at.
Comment 28 Darin Fisher (:fishd, Google) 2011-09-16 14:55:44 PDT
Comment on attachment 107719 [details]
Patch

API changes LGTM
Comment 29 WebKit Review Bot 2011-09-16 22:51:17 PDT
Comment on attachment 107719 [details]
Patch

Clearing flags on attachment: 107719

Committed r95369: <http://trac.webkit.org/changeset/95369>
Comment 30 WebKit Review Bot 2011-09-16 22:51:24 PDT
All reviewed patches have been landed.  Closing bug.