200433 - Image blocking no longer works for <iframe>

Status: RESOLVED FIXED

(Core :: Graphics: Image Blocking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The checkin for bug 121084 broke image blocking for <iframe>.  We want to fix it
but not regress bug 34165 in the process.  Perhaps this is a case for
ShouldProcess?  irc conversation:

> dwitte: <iframe src="image.gif">
> dwitte: right now, this performs no content policy checks
> dwitte: it shows the image in the iframe no matter what
* dwitte coughs
<dwitte> nice
> dwitte: before, it would load all the image data... then perform a content policy
+check and just not paint it if the check failed
> dwitte: ideally, I'd like to just drop the image load as soon as I know I can,
if the
+policy says it's not to be loaded
> dwitte: ok.  So what I want to do is in the OnStartRequest to check with content
+policy somehow (presumably using NS_CheckContentProcessPolicy)
<dwitte> so what do ShouldLoad/ShouldProcess require to work?
> well...
> ShouldLoad and ShouldProcess are slightly different
> ShouldLoad can only ask for a URI and context for the load
> ShouldProcess can ask for a channel, if it needs one, and context
> as I see it, a simplistic ShouldProcess impl for images would just check the
channel. 
+If it's a document channel it would check whether the docshell has parents...
if so,
+block the load
<dwitte> why would you wanna block if the docshell has parents?
* dwitte has trouble interpreting that :)
> because then its a frame or iframe
> or rather, if it has parent we test permissions
> if it has no parents, I don't think image blocking should apply period
<dwitte> hmm
> because then it's just the toplevel mozilla browser area
<dwitte> define your last sentence?
> find an image
> that's blocked
> right click on it
> select "view image"
> that should show the image.  IMO.
<dwitte> bz: so does this sound right/
<dwitte> ?
<dwitte> bz: we start by calling ShouldProcess
<dwitte> bz: that checks if we have parents... if so, we call ShouldLoad to
check the
+permission list, etc
<dwitte> bz: if not, we allow it
> we also need to make sure the channel is a document one
> but yes, that sounds reasonable
<dwitte> right, and all that fun stuff :)
> point is, imo ShouldProcess should take a channel. ;)
<dwitte> so we're just adding an extra layer of abstraction to ShouldLoad,

mvl, got time to work on this?

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

This makes us stop the image load as soon as we realize it's an image.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153494 [details] [diff] [review]
Proposed fix

mvl, would you review the image manager changes?

jst, would you review the content code?

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 153494 [details] [diff] [review]
Proposed fix

nice

>+  if (NS_FAILED(rv) || NS_CP_REJECTED(decision)) {
>+    request->Cancel(NS_ERROR_CONTENT_BLOCKED);
>+    return NS_OK;
>+  }

Shouldn't that do the same kind of check as
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#5136 for
example? return SHOW_ALT when needed.

Comment 5

[Boris Zbarsky [:bzbarsky]]

It's a frame pointed to an image... there's no one to see the return value here
but ourselves.  I just needed an error code to pass to necko (and considered
NS_BINDING_ABORTED), but this one is a _little_ more informative...

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 153494 [details] [diff] [review]
Proposed fix

sr=jst

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 8

[Worcester12345]

(In reply to comment #7)
> Fixed on trunk.

Is this one also on branch, or is it working on branch?

Comment 9

[Boris Zbarsky [:bzbarsky]]

This is not working on branch.  It will continue to not work on branch, since
making it work relies on major API changes that happened after branch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Note that this got broken again by the patch in bug 253597.  I've filed bug
254510 on that.