1256999 - Pass the right context to new channels for image loads

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

As discussed within Bug 1206961, we need to pass the right context to new image loads. For more details see [1] in particular.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1206961#c37

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1256999_pass_right_context_to_image_channels.patch

Boris, just extending nsContentUtils::LoadImage() by an additional argument. Everything basically remains the same, but we are passing the new 'context' as an argument to NS_NewChannel() in the end. Does that make sense? Later we can use that context to perform a do_QI into the nsIImageLoadingContent so we can call SetBlockedRequest on it.

Further, I am not sure, but I suppose we could also remove the aCx argument from LoadImageXPCOM() completely, because as far as I can tell, we only pass 'null' for that argument in all the cases anyway, see:
http://mxr.mozilla.org/mozilla-central/search?string=LoadImageXPCOM%28

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8730958 [details] [diff] [review]
bug_1256999_pass_right_context_to_image_channels.patch

If aContext is supposed to be the context node, please make it be an nsINode at least in the args to the nsContentUtils method.  That's all you really care about in the end once you get down to NewImageChannel.

But also, if you're going to be passing nsIDocument across the imgLoader API boundary, there's no reason not to pass nsINode.

The documentation should more accurately reflect this.

>+++ b/dom/base/nsDocument.cpp

And then that would help prevent issues like this one, where you got the arguments in the "wrong" order.  I assume this doesn't actually compile.

>+++ b/image/imgLoader.cpp
>+  nsCOMPtr<nsIDOMDocument> doc = do_QueryInterface(aLoadingDocument);

If we have an nsIDocument already, we should just use nsIDocument for the key type in the hashtable, right?  Followup is OK for this part.

>+++ b/image/imgLoader.h

This needs review from Seth.

r=me modulo the above.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #3)
> If aContext is supposed to be the context node, please make it be an nsINode
> at least in the args to the nsContentUtils method.  That's all you really
> care about in the end once you get down to NewImageChannel.

Well, within nsImageLoadingContent.cpp we need to pass |static_cast<nsIImageLoadingContent*>(this)| as the context so we can call SetBlockedRequest() on it (within Bug 1206961). nsIImageLoadingContent does not implement nsINode, see:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIImageLoadingContent.idl#36

> But also, if you're going to be passing nsIDocument across the imgLoader API
> boundary, there's no reason not to pass nsINode.

I don't fully understand what you mean here. Are you saying we should update |nsIDocument* aLoadingDocument| to nsINode? Because that doesn't really make sense to me, but maybe I am interpreting your suggestion completely wrong.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> Well, within nsImageLoadingContent.cpp we need to pass
> |static_cast<nsIImageLoadingContent*>(this)| as the context so we can call
> SetBlockedRequest() on it (within Bug 1206961). nsIImageLoadingContent does
> not implement nsINode, see:
> http://mxr.mozilla.org/mozilla-central/source/dom/base/
> nsIImageLoadingContent.idl#36

Ah facepalm, QI of cource :-(

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> > But also, if you're going to be passing nsIDocument across the imgLoader API
> > boundary, there's no reason not to pass nsINode.

Ah, now I understand. Since we are already passing nsIDocument, we should use nsINode for the context and not nsISupports. I'll update that as well.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1256999_pass_right_context_to_image_channels.patch

> >+++ b/image/imgLoader.cpp
> >+  nsCOMPtr<nsIDOMDocument> doc = do_QueryInterface(aLoadingDocument);
> 
> If we have an nsIDocument already, we should just use nsIDocument for the
> key type in the hashtable, right?  Followup is OK for this part.

That still needs a followup.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8732407 [details] [diff] [review]
bug_1256999_pass_right_context_to_image_channels.patch

Seth, Boris seems fine with this change. Still need your stamp on it though - thanks!

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Ah facepalm, QI of cource :-(

Right.  Note that you can't safely static_cast from nsISupports to nsIImageLoadingContent either, obviously.  ;)

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1256999_fix_imagecachekey.patch

As suggested by Boris, we should use nsIDocument instead of nsIDOMDocument within ImageCacheKey.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e10baaa932f

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8732417 [details] [diff] [review]
bug_1256999_fix_imagecachekey.patch

r=me

Comment 13

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8732407 [details] [diff] [review]
bug_1256999_pass_right_context_to_image_channels.patch

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

OK with me, though it seems a bit unfortunate that AFAICT |aContext| and |aLoadingDocument| are identical in the vast majority of cases. (Is XUL stuff the only exception?)

::: dom/base/nsContentUtils.h
@@ +688,5 @@
>     * keep a mutable version around should pass in a clone.
>     *
>     * @param aURI uri of the image to be loaded
> +   * @param aContext element of document where the result of this request
> +   *        will be used.

Align "will be used" with "element", please.

Comment 14

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8732417 [details] [diff] [review]
bug_1256999_fix_imagecachekey.patch

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

LGTM.

::: image/imgLoader.cpp
@@ +1327,5 @@
>  }
>  
>  NS_IMETHODIMP
>  imgLoader::FindEntryProperties(nsIURI* uri,
> +                               nsIDOMDocument* domDoc,

Please rename to aDOMDoc or something similar. I realize the other arguments don't follow Mozilla style, but let's at least move in that direction as long as we're touching this line.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1256999_pass_right_context_to_image_channels.patch

Incorporated nits, carrying over r+ from Seth and bz.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1256999_fix_imagecachekey.patch

Incorporated nits, carrying over r+ from Seth and bz.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b53ecaae8f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/1258adc301a8

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3b53ecaae8f2
https://hg.mozilla.org/mozilla-central/rev/1258adc301a8