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

[Sayak Banerjee]

Summary: Firefox does not display images even on right clicking on the blank area for the image and clicking "Show Image" when automatic displaying of images is disabled from FF settings.

Steps to reproduce:
  1. Disable automatic displaying of images. Goto Edit->Preferences->Content and uncheck the "Load images automatically" checkbox.
  2. Now open a webpage, say for example, your Launchpad profile page (https://launchpad.net/~usrname)
  3. As the page is displayed, right click on the blank area where your profile mugshot is supposed to be displayed (mugshot: the 192x192 profile picture).
  4. Select the "Show Image" option from the menu.

Expected Result:
The mugshot should be loaded and displayed.

Actual Result:
Nothing happens. The image is still not displayed. So there remains no way to view images on the page other than clicking on "View Image" and opening viewing the image only.

Problems Faced:
Some users disable auto loading of images due to slow internet connections. So when I visit a website which need a visual verification (captcha), I have to select "Copy image location" from right click menu, open it in a new tab and view it there. It would be rather more desirable if the image is displayed on selecting "Show Image" from the menu.

[Jonas-sicking (jonas-sicking)]

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

[Bzbarsky (bzbarsky)]

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

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

[Ehsan Akhgari (ehsan)]

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

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

[Ehsan Akhgari (ehsan)]

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?

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

[Ehsan Akhgari (ehsan)]

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

[Ehsan Akhgari (ehsan)]

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.

[Reed Loden (reed)]

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

[Bzbarsky (bzbarsky)]

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

[Ehsan Akhgari (ehsan)]

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

[Ehsan Akhgari (ehsan)]

Gavin: ping...

[Egor-pelevin (egor-pelevin)]

Any chance to get this in Fx 3?

[Ehsan Akhgari (ehsan)]

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.

[Beltzner (beltzner)]

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

[Ehsan Akhgari (ehsan)]

Gavin: ping...

[Crazy-eye-bugzilla (crazy-eye-bugzilla)]

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.

[Gavin Sharp (gavin-sharp)]

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.

[Egor-pelevin (egor-pelevin)]

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

[Rory (roryk8-deactivatedaccount)]

any news on this?

[Ivan Ivanov (komarik)]

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

[Ehsan Akhgari (ehsan)]

Gavin: ping...

[Rory (roryk8-deactivatedaccount)]

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

[Michael Vincent van Rantwijk (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.

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

[Ehsan Akhgari (ehsan)]

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

[Crazy-eye-bugzilla (crazy-eye-bugzilla)]

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.

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

[Michael Vincent van Rantwijk (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?

[Sayak Banerjee (sayakb-deactivatedaccount)]

Summary: Firefox does not display images even on right clicking on the blank area for the image and clicking "Show Image" when automatic displaying of images is disabled from FF settings.

Steps to reproduce:
  1. Disable automatic displaying of images. Goto Edit->Preferences->Content and uncheck the "Load images automatically" checkbox.
  2. Now open a webpage, say for example, your Launchpad profile page (https://launchpad.net/~usrname)
  3. As the page is displayed, right click on the blank area where your profile mugshot is supposed to be displayed (mugshot: the 192x192 profile picture).
  4. Select the "Show Image" option from the menu.

Expected Result:
The mugshot should be loaded and displayed.

Actual Result:
Nothing happens. The image is still not displayed. So there remains no way to view images on the page other than clicking on "View Image" and opening viewing the image only.

Problems Faced:
Some users disable auto loading of images due to slow internet connections. So when I visit a website which need a visual verification (captcha), I have to select "Copy image location" from right click menu, open it in a new tab and view it there. It would be rather more desirable if the image is displayed on selecting "Show Image" from the menu.

[Xtc4uall (xtc4uall)]

so does this need a new patch for sure?

[Gavin Sharp (gavin-sharp)]

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

[Alexander Sack (asac)]

happens with latest upstream trunk build too

[Alexander Sack (asac)]

now that i think about it, how did this work in firefox 2? i am a bit unsure how exactly the semantics is supposed to be.

[Alexander Sack (asac)]

found upstream bug https://bugzilla.mozilla.org/show_bug.cgi?id=457170 .. lets continue discussion there.

[Ehsan Akhgari (ehsan)]

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

[Hskupin (hskupin)]

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

[Hskupin (hskupin)]

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

[Highmind63 (highmind63)]

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

[Highmind63 (highmind63)]

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?

[Gavin Sharp (gavin-sharp)]

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

[Bzbarsky (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"?

[Gavin Sharp (gavin-sharp)]

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.

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

[Gavin Sharp (gavin-sharp)]

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

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

[Gavin Sharp (gavin-sharp)]

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

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

[Trev-moz (trev-moz)]

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.

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

[Highmind63 (highmind63)]

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

[Highmind63 (highmind63)]

Created an attachment (id=382214)
even better

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

[Bzbarsky (bzbarsky)]

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

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

[Highmind63 (highmind63)]

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

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

[Highmind63 (highmind63)]

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

[Reed Loden (reed)]

(From update of attachment 382357)
>+ * @param aBypassPolicy whether to force the image load even if the content
>+ * policy rejects the load. Works only from priveleged scripts.

s/priveleged/privileged/

[Highmind63 (highmind63)]

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

[Highmind63 (highmind63)]

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

[Highmind63 (highmind63)]

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.

[Bzbarsky (bzbarsky)]

See comment 46?

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

imageBlockingStatus?

[Highmind63 (highmind63)]

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

[Highmind63 (highmind63)]

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

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

[Highmind63 (highmind63)]

Created an attachment (id=382874)
irc chat with gavin

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

[Highmind63 (highmind63)]

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.

[Highmind63 (highmind63)]

Created an attachment (id=383869)
WIP

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

[Highmind63 (highmind63)]

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.

[Highmind63 (highmind63)]

(From update of attachment 383869)
Found the issue, fix coming hopefully later today.

[Highmind63 (highmind63)]

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

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

[Highmind63 (highmind63)]

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

[Highmind63 (highmind63)]

Created an attachment (id=384799)
patch

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

[Highmind63 (highmind63)]

(From update of attachment 384799)
r?'ing gavin for the browser/ parts.

[Highmind63 (highmind63)]

Created an attachment (id=384802)
Fix some uri stuff and add nsIContentBlocker

[Bzbarsky (bzbarsky)]

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

[Highmind63 (highmind63)]

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

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

[Highmind63 (highmind63)]

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

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

[Highmind63 (highmind63)]

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.

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

[Highmind63 (highmind63)]

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

[Highmind63 (highmind63)]

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.

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

[Highmind63 (highmind63)]

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

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

[Highmind63 (highmind63)]

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

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

[Highmind63 (highmind63)]

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

[Highmind63 (highmind63)]

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

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

[Highmind63 (highmind63)]

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

[Highmind63 (highmind63)]

Filed bug 502486 on the permission manager thing.

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

[Highmind63 (highmind63)]

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

[Bzbarsky (bzbarsky)]

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

[Highmind63 (highmind63)]

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.

[Highmind63 (highmind63)]

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

[Ehsan Akhgari (ehsan)]

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

[Hskupin (hskupin)]

See https://<email address hidden>/

[Highmind63 (highmind63)]

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

[Bzbarsky (bzbarsky)]

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

[Dwitte (dwitte)]

Will get to it soon.

[Dwitte (dwitte)]

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?

[Bzbarsky (bzbarsky)]

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

[Highmind63 (highmind63)]

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.

[Bzbarsky (bzbarsky)]

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

[Highmind63 (highmind63)]

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

[Highmind63 (highmind63)]

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?

[Bzbarsky (bzbarsky)]

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

Just for content blocker? How, exactly?

[Highmind63 (highmind63)]

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.

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

[Highmind63 (highmind63)]

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

[Highmind63 (highmind63)]

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

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

[Highmind63 (highmind63)]

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

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

[Dwitte (dwitte)]

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

[Highmind63 (highmind63)]

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

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

[Highmind63 (highmind63)]

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

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

[Highmind63 (highmind63)]

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

[Highmind63 (highmind63)]

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

[Highmind63 (highmind63)]

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.

[Highmind63 (highmind63)]

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

[Mounir-lamouri (mounir-lamouri)]

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

[Matti-mversen (matti-mversen)]

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

[Highmind63 (highmind63)]

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

[Highmind63 (highmind63)]

Comment on attachment 556604
WIP 1 - New Approach

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

Thanks.

[Bzbarsky (bzbarsky)]

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

[Highmind63 (highmind63)]

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

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

[Highmind63 (highmind63)]

Created attachment 556653
WIP 2

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

[Benjamin Smedberg (Mozilla) [:bs] (benjamin-smedbergs)]

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

[Ehsan-mozilla (ehsan-mozilla)]

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.

[Bzbarsky (bzbarsky)]

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

[Highmind63 (highmind63)]

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

[Ehsan-mozilla (ehsan-mozilla)]

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

[Sgautherie-bz (sgautherie-bz)]

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?

[Highmind63 (highmind63)]

Not really working on this anymore.

[Release-mgmt-account-bot (release-mgmt-account-bot)]

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

[Autonag-nomail-bot (autonag-nomail-bot)]

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.