1144270 - Update remaining calles of newChannel to newChannel2 in netwerk

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1144270_ioservice.patch

Jonas, we are creating a new channel within speculativeConnect() - ideally we would change the idl [1] to also pass a loadInfo. At them moment I am not sure how feasible that is, what do you think?

We can and should definitely *not* use the systemPrincipal for speculative loads.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsISpeculativeConnect.idl#28

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8578817 [details] [diff] [review]
bug_1144270_ioservice.patch

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

I don't know enough about this code to really have an informed opinion. But if these are "real" network connects that actually end up downloading a resource from the network, then yeah, this needs to take a loadInfo.

Or probably more likely, needs to take a |aLoadingNode, aLoadingPrincipal, aTriggeringPrincipal| triplet since it looks like this function is called from script.

I.e. if this is just a "normal" network request, just happening at a lower priority, then this function needs to be treated just like ioservice.newChannel is, i.e. we likely need to add ioservice.speculativeConnect2(...)

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

OTOH, if this just does a "preconnect", i.e. if this just does the TCP and TLS handshake, and doesn't actually do any http traffic, then we can do something simpler. But it doesn't look like that's what this is?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Arthur, since you wrote the code in bug 436344, I was wondering if you are able to answers Jonas' questions about the channel in question.

Comment 5

[Arthur Edelstein [:arthur]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> Arthur, since you wrote the code in bug 436344, I was wondering if you are
> able to answers Jonas' questions about the channel in question.

It does seem that requiring a |aLoadingNode, aLoadingPrincipal, aTriggeringPrincipal| triple would be important from a privacy perspective. For example, ad- and tracker-blocking extensions might intend to block all connections from a particular loading node.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1144270_netwerk.patch

Pat, I just chatted with sworkman on IRC, here are the important parts:

(10:37:55 AM) ckerschb: question, what is speculativeLoad doing? do we really need to create a channel there?
(10:38:14 AM) sworkman: yes, you need to create a channel
(10:38:24 AM) sworkman: spec connect is predictive networking
(10:38:34 AM) sworkman: getting connections warmed up so data can flow
(10:38:39 AM) ckerschb: is it actually doing a network load?
(10:38:47 AM) sworkman: no, not yet
(10:38:53 AM) sworkman: it’s just a TCP connection to a server
(10:38:54 AM) ckerschb: but potentially?
(10:38:58 AM) sworkman: potentially yes
(10:39:04 AM) ckerschb: so we need loadInfo args?
(10:39:19 AM) sworkman: I’m not sure, but I don’t think so
(10:39:23 AM) ckerschb: which means we have to change the *.idl of speculativeConnecct to hand args, right?
(10:39:31 AM) ckerschb: why not?
(10:39:32 AM) sworkman: this is a dummy channel used to create the TCP connection
(10:39:55 AM) sworkman: when the REAL channel responsible for the load is started, it will check the TCP pool to see if there’s an available connection
(10:40:10 AM) sworkman: that channel is the one that the checks should (and will) be done on
(10:40:37 AM) ckerschb: well, that is great news
(10:40:45 AM) ckerschb: do you think we could use systemPrincipal then?
(10:43:08 AM) ckerschb: still there?
(10:44:19 AM) sworkman: I think you could


I would like to make sure that it's safe to use the systemPrincipal as the loadingPrincipal in both cases, do you agree with Steve and me?

Comment 8

[Patrick McManus [:mcmanus]]

right - this is just a preconnect. Any preload (when it happens) will have a real channel attached to it.. and the expectation is that any load that uses this preconnect result comes via a channel.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b47ec7ffe71

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9b47ec7ffe71