1144274 - Update remaining calles of newChannel to newChannel2 in netwerk/base/nsProtocolProxyService.cpp

Status: RESOLVED DUPLICATE of bug 1144270

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1144274_protocolproxyservice.patch

Gijs, by now you know what arguments we are looking for, right? Please find a description of all the arguments here:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.h#190

Please note that using the systemPrincipal is not that great, it would be better to find a more accurate loading principal.

Comment 2

[:Gijs (he/him)]

Comment on attachment 8578827 [details] [diff] [review]
bug_1144274_protocolproxyservice.patch

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

I am not a netwerk peer, so I really shouldn't be reviewing this, sorry.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 8578827 [details] [diff] [review]
> bug_1144274_protocolproxyservice.patch
> 
> Review of attachment 8578827 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am not a netwerk peer, so I really shouldn't be reviewing this, sorry.

Hey Gijs, the reason I asked you is because you wrote that code in bug 1125372, so I was thinking that you might be best suited to help me find the right principal to use.

Once that is done I can ask a network peer(e.g. sworkman) to sign off on the final patch.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8578827 [details] [diff] [review]
bug_1144274_protocolproxyservice.patch

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

::: netwerk/base/nsProtocolProxyService.cpp
@@ +1287,5 @@
>          }
>  
>          // make a temporary channel from the URI
> +        nsCOMPtr<nsIScriptSecurityManager> secMan(
> +            do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv));

Hrmpf. Wonder if you could get this added to the C++ mozilla::services instead. (add a macro in ServiceList.h). Maybe that should be a followup bug, I don't know how difficult it is to get things in there and/or how people deal with that.

@@ +1295,5 @@
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        rv = NS_NewChannel(getter_AddRefs(channel),
> +                           uri,
> +                           systemPrincipal,

Right... I don't think there's any way to get a better principal here - all we get is a URI. We should audit callers in-tree inasmuch as they haven't been already to just pass channels instead.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Just chatted with sworkman on IRC, it seems we can combine this patch with the patch in bug 1144270. Hence marking this as a duplicate.