Firefox does not display images when "Show image" is selected if auto display of images is disabled
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Mozilla Firefox |
Confirmed
|
Unknown
|
|||
firefox-3.0 (Ubuntu) |
Triaged
|
Medium
|
Unassigned |
Bug Description
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->Preferenc
2. Now open a webpage, say for example, your Launchpad profile page (https:/
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.
In Mozilla Bugzilla #417545, Jonas-sicking (jonas-sicking) wrote : | #1 |
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #2 |
(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?
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #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 nsIImageLoading
In Mozilla Bugzilla #417545, Ehsan Akhgari (ehsan) wrote : | #4 |
(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 nsIImageLoading
> 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 nsIImageLoading
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #5 |
> 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
> nsIImageLoading
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?
In Mozilla Bugzilla #417545, Ehsan Akhgari (ehsan) wrote : | #6 |
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?
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #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?
In Mozilla Bugzilla #417545, Ehsan Akhgari (ehsan) wrote : | #8 |
(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.
In Mozilla Bugzilla #417545, Ehsan Akhgari (ehsan) wrote : | #9 |
Created an attachment (id=303675)
Patch (v3)
This patch handles all this in the context menu code. It checks both permissions.
This should address bz's concerns. Off to gavin for review.
In Mozilla Bugzilla #417545, Reed Loden (reed) wrote : | #10 |
(From update of attachment 303675)
I'd prefer bz looked at it one more time to confirm since he's been reviewing this.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #11 |
(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 Mozilla Bugzilla #417545, Ehsan Akhgari (ehsan) wrote : | #12 |
(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 nsIPermissionMa
> 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...
In Mozilla Bugzilla #417545, Ehsan Akhgari (ehsan) wrote : | #13 |
Gavin: ping...
In Mozilla Bugzilla #417545, Egor-pelevin (egor-pelevin) wrote : | #14 |
Any chance to get this in Fx 3?
In Mozilla Bugzilla #417545, Ehsan Akhgari (ehsan) wrote : | #15 |
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.
In Mozilla Bugzilla #417545, Beltzner (beltzner) wrote : | #16 |
Don't think this blocks, but also think we have an sr'd patch ...
In Mozilla Bugzilla #417545, Ehsan Akhgari (ehsan) wrote : | #17 |
Gavin: ping...
In Mozilla Bugzilla #417545, Crazy-eye-bugzilla (crazy-eye-bugzilla) wrote : | #18 |
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.
In Mozilla Bugzilla #417545, Gavin Sharp (gavin-sharp) wrote : | #19 |
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.
In Mozilla Bugzilla #417545, Egor-pelevin (egor-pelevin) wrote : | #20 |
We've already got a couple of topics on this issue on forums.
In Mozilla Bugzilla #417545, Rory (roryk8-deactivatedaccount) wrote : | #21 |
any news on this?
In Mozilla Bugzilla #417545, Ivan Ivanov (komarik) wrote : | #22 |
Any chance to get this fixed in upcoming Firefox 3.1 release?
In Mozilla Bugzilla #417545, Ehsan Akhgari (ehsan) wrote : | #23 |
Gavin: ping...
In Mozilla Bugzilla #417545, Rory (roryk8-deactivatedaccount) wrote : | #24 |
Gavin: another ping...
Would be great to implement in 3.1
In Mozilla Bugzilla #417545, Michael Vincent van Rantwijk (mv-van-rantwijk) wrote : | #25 |
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.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #26 |
> 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 Mozilla Bugzilla #417545, Ehsan Akhgari (ehsan) wrote : | #27 |
(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...
In Mozilla Bugzilla #417545, Crazy-eye-bugzilla (crazy-eye-bugzilla) wrote : | #28 |
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.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #29 |
> 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.
In Mozilla Bugzilla #417545, Michael Vincent van Rantwijk (mv-van-rantwijk) wrote : | #30 |
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) wrote : | #31 |
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->Preferenc
2. Now open a webpage, say for example, your Launchpad profile page (https:/
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.
In Mozilla Bugzilla #417545, Xtc4uall (xtc4uall) wrote : | #32 |
so does this need a new patch for sure?
In Mozilla Bugzilla #417545, Gavin Sharp (gavin-sharp) wrote : | #33 |
*** Bug 457170 has been marked as a duplicate of this bug. ***
Alexander Sack (asac) wrote : | #34 |
happens with latest upstream trunk build too
Changed in firefox: | |
importance: | Undecided → Medium |
status: | New → Triaged |
Alexander Sack (asac) wrote : | #35 |
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) wrote : | #36 |
found upstream bug https:/
Changed in firefox: | |
importance: | Undecided → Unknown |
status: | New → Unknown |
Changed in firefox: | |
status: | Unknown → In Progress |
In Mozilla Bugzilla #417545, Ehsan Akhgari (ehsan) wrote : | #37 |
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? :-)
Changed in firefox: | |
status: | In Progress → Confirmed |
In Mozilla Bugzilla #417545, Hskupin (hskupin) wrote : | #38 |
This menu item is now called "Reload Image". Updating summary. Aren't we be able to test this automatically?
In Mozilla Bugzilla #417545, Hskupin (hskupin) wrote : | #39 |
*** Bug 435167 has been marked as a duplicate of this bug. ***
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #40 |
*** Bug 489103 has been marked as a duplicate of this bug. ***
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #41 |
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?
In Mozilla Bugzilla #417545, Gavin Sharp (gavin-sharp) wrote : | #42 |
(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 nsContentBlocke
specifically, when called from nsImageLoadingC
A simple approach might be to create a new interface to nsContentBlocker that allows you to tell it to return nsIContentPolic
bzbarsky, what do you think of that idea?
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #43 |
Hmm. We could add arguments to ForceReload/
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"?
In Mozilla Bugzilla #417545, Gavin Sharp (gavin-sharp) wrote : | #44 |
I was assuming that we'd only veto content policy implementations we know about, hence the nsContentBlocke
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_CheckContent
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #45 |
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?
In Mozilla Bugzilla #417545, Gavin Sharp (gavin-sharp) wrote : | #46 |
(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.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #47 |
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.
In Mozilla Bugzilla #417545, Gavin Sharp (gavin-sharp) wrote : | #48 |
I covered that in the second paragraph of coment 40, no?
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #49 |
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...
In Mozilla Bugzilla #417545, Trev-moz (trev-moz) wrote : | #50 |
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.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #51 |
At that rate, I'd rather just go with the simple solution for now. Anything else gets me worried about the performance of nsContentBlocker...
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #52 |
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?
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #53 |
Created an attachment (id=382214)
even better
Thanks to Gavin, this patch is now a mere 1.5 line change!
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #54 |
(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.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #55 |
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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #56 |
(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:
Basically, what the patch does is:
1) forceReload becomes forceReload(
2) LoadImage (the second one with 5 arguments) gets another optional argument (the default is PR_FALSE) of aBypassPolicy.
3) nsContentUtils:
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #57 |
> Can I change the signature of nsContentUtils:
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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #58 |
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_
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?
In Mozilla Bugzilla #417545, Reed Loden (reed) wrote : | #59 |
(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/
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #60 |
(In reply to comment #55)
Hah, I do that all the time :( . I'll fix that with the next iteration...
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #61 |
(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"...
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #62 |
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.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #63 |
See comment 46?
> I can't seem to figure out how to detect an image rejection or success on the
> image itself
imageBlockingSt
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #64 |
(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-
> > I can't seem to figure out how to detect an image rejection or success on the
> > image itself
>
> imageBlockingSt
Ok, I'll give that a shot. Thx.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #65 |
(In reply to comment #60)
> > imageBlockingSt
>
> 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...
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #66 |
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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #67 |
Created an attachment (id=382874)
irc chat with gavin
Gonna do what Gavin explained on irc. Hope there aren't any objections.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #68 |
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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #69 |
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 :) .
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #70 |
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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #71 |
(From update of attachment 383869)
Found the issue, fix coming hopefully later today.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #72 |
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://
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.
var cb = Cc["@mozilla.
var uri = Cc["@mozilla.
for (var i = 0; i < 7000; i++) {
uri.spec = "http://
cb.ignorePoli
}
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://
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/
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #73 |
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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #74 |
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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #75 |
Created an attachment (id=384799)
patch
Patch with a test. The test lives in browser/
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #76 |
(From update of attachment 384799)
r?'ing gavin for the browser/ parts.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #77 |
Created an attachment (id=384802)
Fix some uri stuff and add nsIContentBlocker
Changed in firefox: | |
status: | Confirmed → In Progress |
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #78 |
(From update of attachment 384802)
>+++ b/browser/
You have Windows newlines here. Fix that, please.
>+++ b/extensions/
>+struct OverridePermiss
>+ OverridePermiss
>+ mContentType(
>+ mPolicy(aPolicy)
>+ {
>+ mHost.Assign(
Why not just mHost(aHost) in the initializers?
>+ OverridePermiss
>+ mContentType(
>+ mPolicy(
>+ {
>+ mHost.Assign(
And mHost(entry.mHost) here?
Also, s/entry/aOther/ or some such?
>+ OverridePermiss
>+static nsTArray<
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(
....
>+ NS_ENSURE_
...
>+ return mOverridePermis
IndexOf on nsTArray doesn't return |int|. It returns the relevant index_type. Make this function return nsTArray<
>+static int iprint = 0;
I have no idea what this is left over from, but presumably it should go away.
>+nsContentBloc
>+ NS_ENSURE_
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 = nsIContentPolic
I don't like this part, but more on that below.
>+ nsCString host;
>+ nsresult rv = aURI->GetHost(
>+ NS_ENSURE_
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(
>+ if (loc == -1) {
Again, index_type. You might want to typedef the right type (e.g. the relevant nsTArray specialization type) for readability.
>+nsContentBloc
>+ NS_ENSURE_
Again, not sure what that would even mean.
>+ const int loc = FindPermission(
>+ if (loc != -1)
>+ mOverridePermis
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 ...
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #79 |
(In reply to comment #74)
> >+ OverridePermiss
>
> >+static nsTArray<
>
> 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.
> >+nsContentBloc
> >+ NS_ENSURE_
>
> 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(
> >+ NS_ENSURE_
>
> 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(
> >+ if (loc != -1)
> >+ mOverridePermis
>
> 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(
> 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/
> Also, shouldn't this compare to nsIContentPolic
> 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 nsIContentPolic
> It looks like overriding with REJECT_* is just ignored. Is that actually on
> purpo...
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #80 |
> 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...
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #81 |
(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.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #82 |
> 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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #83 |
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 nsImageLoadingC
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #84 |
> 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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #85 |
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 nsContentBlocke
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #86 |
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/
So basically, only http/https/ftp are possible to block.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #87 |
> so no need to provide the error handling
Why do you assume there will be no other callers?
> There's a short-circuit in nsContentBlocke
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/
You might want to get gavin's review before having me look at this again....
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #88 |
(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?
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #89 |
"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-
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #90 |
(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-
> 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|
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #91 |
> 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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #92 |
...and sorry again, I had thought nsContentBlocker dealt with the additions and removals of the content policy. Apparently it uses nsIPermissionMa
I'll update the patch later today, hopefully.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #93 |
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.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #94 |
>+ if (!aNewDecision)
>+ aNewDecision = nsIContentPolic
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(
NO_INDEX?
> + inline void CutWWWFromHost(
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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #95 |
Created an attachment (id=386894)
patch ver. 3
A little intro to this new patch:
The way nsPermissionsMa
This I took from http://
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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #96 |
Filed bug 502486 on the permission manager thing.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #97 |
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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #98 |
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 nsPermissionMan
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #99 |
I'd really welcome some feedback from whoever owns the permission manager here.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #100 |
From http://
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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #101 |
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...
In Mozilla Bugzilla #417545, Ehsan Akhgari (ehsan) wrote : | #102 |
(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.
In Mozilla Bugzilla #417545, Hskupin (hskupin) wrote : | #103 |
See https://<email address hidden>/
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #104 |
Thank you Ehsan, seems like there isn't any perf problem, at least none that talos could identify.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #105 |
Just to be clear, my review is waiting on a response to comment 95.
In Mozilla Bugzilla #417545, Dwitte (dwitte) wrote : | #106 |
Will get to it soon.
In Mozilla Bugzilla #417545, Dwitte (dwitte) wrote : | #107 |
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_
Is this not feasible?
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #108 |
The content blocker doesn't see a channel. It sees a URI.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #109 |
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.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #110 |
The caller of ShouldLoad does not have a channel in this case.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #111 |
Right, I keep on forgetting the content blocker comes in before the request is sent...
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #112 |
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?
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #113 |
> bz: would it be possible to ignore on a per element basis?
Just for content blocker? How, exactly?
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #114 |
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.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #115 |
> 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...
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #116 |
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...
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #117 |
(From update of attachment 390741)
r? gavin for /browser parts
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #118 |
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 Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #119 |
(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:
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.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #120 |
> 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 Mozilla Bugzilla #417545, Dwitte (dwitte) wrote : | #121 |
(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 Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #122 |
(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-
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.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #123 |
> 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 Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #124 |
(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...
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #125 |
> 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.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #126 |
(From update of attachment 390741)
http://
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...
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #127 |
Ok, so what now? Would using aExtra for this be better? Is there any consensus on how to implement ignorePolicy?
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #128 |
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.
Changed in firefox: | |
status: | In Progress → Confirmed |
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #129 |
(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.
Changed in firefox: | |
importance: | Unknown → Medium |
In Mozilla Bugzilla #417545, Mounir-lamouri (mounir-lamouri) wrote : | #130 |
*** Bug 626514 has been marked as a duplicate of this bug. ***
In Mozilla Bugzilla #417545, Matti-mversen (matti-mversen) wrote : | #131 |
*** Bug 681610 has been marked as a duplicate of this bug. ***
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #132 |
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 nsImageLoadingC
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #133 |
Comment on attachment 556604
WIP 1 - New Approach
Jonas, I'd appreciate some feedback on this patch.
Thanks.
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #134 |
Casting from a contact-gotten interface pointer to a concrete class is not ok.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #135 |
(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?
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #136 |
> 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...
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #137 |
Created attachment 556653
WIP 2
Hi Benjamin, can you let me know if including "nsContentBlock
In Mozilla Bugzilla #417545, Benjamin Smedberg (Mozilla) [:bs] (benjamin-smedbergs) wrote : | #138 |
Comment on attachment 556653
WIP 2
extensions/
In Mozilla Bugzilla #417545, Ehsan-mozilla (ehsan-mozilla) wrote : | #139 |
Couldn't we just suppress checking the blocking status inside nsImageLoadingC
In Mozilla Bugzilla #417545, Bzbarsky (bzbarsky) wrote : | #140 |
There are content policies that implement security checks, last I checked, so we can't just skip calling into content policy code.
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #141 |
(In reply to Ehsan Akhgari [:ehsan] from comment #135)
> Couldn't we just suppress checking the blocking status inside
> nsImageLoadingC
> 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?
In Mozilla Bugzilla #417545, Ehsan-mozilla (ehsan-mozilla) wrote : | #142 |
Try pinging him on IRC? He may not be watching his bugmail closely...
In Mozilla Bugzilla #417545, Sgautherie-bz (sgautherie-bz) wrote : | #143 |
Comment on attachment 556653
WIP 2
Review of attachment 556653:
-------
::: browser/
@@ +189,5 @@
> browser_
> browser_
> browser_
> + browser_
> + image_Reload.html \
These files are missing from this patch, aren't they?
In Mozilla Bugzilla #417545, Highmind63 (highmind63) wrote : | #144 |
Not really working on this anymore.
Changed in firefox: | |
status: | Confirmed → Unknown |
Changed in firefox: | |
status: | Unknown → Confirmed |
Changed in firefox: | |
importance: | Medium → Unknown |
In Mozilla Bugzilla #417545, Release-mgmt-account-bot (release-mgmt-account-bot) wrote : | #145 |
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:/
In Mozilla Bugzilla #417545, Autonag-nomail-bot (autonag-nomail-bot) wrote : | #146 |
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.
(From update of attachment 303318) unencrypted- resources- from-encrypted- pages.
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-
Could you instead have the specific content policies check the new property? (and call it something like userInitiatedLoad or some such)