Pass the right context to new channels for image loads

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 attachments, 3 obsolete attachments)

No description provided.
Assignee: nobody → mozilla
Blocks: 1206961
Status: NEW → ASSIGNED
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
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
Attachment #8730958 - Flags: review?(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.
Attachment #8730958 - Flags: review?(seth)
Attachment #8730958 - Flags: review?(bzbarsky)
Attachment #8730958 - Flags: review+
(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.
Flags: needinfo?(bzbarsky)
(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 :-(
(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.
Flags: needinfo?(bzbarsky)
> >+++ 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.
Attachment #8730958 - Attachment is obsolete: true
Attachment #8730958 - Flags: review?(seth)
Attachment #8732407 - Flags: review+
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!
Attachment #8732407 - Flags: review?(seth)
> Ah facepalm, QI of cource :-(

Right.  Note that you can't safely static_cast from nsISupports to nsIImageLoadingContent either, obviously.  ;)
As suggested by Boris, we should use nsIDocument instead of nsIDOMDocument within ImageCacheKey.
Attachment #8732417 - Flags: review?(seth)
Attachment #8732417 - Flags: review?(bzbarsky)
Comment on attachment 8732417 [details] [diff] [review]
bug_1256999_fix_imagecachekey.patch

r=me
Attachment #8732417 - Flags: review?(bzbarsky) → review+
Whiteboard: [domsecurity-active]
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.
Attachment #8732407 - Flags: review?(seth) → review+
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.
Attachment #8732417 - Flags: review?(seth) → review+
Incorporated nits, carrying over r+ from Seth and bz.
Attachment #8732407 - Attachment is obsolete: true
Attachment #8732417 - Attachment is obsolete: true
Attachment #8739888 - Flags: review+
Incorporated nits, carrying over r+ from Seth and bz.
Attachment #8739889 - Flags: review+

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3b53ecaae8f2
https://hg.mozilla.org/mozilla-central/rev/1258adc301a8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.