Firefox does not display images when "Show image" is selected if auto display of images is disabled

(From update of attachment 303318)
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)

(From update of attachment 303318)
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?

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?

(From update of attachment 303318)
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?

> 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?

Created an attachment (id=303670)
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?

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?

(From update of attachment 303670)
(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.

Created an attachment (id=303675)
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.

(From update of attachment 303675)
I'd prefer bz looked at it one more time to confirm since he's been reviewing this.

(From update of attachment 303675)
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.

(In reply to comment #11)
> (From update of attachment 303675 [details])
> 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...

Gavin: ping...

Any chance to get this in Fx 3?

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.

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

Gavin: ping...

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.

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.

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

any news on this?

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

Gavin: ping...

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

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.

> 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.

(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...

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.

> 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.

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?

so does this need a new patch for sure?

*** Bug 457170 has been marked as a duplicate of this bug. ***

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? :-)

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

*** Bug 435167 has been marked as a duplicate of this bug. ***

*** Bug 489103 has been marked as a duplicate of this bug. ***

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?

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

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.

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

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

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?

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

Just for content blocker? How, exactly?

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.

> 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...

Created an attachment (id=390741)
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...

(From update of attachment 390741)
r? gavin for /browser parts

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?

(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.

> 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?

(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.

(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.

> 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.

(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...

> 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.

(From update of attachment 390741)
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...

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

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.

(From update of attachment 386894)
Gonna hopefully come up with a better solution based on the aExtra argument. That should also allow addons join in the fun.

*** Bug 626514 has been marked as a duplicate of this bug. ***

*** Bug 681610 has been marked as a duplicate of this bug. ***

Created attachment 556604
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 on attachment 556604
WIP 1 - New Approach

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

Thanks.

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

(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?

> 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...

Created attachment 556653
WIP 2

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

Comment on attachment 556653
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).

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.

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

(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?

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

Comment on attachment 556653
WIP 2

Review of attachment 556653:
-----------------------------------------------------------------

::: 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?

Not really working on this anymore.

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](https://wiki.mozilla.org/Release_Management/autonag#severity_underestimated.py).

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.