417545 - Ignore image blocking status for the "Reload Image" menu item

Status: NEW

(Firefox :: General, defect)

Description

[(no longer active)]

Attachment: Patch (v1)

Spinoff from bug 218142 comment 59.

We need to ignore the image blocking status when the user right-clicks on an unloaded image and selects Show Image.

Rationale:
(In reply to bug 218142 comment #56)
> This doesn't work (nothing happens, no error msgs) if "Load images
> automatically" is disabled in Options->Content, if I add the hack from the 2006
> patch it does work.
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008021304
> Minefield/3.0b4pre

My first reaction was something along the lines of "we must respect user prefs
if they don't want to load an image, but we present this option in the UI as
"Load images _automatically_".  So, a logical use case would be for a user to
disable this pref, and then use the new Show Image menu item on image
placeholders to individually load the ones she may be interested in.  This
actually looks like a valid point to me, so I'll provide a followup patch soon
based on bug 218142 comment 26.

Here is a patch to implement this.
This patch adds the checkContentPolicy attribute to the nsIImageLoadingContent
interface, and skips the content policy check for the image blocking status if
it's set to false in nsImageLoadingContent::LoadImage().  This is implemented
in nsContentUtils::CanLoadImage().

Requesting review and super-review on the content/ part from sicking, and
review on the browser/ part from gavin.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 303318 [details] [diff] [review]
Patch (v1)

Overriding *all* content policies kind of scares me. This will include much more than just plain image blocking. It'll circumvent things like same-origin policies (iirc we have content policies that enfoce same-origin for images), in the future likely things like don't-load-unencrypted-resources-from-encrypted-pages.

Could you instead have the specific content policies check the new property? (and call it something like userInitiatedLoad or some such)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 303318 [details] [diff] [review]
Patch (v1)

Yeah, what sicking said.  Skipping all content policies will break a number of extensions, I would think, and definitely cause issues if the wrong sort of node (e.g. <img> from a data document) ends up in this code...

Then again, there's the question of what this UI is supposed to do.  Show the image come hell or high water, or just undo the effects of our built-in image blocking?  The naming makes it sound like the former, which I think is a bad idea...  Do we show this option no matter what, or only if the image was blocked by our code?

Comment 3

[Boris Zbarsky [:bzbarsky]]

One other thing.  Last I checked (perhaps our new wrappers make this better, but I doubt it), that boolean property would be exposed to page script once chrome QIs the image to nsIImageLoadingContent.  So as written, the moment you right-click on the image, the page can force it to be shown.  Is that desirable?

Comment 4

[(no longer active)]

Comment on attachment 303318 [details] [diff] [review]
Patch (v1)

Thanks for the reviews.  I'll post a patch which only disables the image blocking status check shortly.

(In reply to comment #3)
> One other thing.  Last I checked (perhaps our new wrappers make this better,
> but I doubt it), that boolean property would be exposed to page script once
> chrome QIs the image to nsIImageLoadingContent.  So as written, the moment you
> right-click on the image, the page can force it to be shown.  Is that
> desirable?

Do you mean the non-privileged code running inside the page?  Wouldn't such a thing affect the rest of the attributes/methods of nsIImageLoadingContent?  What can I do to solve it?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Do you mean the non-privileged code running inside the page? 

Yes.

> Wouldn't such a thing affect the rest of the attributes/methods of
> nsIImageLoadingContent? 

Yes, but we've been careful to make sure all the existing stuff exposed on that interface is ok.

> What can I do to solve it?

Don't add anything to this interface that you don't want content code calling...  

Perhaps the right approach is to frob the permission manager, not the image node?

Comment 6

[(no longer active)]

Attachment: Patch (v2)

I have another patch which limits this only to the image blocking status check in the permission manager.  bz: do you think this new userInitiatedLoad attribute is safe with this patch?

Comment 7

[Boris Zbarsky [:bzbarsky]]

It has the same issue as before: the moment the user right-clicks on the image the page can force it to be shown...

Can we just add an exception for that one image in the content blocker?

Comment 8

[(no longer active)]

Comment on attachment 303670 [details] [diff] [review]
Patch (v2)

(In reply to comment #7)
> It has the same issue as before: the moment the user right-clicks on the image
> the page can force it to be shown...
> 
> Can we just add an exception for that one image in the content blocker?

OK, I'll investigate this further.

Comment 9

[(no longer active)]

Attachment: Patch (v3)

This patch handles all this in the context menu code.  It checks both permissions.default.image and the permission for the host the image is coming from.  If any of them would cause the image not to load, the code flips it and then restores the original value when the nsIImageLoadingContent::forceReload() call returns.

This should address bz's concerns.  Off to gavin for review.

Comment 10

[Reed Loden [:reed]]

Comment on attachment 303675 [details] [diff] [review]
Patch (v3)

I'd prefer bz looked at it one more time to confirm since he's been reviewing this.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 303675 [details] [diff] [review]
Patch (v3)

Seems OK if the ALLOW_ACTION would override a "deny" default.  I'd make the magic constant "1" a named constant of some sort, though.

To be honest, I was more thinking that you'd tell the permission manager that it should return "allow" for this specific image object, but if this works in all the various situations (cross-site images, etc, etc), then it's probably fine.

Comment 12

[(no longer active)]

(In reply to comment #11)
> (From update of attachment 303675 [details] [diff] [review])
> Seems OK if the ALLOW_ACTION would override a "deny" default.

It does.  The default prefs are only looked up if a permission entry is not found for the domain.

> I'd make the
> magic constant "1" a named constant of some sort, though.

It can be ALLOW_ACTION actually.  But this pref accepts a value of 3 as well (to allow same-site images), which makes it not map exactly on nsIPermissionManager constants.  I can change this if gavin wants it as well, anyway.

> To be honest, I was more thinking that you'd tell the permission manager that
> it should return "allow" for this specific image object, but if this works in
> all the various situations (cross-site images, etc, etc), then it's probably
> fine.

It does handle all the situations...

Comment 13

[(no longer active)]

Gavin: ping...

Comment 14

[Egor Pelevin]

Any chance to get this in Fx 3?

Comment 15

[(no longer active)]

Let's get this on the radar.  Without this bug, the functionality added in bug 218142 does not always work as the user expects.  See comment 0.  The bug has a patch with bzbarsky's sr+, waiting for review.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Don't think this blocks, but also think we have an sr'd patch ...

Comment 17

[(no longer active)]

Gavin: ping...

Comment 18

[Andrey Lokot]

Without this patch "Show Image" menu is confusing as it is shown even when "Load images automatically" is off but has no effect in this case. It's RC1 now and this  haven't been checked in yet. 

Comment 19

[:Gavin Sharp [email: [e-mail]]]

We're not going to take a non-blocker for RC2. I'll get to the review soon and it can land for 3.1.

Comment 20

[Egor Pelevin]

We've already got a couple of topics on this issue on forums.mozilla-russia.org...

Comment 21

[Sk]

any news on this?

Comment 22

[Danny]

Any chance to get this fixed in upcoming Firefox 3.1 release?

Comment 23

[(no longer active)]

Gavin: ping...

Comment 24

[Sk]

Gavin: another ping... 
Would be great to implement in 3.1

Comment 25

[mv_van_rantwijk]

I would be rather surprised when this patch goes in, and I'mm sorry but your patch is a workaround only, not a real solution.

Changing users preferences?  To me that is really bad thing to do, because what happens when I'm in tab A and right click to show this blocked image, while other tabs B and C are still loading?  Wouldn't that load images that I don't like to see in the first place, just because you changed the pref for it?

Also, what about locked preferences in environments where the end-user can't change/add preferences?  This would mean that your patch won't work, and thus should not be accepted.

What we need is a changed IDL, adding a new function that skips the PM check for any type of content, but hey I'm just one of the add-on developer working on the very first content blocker for Mozilla.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> because what happens when I'm in tab A and right click to show this blocked
> image, while other tabs B and C are still loading

There's no problem in this scenario.

> Also, what about locked preferences in environments where the end-user can't
> change/add preferences? 

This, on the other hand is a serious problem.  I agree that the patch to this bug should address this use case.

Comment 27

[(no longer active)]

(In reply to comment #26)
> > Also, what about locked preferences in environments where the end-user can't
> > change/add preferences? 
> 
> This, on the other hand is a serious problem.  I agree that the patch to this
> bug should address this use case.

I'm not sure what mv_van was pointing out here...

Comment 28

[Andrey Lokot]

Also this patch doesn't solve another problem. Both current behavior and the one in the patch reload only that particular image but IE, for example, (re)loads all images with the same URL (emoticons for instance).

This is not a problem when you reload broken image, but it's a bit confusing if the patch will be applied. However, I'm sure someone will fill enhancement bug if current implementation will go.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> I'm not sure what mv_van was pointing out here...

The fact that just because you set a pref value doesn't mean a get will get that value.  If the pref is locked it'll continue to get the default value.

Comment 28 doesn't seem to have much to do with this bug, since we're not realoading anything: we're just loading the image standalone.

Comment 30

[mv_van_rantwijk]

I just thought about something, but I haven't had a chance to check it due to college starting in a few minutes, but doesn't the media tab in the page info bypass permissions somehow?

Comment 31

[(mostly gone) XtC4UaLL [:xtc4uall]]

so does this need a new patch for sure?

Comment 33

[(no longer active)]

Upating to reality: I probably won't have time to look into this any time soon, maybe someone else wants to step up.  Natch, maybe?  :-)

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This menu item is now called "Reload Image". Updating summary. Aren't we be able to test this automatically?

Comment 37

[Nochum Sossonko [:Natch]]

I'll try to get to this sometime soon.

My current proposal is to add some mechanism to the image loader that can override the pref, is that what would be desirable? Otherwise, any other suggestions?

Comment 38

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #37)
> My current proposal is to add some mechanism to the image loader that can
> override the pref, is that what would be desirable?

That sounds like the ideal solution, yeah. The tricky part is that I think what you really want to change the return value of nsContentBlocker::TestPermission 
specifically, when called from nsImageLoadingContent::LoadImage (via nsContentUtils::CanLoadImage->NS_CheckContentLoadPolicy->nsContentPolicy::ShouldLoad etc.), and the amount of indirection makes that difficult.

A simple approach might be to create a new interface to nsContentBlocker that allows you to tell it to return nsIContentPolicy::ACCEPT from TestPermission for a given URI (e.g. contentBlocker->IgnorePolicy(aURI, ACCEPT)), and then use that from either the browser.js code that calls forceReload or forceReload itself (perhaps controlled by a new optional argument?). You'd need a way to tell it to continue enforcing the policy afterward as well.

bzbarsky, what do you think of that idea?

Comment 39

[Boris Zbarsky [:bzbarsky]]

Hmm.  We could add arguments to ForceReload/LoadImage/CanLoadImage to ignore the content policy response, of course.  We'd need to do this for all content policies across the board, though.

Here's a question.  If the image load was prevented by an extension content policy, not nsContentBlocker, should that be ignored for purposes of "Show Image"?

Comment 40

[:Gavin Sharp [email: [e-mail]]]

I was assuming that we'd only veto content policy implementations we know about, hence the nsContentBlocker-specific solution.

Perhaps we could allow third-party content policies to opt-in to the override behavior by implementing this new interface, and have a helper similar to NS_CheckContentLoadPolicy that enumerates content policies, attempts to QI them,  and calls IgnorePolicy. That's probably over engineered, though... I think a nsContentBlocker-specific solution would cover the majority case pretty well.

Comment 41

[Boris Zbarsky [:bzbarsky]]

The thing is, content policy has a built-in notion of different sorts of blocking...  You can block a server, or a request, or a type, and we handle those a bit differently in terms of whether we, e.g., show alt text.  It would be ideal if we allowed content policies to just opt into the "right" behavior here, whatever that is.

Wladimir, do you have an thoughts on this?

Comment 42

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #41)
> The thing is, content policy has a built-in notion of different sorts of
> blocking...  You can block a server, or a request, or a type, and we handle
> those a bit differently in terms of whether we, e.g., show alt text.

I'm not sure how that's relevant to this proposal. All I'm suggesting is to allow overriding to always-ACCEPT for a given URI. I can't think of a use case that involves overriding a policy to change the rejection type.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Sure, but the question is whether the override should apply to the particular URI for nsContentBlocker or to the particular URI for any content policy returning the appropriate rejection type.

Comment 44

[:Gavin Sharp [email: [e-mail]]]

I covered that in the second paragraph of coment 40, no?

Comment 45

[Boris Zbarsky [:bzbarsky]]

Sort of.  If the ignoring were in the CanLoadImage code, not in nsContentBlocker, there wouldn't be as much complexity as you describe, I think...

Comment 46

[Wladimir Palant]

Yes, allowing other content policies to opt-in would be good. I have an unfinished implementation of the very same feature for Adblock Plus (with the difference that reloading shouldn't be limited to images) which I would like to pick up again soon. As of now, I tell my own content policy to allow the request - if this can be generalized, all the better.

Boris, if I understand your proposal correctly, the override should apply automatically to all content policies returning a particular rejection reason? Not sure whether this is a good idea - these return values already have some non-trivial implications attached to them. In particular, REJECT_TYPE and REJECT_SERVER have CSS pseudo-classes associated with them which rules them out for Adblock Plus for example. But some other content policy might want these CSS rules to apply - and that's independent of whether it wants to allow being overridden. Another issue is that content policies sometimes have side-effects in addition to blocking content, allowing them to run but simply ignoring the return value might now have the desired effect.

I must say, Gavin's suggestion sounds better - it allows content policies to adjust their behavior in case of a user override. If that's over-engineered, maybe a global flag indicating user override won't be. E.g. make "@mozilla.org/layout/content-policy;1" component implement an interface in addition to nsIContentPolicy that allows getting and setting the current policy override URL. Any content policy interested can get it from there. And - yes, I realize that global variables are bad, but that's probably the cheapest solution.

Comment 47

[Boris Zbarsky [:bzbarsky]]

At that rate, I'd rather just go with the simple solution for now.  Anything else gets me worried about the performance of nsContentBlocker...

Comment 48

[Nochum Sossonko [:Natch]]

Attachment: possible fix?

Before I take this on, can anyone tell me why this won't work? This is a three-line patch that seems to fix the issue for me...

Unless I'm missing something, is this not the simplest solution?

Comment 49

[Nochum Sossonko [:Natch]]

Attachment: even better

Thanks to Gavin, this patch is now a mere 1.5 line change!

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 382214 [details] [diff] [review]
even better

1)  The patch will fire that nice assert in LoadImage() that was added precisely to catch someone doing something daft like this.  Please test patches in debug builds?

2)  This patch makes it so that if chrome script does an innerHTML set on the document the images will load.  If the html string happened to be under the control of the page, this could be pretty bad because:

3)  This patch makes it so that if chrome script happens to be on the stack when we enter this code all security checks (not just the "is this image blocked check", but the CheckLoadURI checks) are bypassed.  I doubt that's desirable.

4)  If we fix things so that <img src="javascript:"> works in a sandbox, this code would run said javascript in the sandbox with the system principal...  Could also be bad.

Comment 51

[Boris Zbarsky [:bzbarsky]]

Oh, and:

5)  That approach doesn't work for embedding situations.

6)  That approach is likely to not work once content is not in the same process
    as chrome.

Comment 52

[Nochum Sossonko [:Natch]]

(In reply to comment #51)
> Oh, and:
> 
> 5)  That approach doesn't work for embedding situations.
> 
> 6)  That approach is likely to not work once content is not in the same
>     process as chrome.

This code will be problematic anyway, nsContextMenu.js was the biggest problem iirc when the thread about the separate process thing came up.


Can I change the signature of nsContentUtils::CanLoadImage? It wouldn't harm any current users. I want to add a boolean parameter (aBypassPolicy) that indicates whether or not to return early without checking the content policy. It will still check with CheckLoadURI, but skip the content policy stuff. I have a patch ready that does just that, would that be ok?

Basically, what the patch does is:

1) forceReload becomes forceReload([optional] in boolean aBypassPolicy). The function then passess the value of aBypassPolicy, if present, to LoadImage after checking IsCallerChrome().
2) LoadImage (the second one with 5 arguments) gets another optional argument (the default is PR_FALSE) of aBypassPolicy.
3) nsContentUtils::CanLoadImage gets the same as LoadImage and returns after the CheckLoadURIWithPrincipal check if aBypassPolicy is true.

Comment 53

[Boris Zbarsky [:bzbarsky]]

> Can I change the signature of nsContentUtils::CanLoadImage?

Yes.

>2) LoadImage (the second one with 5 arguments) gets another optional argument
>(the default is PR_FALSE) of aBypassPolicy.

Please make it non-optional.  Same for nsContentUtils (and adjust callers).

But that said, this is the same approach as in comment 0; see the comments after that for why it was deemed undesirable.

Comment 54

[Nochum Sossonko [:Natch]]

Attachment: patch ver.2

This nixes the changes to nsContentUtils altogether, and allows the image load (if the content policy denies it) only if the REJECT type is TYPE || SERVER.

According to the idl this should be fine, REJECT_REQUEST is for other reasons that we may not want to bypass, but REJECT_{IMAGE|SERVER} are directly connected with permissions granted either to images as a whole or to this particular server.

I tried to write a test for this, but I can't seem to figure out how to detect an image rejection or success on the image itself...

Since bz gave me the r-/sr-, is it ok that I ask you for r/sr or do you want jonas to look at this as well?

Comment 55

[Reed Loden [:reed]]

Comment on attachment 382357 [details] [diff] [review]
patch ver.2

>+   * @param aBypassPolicy whether to force the image load even if the content
>+   *        policy rejects the load. Works only from priveleged scripts.

s/priveleged/privileged/

Comment 56

[Nochum Sossonko [:Natch]]

(In reply to comment #55)
Hah, I do that all the time :( . I'll fix that with the next iteration...

Comment 57

[Nochum Sossonko [:Natch]]

(In reply to comment #54)
> This nixes the changes to nsContentUtils altogether, and allows the image load
> (if the content policy denies it) only if the REJECT type is TYPE || SERVER.

Oh and that should be "REJECT type is IMAGE || SERVER"...

Comment 58

[Nochum Sossonko [:Natch]]

bleh, I'm all fail today :(

Just to be clear: the two types of rejections I singled out to bypass are: REJECT_TYPE and REJECT_SERVER. Both should be safe to bypass, they just represent the users choice not to show images. The problem with REJECT_SERVER is that the images are not displayed at all, so the context menu wouldn't show up for them anyhow. I included it in the bypass mechanism so that in the future (or for other purposes) it will work.

Comment 59

[Boris Zbarsky [:bzbarsky]]

See comment 46?

> I can't seem to figure out how to detect an image rejection or success on the
> image itself

imageBlockingStatus?

Comment 60

[Nochum Sossonko [:Natch]]

(In reply to comment #59)
> See comment 46?

I'm not really sure what to do about that, the change in this bug is a one-time thing, not something that is a "policy". Perhaps removing the policy would be an option, but that is error-prone and was attempted before.

Another solution might be to send a notification (something like "content-policy-override") that extensions can observe, making its' modifications as necessary. Would that be ok with everyone?

> > I can't seem to figure out how to detect an image rejection or success on the
> > image itself
> 
> imageBlockingStatus?

Ok, I'll give that a shot. Thx.

Comment 61

[Nochum Sossonko [:Natch]]

(In reply to comment #60)
> > imageBlockingStatus?
> 
> Ok, I'll give that a shot. Thx.

Err, that wouldn't help me much, I was looking for a way to detect the state of the image, i.e. something like | image.loaded | that would change only because it loaded, not because the policy changed. I guess I ran into the exact problem that was outlined in the previous comment(s).

So, how about an attribute that returns the override status when accessed from chrome? Any other ideas?

The suggestions until now were indicating that the _url_ of the image would be overridden, how is that good? Other pages with the same image should allow the image? Images from the same server would also be included in that, so it has to be specific to this image itself...

Comment 62

[Boris Zbarsky [:bzbarsky]]

I'm not quite sure what comment 61 is asking.

At this point, my big problem is that we have several different viewpoints on what the "right" behavior is.  Can we decide on what we're trying to accomplish in behavior terms?  Once we do that, implementing should be pretty straightforward, I would think.

Comment 63

[Nochum Sossonko [:Natch]]

Attachment: irc chat with gavin

Gonna do what Gavin explained on irc. Hope there aren't any objections.

Comment 64

[Nochum Sossonko [:Natch]]

One question: would it be better if I made the IgnorePolicy function non-scriptable. That way the js call would just be reloadImage(true) and ReloadImage will deal with the IgnorePolicy calls.

IOW leave the patch as is, only adding the the IgnorePolicy function to nsIContentPolicy and reloading based on that response.

Comment 65

[Nochum Sossonko [:Natch]]

Attachment: WIP

This seems to work fine in my tests locally, gonna hook up automatic tests next. Comments, as always, welcome :) .

Comment 66

[Nochum Sossonko [:Natch]]

For some reason this crashes when I add 7000 different entries twice. I'm trying to figure that out, seems like I'm not using the nsTArray right, there also seems to be some amount of memory leaking, I suspect both of these are related. I also need to add a few more NS_ENSURE_ARGs, or at least just null check aURI.

Comment 67

[Nochum Sossonko [:Natch]]

Comment on attachment 383869 [details] [diff] [review]
WIP

Found the issue, fix coming hopefully later today.

Comment 68

[Nochum Sossonko [:Natch]]

So, one of the issues I ran into is that I can't get a dependable domain to ignore, similar to the issue mentioned at: http://mxr.mozilla.org/mozilla-central/source/extensions/permissions/nsContentBlocker.cpp?mark=310-314#296

I'm not sure how to proceed, should I just continue using the host, or find a way of getting just the domain name?

I still can't figure out exactly what I'm doing wrong here, memory seems to be corrupted when I do something like:

var Cc = Components.classes, Ci = Components.interfaces;
var cb = Cc["@mozilla.org/permissions/contentblocker;1"]
           .getService(Ci.nsIContentBlocker);
var uri = Cc["@mozilla.org/network/standard-url;1"]
           .createInstance(Ci.nsIURI);
for (var i = 0; i < 7000; i++) {
  uri.spec = "http://www.google" + i +".com";
  cb.ignorePolicy(uri, 
                  Ci.nsIContentPolicy.TYPE_IMAGE,
                  Ci.nsIContentPolicy.ACCEPT);
}

When I do that twice with this patch I crash with an access violation somewhere in string land. Also, I created a patch to log some of the functions and see what's going on, seems like the entries aren't kept around for some reason after that loop exits. If I put a dot at the end of "google", so that the url becomes "http://www.google." + i + ".com", it seems to work, I'm still not sure why...

Any help here is appreciated. Could it be that the string's copy constructor is copying by reference, then the string gets lost? If so, how come it only crashes/doesnt work if I loop over 7000 uris? It should crash on one ignorePolicy/restorePolicy call afaict...

Comment 69

[Boris Zbarsky [:bzbarsky]]

So you have to run that loop twice to get the crash?  What's the stack to the crash (pastebin or catch me on irc or both)?

> seems like the entries aren't kept around for some reason after that loop exits.

Which entries kept around where?

At first glance, my guess is that your array usage is broken because your struct has no copy constructor.  So the default "copy the bits" constructor is used, so when you AppendElement you get a bit-for-bit copy of the nsAutoString, not a string copy.  If the nsAutoString is using its internal buffer, that sorta works.  If it's using a shared buffer, you copy it but don't addref it.  Then later someone releases it and it goes away... but it's still referenced.

I suggest implementing a sane copy constructor in your struct (and probably using nsString, not nsAutoString).  I'd also specialize DefaultComparator instead of implementing a comparator, I think...  Then you don't need to use the 3-arg version of IndexOf.

Comment 70

[Nochum Sossonko [:Natch]]

Attachment: WIP 2

Ok, this is much better, thanks goes to bz.

Still need to write up some tests, after which this should be ready for review.

Comment 71

[Nochum Sossonko [:Natch]]

Attachment: patch

Patch with a test. The test lives in browser/base/content as it tests the context menu (original intention of this bug).

Comment 72

[Nochum Sossonko [:Natch]]

Comment on attachment 384799 [details] [diff] [review]
patch

r?'ing gavin for the browser/ parts.

Comment 73

[Nochum Sossonko [:Natch]]

Attachment: Fix some uri stuff and add nsIContentBlocker

Comment 74

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 384802 [details] [diff] [review]
Fix some uri stuff and add nsIContentBlocker

>+++ b/browser/base/content/test/image_Reload.html

You have Windows newlines here.  Fix that, please.

>+++ b/extensions/permissions/nsContentBlocker.cpp

>+struct OverridePermissionEntry {
>+  OverridePermissionEntry(nsCString aHost, PRUint32 aContentType, PRInt16 aPolicy) :
>+    mContentType(aContentType),
>+    mPolicy(aPolicy)
>+  {
>+    mHost.Assign(aHost);

Why not just mHost(aHost) in the initializers?

>+  OverridePermissionEntry(const OverridePermissionEntry &entry) :
>+    mContentType(entry.mContentType),
>+    mPolicy(entry.mPolicy)
>+  {
>+    mHost.Assign(entry.mHost);

And mHost(entry.mHost) here?

Also, s/entry/aOther/ or some such?

>+  OverridePermissionEntry() {};

>+static nsTArray<OverridePermissionEntry> mOverridePermissionEntries;

This will show up as a leak.  Why exactly do you need this as a static global instead of a member on nsContentBlocker?

>+static int FindPermission(nsIURI *aURI, PRUint32 aContentType)
....
>+  NS_ENSURE_SUCCESS(rv, -1);
...
>+  return mOverridePermissionEntries.IndexOf(temp);

IndexOf on nsTArray doesn't return |int|.  It returns the relevant index_type. Make this function return nsTArray<OverridePermissionEntry>::index_type (and return NoIndex in the case when GetHost fails).

>+static int iprint = 0;

I have no idea what this is left over from, but presumably it should go away.

>+nsContentBlocker::IgnorePolicy(nsIURI *aURI,
>+  NS_ENSURE_ARG(aContentType);

I'm not sure why you're doing that, since you're not actually making sure it's a "valid" content type.

>+  if (!aNewDecision)
>+    aNewDecision = nsIContentPolicy::ACCEPT;

I don't like this part, but more on that below.

>+  nsCString host;
>+  nsresult rv = aURI->GetHost(host);
>+  NS_ENSURE_SUCCESS(rv, rv);

Did you actually test this?  It looks like with your patch calling reloadImage on a data: image, say, will throw an exception on the ignorePolicy() call and that you don't bother catching that.  I don't know whether you want to change this code or the caller, but one or the other needs changing.  I should note that your IDL doesn't talk about the fact that this method will throw on all sorts of URIs...

>+  if (aReturn && *aReturn == aNewDecision)
>+    return NS_OK; // xxx: Should we override future policies?

Why exactly are we null-testing aReturn here?  It can't be null if the caller is following the contract.

>+  const int loc = FindPermission(aURI, aContentType);
>+  if (loc == -1) {

Again, index_type.  You might want to typedef the right type (e.g. the relevant nsTArray specialization type) for readability.

>+nsContentBlocker::RestorePolicy(nsIURI *aURI,
>+  NS_ENSURE_ARG(aContentType);

Again, not sure what that would even mean.

>+  const int loc = FindPermission(aURI, aContentType);
>+  if (loc != -1)
>+    mOverridePermissionEntries.RemoveElementAt(loc);

This looks bad to me.  In particular if I ignorePolicy, then someone else does ignorePolicy, then they restorePolicy they'll restore both ignores.  If that's not desirable, then we should either throw on nested ignore or properly have an ignore/restore stack or something.

I'm not sure why ignorePolicy and restorePolicy are returning "the current policy", given the above.

>+nsContentBlocker::IsPolicyOverriden(nsIURI *aURI,
>+  NS_ENSURE_ARG(aContentType);

Again, not sure what this means.

>+  if (aReturn)
>+    *aReturn = FindPermission(aURI, aContentType) != -1;

aReturn can't be null here  And again, use the right types and NoIndex.

>@@ -264,6 +358,12 @@ nsContentBlocker::TestPermission(nsIURI 

>+  const int loc = FindPermission(aCurrentURI, aContentType);
>+  if (loc != -1) {

Again, the right types and NoIndex.

>+    *aPermission = mOverridePermissionEntries[loc].mPolicy == BEHAVIOR_ACCEPT ? PR_TRUE :
>+                                                                                PR_FALSE;

No need for the whole "? PR_TRUE : PR_FALSE" part.

Also, shouldn't this compare to nsIContentPolicy::ACCEPT, since that's what the callers will be passing in?

It looks like overriding with REJECT_* is just ignored.  Is that actually on purpose?  If so, the API should reflect that, I would think.

>+++ b/extensions/permissions/nsIContentBlocker.idl

Nix the Windows newlines throughout this file.

>+   * @param aContentLocation The location for which to override content policies.
>+   *                         The policy is ignored for the domain of the site.

This should document what happens if the URI passed in doesn't have a meaningful "domain" (whatever that is; are these policies really per-host and not at least per hostport?)

>+   * @param aContentType     The type of content for which to override the policy.

That's completely useless documentation.  The right way to document something like this is to at least mention that the values are precisely the TYPE_* values of nsIContentPolicy or precisely the types one would pass to nsIContentPolicy::shouldLoad, or something.

>+   * @param aNewDecision     What the new policy should be. If null, the policy
>+   *                         will be ACCEPT.

Similar here; document what the values here can actually be, preferably by reference to the canonical place where they're defined.  @see is your friend.

>+   * @returns The current nsContentBlocker policy that is being overriden.

Probably clearer as "@returns the value content blocker policy had for this location and type before this ignorePolicy call".  That is, don't mention the concrete implementation and make it clear when the value is measured.

Similar comments apply to the other functions on this interface, where applicable.  And discuss the behavior if calls to ignore/restorePolicy are interleaved in various ways, ok?

>+   * @returns The current nsContentBlocker policy for this content/domain.

Here "current" means "after this call completes", right?

Comment 75

[Nochum Sossonko [:Natch]]

(In reply to comment #74)
> >+  OverridePermissionEntry() {};
> 
> >+static nsTArray<OverridePermissionEntry> mOverridePermissionEntries;
> 
> This will show up as a leak.  Why exactly do you need this as a static global
> instead of a member on nsContentBlocker?

I thought that createInstance on nsIContentBlocker might make two instances of this array, but that's probably wrong anyhow. I'll fix that.

> >+nsContentBlocker::IgnorePolicy(nsIURI *aURI,
> >+  NS_ENSURE_ARG(aContentType);
> 
> I'm not sure why you're doing that, since you're not actually making sure it's
> a "valid" content type.

Well, XPConnect throws an error when an invalid _type_ is used, but it won't complain if |null| is used. But I should probably just change this to |if (!aContentType || aContentType < ...)...|, is that fine?

> >+  nsCString host;
> >+  nsresult rv = aURI->GetHost(host);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> 
> Did you actually test this?  It looks like with your patch calling reloadImage
> on a data: image, say, will throw an exception on the ignorePolicy() call and
> that you don't bother catching that.  I don't know whether you want to change
> this code or the caller, but one or the other needs changing.  I should note
> that your IDL doesn't talk about the fact that this method will throw on all
> sorts of URIs...

Yeah, I need to change this. I think this shouldn't throw, just return NS_OK. On the caller side, I'll try to grab the principal uri of the image so that data images embedded in websites will work. Does that work? Will the imgIRequest uri for that image reflect the principal uri on a data image?

> >+  const int loc = FindPermission(aURI, aContentType);
> >+  if (loc != -1)
> >+    mOverridePermissionEntries.RemoveElementAt(loc);
> 
> This looks bad to me.  In particular if I ignorePolicy, then someone else does
> ignorePolicy, then they restorePolicy they'll restore both ignores.  If that's
> not desirable, then we should either throw on nested ignore or properly have an
> ignore/restore stack or something.

Ok, I'll do the ignore/restore stack. I'll also document in the idl that the way to delete all overrides for any given content-type + url is by checking isPolicyOverriden and repeatedly restoring the policy. Something like |while(isPolicyOverriden(url, con))restorePolicy(url, con);|

> I'm not sure why ignorePolicy and restorePolicy are returning "the current
> policy", given the above.

The return values are what I felt was most appropriate, if you want them to just be void return values I can do that as well. ignorePolicy changes a policy so it returns that policy. restorePolicy brings back a policy so it returns that. The caller is presumably aware of what he is overriding/restoring so it makes sense to return a value that he might not be aware of.

> Also, shouldn't this compare to nsIContentPolicy::ACCEPT, since that's what the
> callers will be passing in?

The way TestPermission works is it uses the local defines in the file, which are just redefinitions of the nsIContentPolicy constants. So, BEHAVIOR_ACCEPT is always nsIContentPolicy::ACCEPT

> It looks like overriding with REJECT_* is just ignored.  Is that actually on
> purpose?  If so, the API should reflect that, I would think.

They're not ignored at all, in fact each particular REJECT_* value will have a different effect just as if you would make a real policy change. TestPermission has a very strange way of implementing the 3 policies, and I say 3 because that's how it was setup. It uses the aFromPref out value to indicate whether the rejection is REJECT_TYPE or REJECT_SERVER. So, aPermission will indicate whether or not it is rejected and aFromPref indicates what type of rejection it was. A third-party rejection is rejected the same way as REJECT_SERVER, from TestPermission's pov.

> >+   * @param aContentLocation The location for which to override content policies.
> >+   *                         The policy is ignored for the domain of the site.
> 
> This should document what happens if the URI passed in doesn't have a
> meaningful "domain" (whatever that is; are these policies really per-host and
> not at least per hostport?)

They're per host (by which I probably mean hostport, not sure what GetHost returns, I'll check and document).

> >+   * @returns The current nsContentBlocker policy for this content/domain.
> 
> Here "current" means "after this call completes", right?

Yes.

Comment 76

[Boris Zbarsky [:bzbarsky]]

> but it won't complain if |null| is used

That's because null just becomes the integer "0".

> I should probably just change this to 
> |if (!aContentType || aContentType < ...)...|

Do you want to update this code every time a new type is added to the interface?

> On the caller side, I'll try to grab the principal uri of the image so that
> data images embedded in websites will work. Does that work?

I have no idea what you're asking.  Presumably, the uri to be used here should be the same as the uri that will be used for the "load this image" check, no?  Anything else won't give the right behavior...  If images are turned off period, I'm not sure that you can get any sane behavior here with a per-host whitelist, can you?

> The return values are what I felt was most appropriate

That's fine; it's just that with your impl the things they return are semi-random.  And the idl is very much underdocumented in terms of what the return values mean.

> They're not ignored at all, in fact each particular REJECT_* value will have
> a different effect just as if you would make a real policy change.

On reading this again, I agree they are not ignored, but all values other than BEHAVIOR_ACCEPT are treated identically, so I don't see how the second part of what you say here can be true.

GetHost does NOT include the port, so if this file uses GetHost throughout then it's just looking at the hostname.  Might be worth a separate bug...

Comment 77

[Nochum Sossonko [:Natch]]

(In reply to comment #76)
> I have no idea what you're asking.  Presumably, the uri to be used here should
> be the same as the uri that will be used for the "load this image" check, no? 
> Anything else won't give the right behavior...  If images are turned off
> period, I'm not sure that you can get any sane behavior here with a per-host
> whitelist, can you?

I'm asking, when a data image is passed through to ShouldLoad, what's the url that is passed through?  I hope not the data: url as that would make it possible for websites to bypass the url checking at least for REJECT_SERVER, no?
And this works per-host even if images are turned off completely, because the check for the override comes before the check for the pref.

> > They're not ignored at all, in fact each particular REJECT_* value will have
> > a different effect just as if you would make a real policy change.
> 
> On reading this again, I agree they are not ignored, but all values other than
> BEHAVIOR_ACCEPT are treated identically, so I don't see how the second part of
> what you say here can be true.

I'll recheck this, it seems like you're right, must of overlooked the possibility of someone wanting to fake REJECT_TYPE.

I'll try to get some more context in my patch, I seem to have a very small default for my mq patches.

Comment 78

[Boris Zbarsky [:bzbarsky]]

> I'm asking, when a data image is passed through to ShouldLoad, what's the url
> that is passed through? 

The @src attribute of the image.  So the data: URI.

> possible for websites to bypass the url checking at least for REJECT_SERVER,
> no?

That's the content policy's business.  If it wants to be checking the source of the load instead of the destination, it can do so, of course.

> And this works per-host even if images are turned off completely

Not for images with no host (as above), which you can't whitelist but which the global "images are turned off" pref catches.

Comment 79

[Nochum Sossonko [:Natch]]

Even with the global images turned off data: images get through, in fact I can't see how they would make it to ShouldLoad without a uri, and LoadImage from nsImageLoadingContent.cpp only tries to get the imgIRequest url. Filed that as bug 501455 with a testcase.

Comment 80

[Boris Zbarsky [:bzbarsky]]

> in fact I can't see how they would make it to ShouldLoad without a uri

They have a URI.  The data: uri.  I can assure you that ShouldLoad is called for them.

In any case, all I care about here is that the behavior of the new code _exactly_ match the behavior of other content blocker code.  If ShouldLoad only handles some URIs (as now), this code should also only handle that exact set of URIs.

Comment 81

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 2, 10 lines of context

Ok, this addresses all of the review comments. I still have one problem that I'm mentioning here because I don't know of a way to fix it, if it is indeed fixable at all. The content policy code somehow manages to get a real domain out of the url's passed to Add. I know that because if you block an image, say, to www.google.com, an image from google.com will also be blocked. I don't know how it's done, while still maintaining international url compatibility (which it does).

TestPermission in nsContentBlocker.cpp has a hack when it check for BEHAVIOR_NOFOREIGN of removing from the second-to-last '.' to the begining and from the last '.' to the end. It clearly states though that it's a hack and won't work for international sites. What can I do about this? Should I do the same hack as TestPermission does? Is there some sort of way that I can reliably grab the domain?

Comment 82

[Nochum Sossonko [:Natch]]

Oh, one more thing, about the error-handling, the caller doesn't provide this context menu item on loaded images (which will be the case when they're data: images) so no need to provide the error handling. file:// and chrome/resource/whatever else is trusted and cannot be blocked anyhow. There's a short-circuit in nsContentBlocker::ShouldLoad that ensures that!

So basically, only http/https/ftp are possible to block.

Comment 83

[Boris Zbarsky [:bzbarsky]]

> so no need to provide the error handling

Why do you assume there will be no other callers?

> There's a short-circuit in nsContentBlocker::ShouldLoad that ensures that!

Yes, and presumably you need to duplicate that here (via code-sharing, not copy-paste).

I have no idea what most of comment 81 is about; maybe gavin does.  We do have an effective tld service that feels like it should do what one wants here, but this code doesn't seem to be using it; I have no idea what behavior is expected/desired/etc here.

You might want to get gavin's review before having me look at this again....

Comment 84

[Nochum Sossonko [:Natch]]

Comment on attachment 386326 [details] [diff] [review]
patch ver. 2, 10 lines of context

What I mean is:

foo.bar.baz and foo2.bar.baz are the same to the content policy in that policies will be enforced for bar.baz in both cases. In this api, if you call ignorePolicy on foo.bar.baz, foo2.bar.baz will not be ignored. I don't know how to reliably get the "bar.baz" out of a url and doing the same thing TestPermission does of culling between the second-to-last and last '.' won't work for foo.bar.co.uk (or any other international suffix).

As to the scheme checking, I can do that, but I just use GetHost, which is pretty much the same thing. I think ShouldLoad should be changed to check for a valid host, as it is probably a bit faster then comparing strings.

Do you want me to do that here?

Comment 85

[Boris Zbarsky [:bzbarsky]]

"content policy" does nothing with hostnames. A particular policy implementation (like nsContentBlocker) might do something.

> doing the same thing TestPermission does of culling between the second-to-last
> and last '.' won't work for foo.bar.co.uk (or any other international suffix).

Yet this is what nsContentBlocker does, right?

> As to the scheme checking, I can do that, but I just use GetHost, which is
> pretty much the same thing.

Uh... how could it possibly be the same thing?

> Do you want me to do that here?

I honestly don't care; I just want the code to be internally consistent.  You might want to not change the existing performance-sensitive codepath as part of a bigger patch, though.

Comment 86

[Nochum Sossonko [:Natch]]

(In reply to comment #85)
> "content policy" does nothing with hostnames. A particular policy
> implementation (like nsContentBlocker) might do something.

I honestly don't know how or where it is done, my testing was litmus-style. I tried blocking an image on google.co.uk and it worked, not sure how though. I don't see any Add or Remove method defined in nsContentBlocker so I'm not sure where that gets implemented. I do see an implementation in nsContentPolicy though...

I think my testing was flawed anyways, it seems that all that is stripped is the www at the beginning of the url. I'll add that to my implementation with the next patch, that should make us compatible. Sorry for the confusion.

> > doing the same thing TestPermission does of culling between the second-to-last
> > and last '.' won't work for foo.bar.co.uk (or any other international suffix).
> 
> Yet this is what nsContentBlocker does, right?

Only for BEHAVIOR_NOFOREIGN so I can't copy that for my use-case. I think I have this figured out anyways, so I'll try to be as compliant as I can.

> > As to the scheme checking, I can do that, but I just use GetHost, which is
> > pretty much the same thing.
> 
> Uh... how could it possibly be the same thing?

I don't know of any other scheme that GetHost works for, is there anything else?

> > Do you want me to do that here?
> 
> I honestly don't care; I just want the code to be internally consistent.  You
> might want to not change the existing performance-sensitive codepath as part of
> a bigger patch, though.

Problem is, I need the host anyways for the entry which is why this was the fastest way for me to bail, ShouldLoad doesn't need the host. The reason I think it's faster anyhow is because ShoudLoad calls GetScheme first, then manually compares the string to http/https/ftp...

Unless there's a case where GetHost doesn't fail on non-{http|https|ftp} I'd like to use that.

Comment 87

[Boris Zbarsky [:bzbarsky]]

> I do see an implementation in nsContentPolicy though...

Implementation of what?

> I'll add that to my implementation with the next patch

Please make sure your patch shares code, not just behavior, so the behaviors can't get out of sync.

> I don't know of any other scheme that GetHost works for, is there anything
> else?

Anything using nsStandardURL.  chrome://, irc://, news://, imap://.  That's off the top of my head; there are probably more we implement internally, and extensions can implement still others.

Comment 88

[Nochum Sossonko [:Natch]]

...and sorry again, I had thought nsContentBlocker dealt with the additions and removals of the content policy. Apparently it uses nsIPermissionManager to deal with that. So the permission manager controls the per-site policies while the pref controls the overall image blocking. nsContentBlocker just implements nsIContentPolicy and provides the methods to check for the policies.

I'll update the patch later today, hopefully.

Comment 89

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 2.1, 10 lines of context

Ok, fixed the inconsistencies and added two inline methods to nsContentBlocker, 1 to check that the scheme is a compatible one, the other strips the www. off of the host.

Let me know if there are any problems with this approach.

Comment 90

[Boris Zbarsky [:bzbarsky]]

>+  if (!aNewDecision)
>+    aNewDecision = nsIContentPolicy::ACCEPT;

Still not sure what this is trying to do, exactly...

>+    return NS_OK; // xxx: Should we override future policies?

Imo, yes.  Especially since it leads to simpler code.

>+  *aReturn = FindPermission(aURI, aContentType) != -1;

NO_INDEX?

> +  inline void CutWWWFromHost(nsCString& host)

I'm confused.  None of the existing code is using this function.  How do you know it does what the existing code does?

>+      printf("%p: init constructor called [mHost: %s, mContentType: %d, mPolicy: %d]\n", this, mHost.get(), mContentType, mPolicy);

Take those out?

> This method only accepts uris with valid hosts.

That doesn't seem to be correct to me.

>+   *                         Corresponds to nsIContePolicy's TYPE_*.

nsIContentPolicy (with the "nt" after "e").

I'm still not happy with the "if null" business.  You can't have a "null" integer.  Just document that behavior is undefined if the value is not a valid ShouldLoad value?

> have a valid host this method does not throw.

How about "does nothing"?

> +   *                         Corresponds to nsIContePolicy's TYPE_*.

Add "nt", as above.

> +   *                         Corresponds to nsIContePolicy's TYPE_*.

And here.

Comment 91

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3

A little intro to this new patch:

The way nsPermissionsManager handles entries is a bit complicated (although not necessarily wrong). Any uri passed to it will get stored by host as is. Then when TestPermission is called, it will check the hash table for that host and all its' super-domains. In practice this means that "http://www.google.com" won't match either "http://google.com" or "http://images.google.com". However, "http://google.com" will match any sub-domain of google.com, including "http://www.google.com" and "http://images.google.com".

This I took from http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#524 where the entry lookup is done (and TestPermission is called with PR_FALSE for aExactMatch). I tried to mimic the code there as close exactly (hence the NS_GetInnermostURI addition), but changed the lookup a bit to account for storage differences (it made more sense for me to modify the temp mHost in place, instead of creating new OverridePermissionEntries for each lookup).

The only thing wrong with that approach (and this probably belongs in a separate bug, which I'll file shortly) is that it doesn't cut off "www" which is not a sub-domain, rather a standard prefix. The intention is that the caller should be able to chose up to which sub-domain it wants to add the permission for, and "www" shouldn't be considered a sub-domain in that case. When the permission manager is fixed, this should be fixed as well.

Everything else in the previous comment was addressed, sorry about the misspellings.

Comment 92

[Nochum Sossonko [:Natch]]

Filed bug 502486 on the permission manager thing.

Comment 93

[Boris Zbarsky [:bzbarsky]]

I really don't like the permission manager code duplication here...  Really really intensely.  Especially because there is no indication in either file that the other needs fixing if that one is changed.

Comment 94

[Nochum Sossonko [:Natch]]

Not really sure of a good alternative, the two methods can't really be merged into one helper method. I can indicate in both files that they are dependent on each other (more accurately, nsContentBlocker is dependent on nsPermissionManager), would that be enough? I can also include a test that compares with entries stored and found in the permission manager vs. nsContentBlocker which will fail if something changes (although it won't be 100%).

Comment 95

[Boris Zbarsky [:bzbarsky]]

I'd really welcome some feedback from whoever owns the permission manager here.

Comment 96

[Nochum Sossonko [:Natch]]

From http://www.mozilla.org/owners.html#cookies-and-permissions that seems to be Dan Witte.

Dan: please see comment 93 - comment 95; this patch is adding a temporary policy override to nsContentBlocker. The attached patch accomplishes this with an in-memory array of policies per type and host. The method of adding and then later testing the host was modeled after nsPermissionManager to be consistent.

Comment 97

[Nochum Sossonko [:Natch]]

If someone could push this to the try-server so that I can see what the perf impact is, I'd greatly appreciate it. I hadn't really considered that yet, but there might be some perf issues that can be ironed out a bit more...

Comment 98

[(no longer active)]

(In reply to comment #97)
> If someone could push this to the try-server so that I can see what the perf
> impact is, I'd greatly appreciate it. I hadn't really considered that yet, but
> there might be some perf issues that can be ironed out a bit more...

I did.  The build ID is imgreload.

Comment 99

[Henrik Skupin [:whimboo][⌚️UTC+1]]

See https://build.mozilla.org/tryserver-builds/[e-mail]-imgreload/

Comment 100

[Nochum Sossonko [:Natch]]

Thank you Ehsan, seems like there isn't any perf problem, at least none that talos could identify.

Comment 101

[Boris Zbarsky [:bzbarsky]]

Just to be clear, my review is waiting on a response to comment 95.

Comment 102

[[e-mail]]

Will get to it soon.

Comment 103

[[e-mail]]

Okay. I've skimmed the bug and the patch. The duplication of functionality, and the overall approach of adding a temporary exception based on host and not an individual load request, trouble me.

I could probably answer this myself if I read all the details in this bug, but bz understands this stuff far better. Could we create a new loadflag "FORCE_LOAD_FROM_USER" or somesuch, stick it on the channel, and then look for that flag in the contentblocker? We could then skip just the permissionmanager check for that one load. This depends on having the right objects available in the right places, of course.

Is this not feasible?

Comment 104

[Boris Zbarsky [:bzbarsky]]

The content blocker doesn't see a channel.  It sees a URI.

Comment 105

[Nochum Sossonko [:Natch]]

Can it be done with the aExtra argument to ShouldLoad? Maybe we can pass in the channel to it and have it take a different path when there's a channel with the above-mentioned flag on it?

I haven't even looked into doing this yet, just trying to get some consensus on the idea.

Comment 106

[Boris Zbarsky [:bzbarsky]]

The caller of ShouldLoad does not have a channel in this case.

Comment 107

[Nochum Sossonko [:Natch]]

Right, I keep on forgetting the content blocker comes in before the request is sent...

Comment 108

[Nochum Sossonko [:Natch]]

bz: would it be possible to ignore on a per element basis? Then the image that we want to ignore on can be passed in and it will last for the lifetime of that image element.

Otherwise, is there anything else that needs to be done to get this going?

Comment 109

[Boris Zbarsky [:bzbarsky]]

> bz: would it be possible to ignore on a per element basis?

Just for content blocker?  How, exactly?

Comment 110

[Nochum Sossonko [:Natch]]

via aRequestingContext which I'm assuming is the element. Then you would call ignorePolicy with an element as the argument instead of the uri. Is that better?

I'm just trying to suggest other ideas so that this can be worked out one way or another. If there's a better option here, please do say.

Comment 111

[Boris Zbarsky [:bzbarsky]]

> via aRequestingContext which I'm assuming is the element.

Ah, for <img> elements that is in fact the case, yes.

I could live with that, I guess, though it presents a certain leak hazard if chrome is careless...

Comment 112

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 4 (different approach)

Ok, so I came up with this idea instead. Since what we really want here is just a method to override nsContentBlocker's policy for any specific image, and creating a whole api that would be element-based would be pretty crufty (the override would only last as long as the element does, so it wouldn't be all that useful), I think this "mime" passing would do better.

Basically, I'm abusing aMimeGuess (which isn't used for images) and sending in a "message" instead to the content policy. nsContentBlocker catches this message and returns ACCEPT for that image. This makes for a leaner and cleaner patch, plus it has the added benefit of allowing other content policies to opt in and override their policy. This should also be less perfy (no iteration, just a simple string comparison).

Let me know if this will do. I'm leaving the other patch unobsoleted, as this is my only other option. Creating a whole api to ignore policies per element seems like a huge waste to me, so if this won't be accepted, my only other bet is the previous approach. Or new suggestions, which I'm not expecting to float in anytime soon...

Comment 113

[Nochum Sossonko [:Natch]]

Comment on attachment 390741 [details] [diff] [review]
patch ver. 4 (different approach)

r? gavin for /browser parts

Comment 114

[Boris Zbarsky [:bzbarsky]]

I'm not happy abusing the MIME guess; that could screw with other content policy implementations.

Plus, this last patch has the problem I mention in comment 3, no?

Comment 115

[Nochum Sossonko [:Natch]]

(In reply to comment #114)
> I'm not happy abusing the MIME guess; that could screw with other content
> policy implementations.

How? It isn't being used for images now, and I'm only sending when we force reload an image. Currently all the callers of CanLoadImage are using EmptyCString() for aMimeGuess, I only changed that when it's called from forceReload(true).

> Plus, this last patch has the problem I mention in comment 3, no?

Well, I'm not able to reproduce that, I've tried the following on google.com:

javascript:document.getElementById('logo').QueryInterface(Components.interfaces.nsIImageLoadingContent).forceReload(true);void 0;

I put that in the urlBar, then right-click the image, then hit enter in the urlBar, and nothing happens (i.e. it gets caught by the IsCallerChrome check in forceReload). Can you explain how that could be used?

One other idea would be to add an api to the permissionManager itself that would allow for temporary permission overrides, but I'm not sure anyone wants to go down that path. Shout out if that's a better solution.

Comment 116

[Boris Zbarsky [:bzbarsky]]

> It isn't being used for images now, and I'm only sending when we force
> reload an image

Why are you assuming that content policies necessarily check the TYPE_* type before the MIME guess?

> it gets caught by the IsCallerChrome check in forceReload

Ah, I'd missed that check.  Given that, I might just barely be ok with this if we have no other options.

What happened to just getting dwitte to ok the patch you'd been working on, or tell you exactly what he wants changed about it?

Comment 117

[[e-mail]]

(In reply to comment #116)
> What happened to just getting dwitte to ok the patch you'd been working on, or
> tell you exactly what he wants changed about it?

Well, I indicated that I thought there almost certainly were technically better solutions in comment 103. From the sound of it you have a similar opinion, though I'm not sure which alternatives are possible and which aren't. I've been out sick this last week but I'll catch you next week and we can briefly talk it over, if you'd like.

Comment 118

[Nochum Sossonko [:Natch]]

(In reply to comment #116)
> Why are you assuming that content policies necessarily check the TYPE_* type
> before the MIME guess?

I'm not. If they're checking the mime guess and are set to do something for
this mime ("policy-user-override"), then I'd have to guess that a) they're
doing the right thing :). Or b) they'll have to update. I still don't
understand what the danger is in this, the mime is for objects only as of now,
and are supposed to be helpful when it's something that can be deciphered. This
is a new notification, why should I suspect that someone will get tripped on
this?

> What happened to just getting dwitte to ok the patch you'd been working on, or
> tell you exactly what he wants changed about it?

Well, comment 103 happened. I thought he pretty much gave us his opinion that
he didn't like the approach of overriding content policies, and his alternative choice wasn't entirely possible. Note: this mime guess override thing is very similar to the approach he suggested.

Either way, I left them both up, and I'm deferring to whomever will make the
final decision on how exactly to tackle this bug. I thought I would share this
alternative approach, being that it was extremely simple to put together and
provided an alternative to what we had, which was deemed unsatisfactory.

Comment 119

[Boris Zbarsky [:bzbarsky]]

> This is a new notification

Why are you assuming that no one else does TYPE_IMAGE content policy requests from an extension?  It's a public API; anyone can do it.

I'm ok with breaking API compat for unfrozen APIs in a good cause, but this doesn't seem like such a cause (and is an abuse of the API no matter how one looks at it).

> Well, comment 103 happened.

I see nothing in that that would prevent a slightly modified version of the patch Dan was looking at (say modified to not copy/paste code and to make exceptions for URIs, not hostnames) from being acceptable to him.

Comment 120

[Nochum Sossonko [:Natch]]

(In reply to comment #119)
> Why are you assuming that no one else does TYPE_IMAGE content policy requests
> from an extension?  It's a public API; anyone can do it.

This doesn't break TYPE_IMAGE policy requests at all, it only breaks requests that are made through nsContentUtil's helper "CanLoadImage". I assumed that was an "internal" helper and that it wasn't being used by extensions, perhaps I'm wrong. The mime guess itself always has to be provided regardless, and CanLoadImage used to do EmptyCString() for it. I was trying _not_ to break any apis, I just wasn't aware that changing CanLoadImage constitutes an api breakage.

> I see nothing in that that would prevent a slightly modified version of the
> patch Dan was looking at (say modified to not copy/paste code and to make
> exceptions for URIs, not hostnames) from being acceptable to him.

Personally, I'd much rather see this version go in as it can be useful for other stuff as well. Especially if it will be extended to all content policy implementations. If the only issue with the patch is that it conforms too closely to the permission manager, I'm more then willing to change it to whatever seems more fit (i.e. full uris, no super-domain checking). I went with this only because it was suggested that we mirror the permanent implementation...

Comment 121

[Boris Zbarsky [:bzbarsky]]

> This doesn't break TYPE_IMAGE policy requests at all, it only breaks requests
> that are made through nsContentUtil's helper "CanLoadImage".

You're passing a bogus value to an API call.  You're trying to justify this by saying that no one passes anything useful for that value right now, so callers don't expect anything useful, so it's OK.  I'm just pointing out that this seems like a baseless assumption based on the data (or lack thereof) presented so far.  I meant the assumption about no one passing anything useful for it.

Comment 122

[Nochum Sossonko [:Natch]]

Comment on attachment 390741 [details] [diff] [review]
patch ver. 4 (different approach)

http://mxr-test.konigsberg.mozilla.org/addons/search?string=shouldLoad&tree=addons

That shows a bunch of addons that have their own content-policies and use aMimeGuess without checking the type. I guess this isn't the best thing then. Maybe aExtra can be used though...

Comment 123

[Nochum Sossonko [:Natch]]

Ok, so what now? Would using aExtra for this be better? Is there any consensus on how to implement ignorePolicy?

Comment 124

[Nochum Sossonko [:Natch]]

gentle ping on this...

Specifically any answers to comment 123? I'd rather not have to workup another alternative based on the aExtra argument if it isn't what the reviewers/module owners want.

Comment 125

[Nochum Sossonko [:Natch]]

Comment on attachment 386894 [details] [diff] [review]
patch ver. 3

Gonna hopefully come up with a better solution based on the aExtra argument. That should also allow addons join in the fun.

Comment 128

[Nochum Sossonko [:Natch]]

Attachment: WIP 1 - New Approach

Here's a new approach, Dan please let me know if this is acceptable and I will forward to others for full review. This places all the "burden" of skipping the policy checking on nsImageLoadingContent::ForceReload

Comment 129

[Nochum Sossonko [:Natch]]

Comment on attachment 556604 [details] [diff] [review]
WIP 1 - New Approach

Jonas, I'd appreciate some feedback on this patch.

Thanks.

Comment 130

[Boris Zbarsky [:bzbarsky]]

Casting from a contact-gotten interface pointer to a concrete class is not ok.

Comment 131

[Nochum Sossonko [:Natch]]

(In reply to Boris Zbarsky (:bz) from comment #130)
> Casting from a contact-gotten interface pointer to a concrete class is not
> ok.

I couldn't figure out a good way of getting to nsContenBlocker. Can you propose something else? I'd hate to have to create a nsIContenBlocker interface.

Aside from that, is including nsContentBlocker.h in nsContentUtils an issue?

Comment 132

[Boris Zbarsky [:bzbarsky]]

> Can you propose something else?

Use the classid?

> Aside from that, is including nsContentBlocker.h in nsContentUtils an issue?

I'd suspect yes, but worth checking with bsmedberg...

Comment 133

[Nochum Sossonko [:Natch]]

Attachment: WIP 2

Hi Benjamin, can you let me know if including "nsContentBlocker.h" in "nsContentUtils.h" is an issue?

Comment 134

[Benjamin Smedberg]

Comment on attachment 556653 [details] [diff] [review]
WIP 2

extensions/permissions is currently ifdef MOZ_PERMISSIONS. I really don't know if that flag works or needs to work (I suspect not).

Comment 135

[(no longer active)]

Couldn't we just suppress checking the blocking status inside nsImageLoadingContent::LoadImage?  This way we wouldn't need to modify the content blocker service at all, we just wouldn't query it if we're doing a force-reload.

Comment 136

[Boris Zbarsky [:bzbarsky]]

There are content policies that implement security checks, last I checked, so we can't just skip calling into content policy code.

Comment 137

[Nochum Sossonko [:Natch]]

(In reply to Ehsan Akhgari [:ehsan] from comment #135)
> Couldn't we just suppress checking the blocking status inside
> nsImageLoadingContent::LoadImage?  This way we wouldn't need to modify the
> content blocker service at all, we just wouldn't query it if we're doing a
> force-reload.

See comments 1 and 2, in tune with bz's response in comment 136.

Is this patch problematic in any way? Also, does anyone know if/when dwitte will be able to look at this?

Comment 138

[(no longer active)]

Try pinging him on IRC?  He may not be watching his bugmail closely...

Comment 139

[Serge Gautherie (:sgautherie)]

Comment on attachment 556653 [details] [diff] [review]
WIP 2

Review of attachment 556653 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/Makefile.in
@@ +189,5 @@
>                   browser_gestureSupport.js \
>                   browser_getshortcutoruri.js \
>                   browser_hide_removing.js \
> +                 browser_imageReload.js \
> +                 image_Reload.html \

These files are missing from this patch, aren't they?

Comment 140

[Nochum Sossonko [:Natch]]

Not really working on this anymore.

Comment 141

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 5 duplicates and 13 votes.
:mossop, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 142

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.