1125618 - Revise NetUtil.jsm API for creating channels and fetching data while providing load info

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[:Paolo Amadini]

+++ This bug was initially created as a clone of Bug #1095798 +++

While the intention behind it could have been to force consumers into thinking about the value of each different parameter required to create the channel, in the end I think the API in bug 1095798 turned out to have the opposite effect, without at the same time being as good an API for JavaScript as it could have been. Here are typical usages in chrome code:

+NetUtil.newChannel2(NetUtil.newURI(download.source.url),
+                                            null,
+                                            null,
+                                            null,      // aLoadingNode
+                                            Services.scriptSecurityManager.getSystemPrincipal(),
+                                            null,      // aTriggeringPrincipal
+                                            Ci.nsILoadInfo.SEC_NORMAL,
+                                            Ci.nsIContentPolicy.TYPE_OTHER);

+    return Services.io.newChannelFromURI2(fileuri,
+                                          null,      // aLoadingNode
+                                          Services.scriptSecurityManager.getSystemPrincipal(),
+                                          null,      // aTriggeringPrincipal
+                                          Ci.nsILoadInfo.SEC_NORMAL,
+                                          Ci.nsIContentPolicy.TYPE_IMAGE);

Unfortunately, there's now a copy-and-paste of this pattern around the tree with minor variations - but each of those doesn't draw attention to which of these parameters are different from the commonly used default, making the intention difficult to understand.

The calls above might be rewritten as:



NetUtil.newChannel({
  uri: NetUtil.newURI(download.source.url),
});

NetUtil.newChannel({
  uri: NetUtil.newURI(download.source.url),
  contentPolicyType: Ci.nsIContentPolicy.TYPE_IMAGE,
});



Open questions:
1) Whether we want to actively force add-on developers into rewriting code that calls NetUtil.newChannel or Services.io.newChannelFromURI.
2) Related, whether we want to overload NetUtil.newChannel or use a different name.

Personally I don't think there's value in doing (1), as we'd only encourage authors to migrate to whatever works by copy-and-pasting code from mozilla-central (and easily picking up a call with the wrong arguments if they don't pay close attention).

Instead we already can, and should anyways, flag AMO add-ons whose code contains the strings "NetUtil.newURI" or "Services.io.newChannelFromURI" so they get additional scrutiny. CC'ing Jorge Villalobos as well, but I believe he'll agree.

In my view, it follows that we shouldn't make the current syntax for NetUtil.newChannel obsolete. We should however encourage migrating JavaScript code from Services.io.newChannelFromURI to NetUtil.newURI.

Comment 1

[:Paolo Amadini]

As a clarification, in this bug I think we should design a better API going forward, but I don't think that we should in any way undo any of the migration work that already started to get the load info on channels from the start (using newChannel2).

Actually, if we go with the assumption that we audited our in-tree calls to NetUtil.newChannel and determined we require default parameters, this might even remove some of the formal review requirements. For example, the global Downloads.jsm code would require no change to its restarting process.

Comment 2

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

Thanks for starting this discussion.

I'm definitely in favor of creating a better API here. However, I think we need to be very careful about which arguments we make optional and give "default" values.

In particular, I don't think that we should allow both the loadingNode and loadingPrincipal to be left out. Mainly because there isn't a reasonable default behavior. I think it needs to be very explicit when a caller wants to use the system principal as the loading principal as that essentially means "don't do any security checks". Clearly that's not something we should do unless explicitly asked for.

Likewise, the load type also doesn't really have a reasonable default. Though possibly we could allow it to be left out if the loadingPrincipal is the system principal since generally we won't be doing security checks then anyway. Not sure if I feel strongly.

But I agree that the rest can remain optional. And I think having just the above two as required would result in people thinking about what values to use for them.

What I do think we should do though, is allow the URL to be passed as a string or a nsIURI object. That actually already appears to be the case. Though we could do this even better by getting the charset and baseURI off of the loadingNode.

The result would be something like:

NetUtil.newChannel({
  uri: download.source.url,
  loadingNode: aContentWindow.document,
  loadType: Ci.nsIContentPolicy.TYPE_IMAGE
});

NetUtil.newChannel({
  uri: download.source.url,
  loadingPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),
});

NetUtil.newChannel({
  uri: download.source.url,
  loadingNode: aContentWindow.document,
  triggeringPrincipal: someOtherPrincipal
});

Comment 3

[:Paolo Amadini]

This is quite an improvement over the original proposal. Sounds good to me!

(In reply to Jonas Sicking (:sicking) from comment #2)
> we could allow it to be left out if the loadingPrincipal is the system principal

I've seen TYPE_OTHER is quite common in chrome code, so I'd say let's make it the default for the system principal.

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to :Paolo Amadini from comment #0)
> Instead we already can, and should anyways, flag AMO add-ons whose code
> contains the strings "NetUtil.newURI" or "Services.io.newChannelFromURI" so
> they get additional scrutiny. CC'ing Jorge Villalobos as well, but I believe
> he'll agree.

You're suggesting that we add this to our automatic code validation on AMO, correct?

Comment 5

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

So, that leaves the important question: Who has time to work on this?

Comment 6

[:Paolo Amadini]

(In reply to Jonas Sicking (:sicking) from comment #5)
> So, that leaves the important question: Who has time to work on this?

I can find some time, as the change is quite simple and does not make sense to defer it too much, or add-ons would have to support multiple APIs instead of skipping newChannel2 entirely.

To handle the special defaults and make the API even simpler, I think we should have a third loading property dedicated to the system principal, which must have the value "true" or must be omitted in favor of loadingNode/loadingPrincipal:

NetUtil.newChannel({
  uri: download.source.url,
  loadingFromSystemPrincipal: true,
});

Comment 7

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

(In reply to :Paolo Amadini from comment #6)
> NetUtil.newChannel({
>   uri: download.source.url,
>   loadingFromSystemPrincipal: true,
> });

I was thinking about that too. I don't have a strong opinion, so this would be fine with me. Maybe "loadUsingSystemPrincipal: true" is technically more correct?

Comment 8

[:Paolo Amadini]

Attachment: Part 1 - Changes to newChannel

The previous API change moved the newChannel tests to newChannel2, but did not add new tests. This one reverts all the tests to use newChannel again, this time with some additional API checks. While not as complete as they could be, there are still more tests than before.

Comment 9

[:Paolo Amadini]

(In reply to Jonas Sicking (:sicking) from comment #7)
> (In reply to :Paolo Amadini from comment #6)
> > NetUtil.newChannel({
> >   uri: download.source.url,
> >   loadingFromSystemPrincipal: true,
> > });
> 
> I was thinking about that too. I don't have a strong opinion, so this would
> be fine with me. Maybe "loadUsingSystemPrincipal: true" is technically more
> correct?

Missed this comment. "loadUsingSystemPrincipal" was actually my first choice too.

Comment 10

[:Paolo Amadini]

Attachment: Part 1 - Changes to newChannel

Comment 11

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

Comment on attachment 8559982 [details] [diff] [review]
Part 1 - Changes to newChannel

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

Looks great!

Should we maybe allow loading an nsIFile using the new syntax too? Something like

NetUtil.newChannel({ file: mynsifile, loadUsingSystemPrincipal: true });

Alternatively we can just let callers keep doing NetUtil.newChannel(mynsifile), but that feels a bit inconsistent.

Comment 12

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

Comment on attachment 8559982 [details] [diff] [review]
Part 1 - Changes to newChannel

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

Hmm.. actually.. i'm not sure how I feel about reverting the tests.

The downside with reverting the tests is that it means that we're still creating channels without an nsILoadInfo (since no loadingNode/loadingPrincipal is provided). I think that will trigger various warnings and make it harder to ensure that we everywhere in our code are creating channels "the right way".

But I guess there are advantages to testing that the old API works as well since we don't want to accidentally regress addons...

Christoph, what do you think?

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8559982 [details] [diff] [review]
Part 1 - Changes to newChannel

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

Paolo, the new API looks awesome; thanks for fixing it.

I am not too worried about converting the tests back; the ioservice should be able to handle both correctly, channels with and without loadInfo. As far as I know, we shouldn't trigger any assertions/warnings at the moment.

Comment 14

[:Paolo Amadini]

(In reply to Jonas Sicking (:sicking) from comment #11)
> Should we maybe allow loading an nsIFile using the new syntax too?

A bit surprisingly to me, I've found the nsIFile syntax doesn't have as many users as I thought, possibly one or two in-tree. Given that using an nsIFile with the new API is as simple as doing newChannel({ uri: NetUtil.newURI(file), ... }), I think we can implement the "file" property later, if ever needed.

Comment 15

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

Sounds good to me.

Comment 16

[:Paolo Amadini]

Attachment: Part 2 - Changes to asyncFetch

We're actually in a good place with asyncFetch, no change is required to it since the first argument is forwarded to the updated newChannel.

As for using nsIFile with asyncFetch, in general it should be avoided because it causes main-thread I/O. When this is needed, the existing syntax can be used, or alternatively the following new syntax:

// Deprecated because of main-thread I/O
asyncFetch({ uri: NetUtil.newURI(file), ...loadinfo... }, callback);

Comment 17

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

Comment on attachment 8560603 [details] [diff] [review]
Part 2 - Changes to asyncFetch

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

::: netwerk/base/NetUtil.jsm
@@ +91,5 @@
>       * @param aSource
> +     *        This should be an nsIChannel, nsIInputStream, or options object
> +     *        that will be passed to NetUtil.newChannel.
> +     *        Using an nsIURI, nsIFile, or string spec directly is now
> +     *        deprecated.

Maybe simply say "see newChannel". The fact that the preferred argument is an options object of the same format as newChannel is a bit buried here.

Comment 18

[:Paolo Amadini]

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

Comment 19

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/e6b4bb4cc393

Comment 20

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

Awesome!

Will you have time to help converting any of the existing callsites? Otherwise we might get to some of them eventually.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e6b4bb4cc393

Comment 22

[:Paolo Amadini]

(In reply to Jonas Sicking (:sicking) from comment #20)
> Will you have time to help converting any of the existing callsites?
> Otherwise we might get to some of them eventually.

I don't think I'll get to it soon, but differently from the migration from newChannel to newChannel2, this is mechanical in nature so I feel it can be simpler and can be done in larger batches, as it doesn't need domain-specific expertise.

For the jsdownloads portion I've filed a bug that I think can be fine for new contributors to take, but don't feel constrained by this and go ahead if you want to do it first. We can remove newChannel2 and asyncFetch2 completely as soon as we remove all the callsites.