1095798 - Add NetUtil.NewChannel2 and NetUtil.asyncFetch2 to NetUtil.jsm

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

In order to land all of the dependency bugs/patches of Bug 1087720 we should add NewChannel2 and asyncFetch2 to Netutil.jsm. We only add them as an interim solution so we can land bugs inependently of each other.

See also:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1087720#c6

Once all dependencies of Bug 1087720 are cleared we can remove newChannel and asyncFetch completely.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

In fact this bug blocks all of the dependencies of Bug 1087720.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1095798.patch

I am not sure if you are fine reviewing this, but it doesn't actually change any of the logic, we just call newChannelFromURI*2* and newChannel*2* internally of the newly added functions.

If not, let me know and I find someone else.

Comment 3

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

Comment on attachment 8519281 [details] [diff] [review]
bug_1095798.patch

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

I'm assuming these are just copies of the existing functions, but which pass through the additional loadinfo arguments.

r=me if that's the case

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #3)
> Comment on attachment 8519281 [details] [diff] [review]
> bug_1095798.patch
> 
> Review of attachment 8519281 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm assuming these are just copies of the existing functions, but which pass
> through the additional loadinfo arguments.

That is exactly what's happening in the patch.
 
> r=me if that's the case

Comment 5

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

Comment on attachment 8519281 [details] [diff] [review]
bug_1095798.patch

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

::: netwerk/base/src/NetUtil.jsm
@@ +182,5 @@
> +     * Note: As an interim we have asyncFetch as well as asyncFetch2.
> +     *       Once Bug 1087720 (which converts all js callers to use
> +     *       asyncFetch2) lands, we can remove newChannel completely.
> +     */
> +    asyncFetch2: function NetUtil_asyncOpen2(aSource,

This should be called NetUtil_asyncFetch2.

You should assert that if |aSource instanceof Ci.nsIChannel| then assert that all the new arguments are '=== undefined'

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_0_extend_netutiljsm.patch

Carrying over r+ from Jonas. We should still wait before we land this until Bug 1099296 is fixed.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_0_extend_netutiljsm.patch

Hey Jonas, hopefully we can avoid such nice surprises as last night. Can you take a look at this one again before it goes in? That is next in the landing plan!

Comment 8

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

Comment on attachment 8535757 [details] [diff] [review]
js_0_extend_netutiljsm.patch

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

::: netwerk/base/src/NetUtil.jsm
@@ +174,5 @@
> +     *        3) Reference to the nsIRequest.
> +     * @param aLoadingNode
> +     *        The loadingDocument of the channel.
> +     *        The element or document where the result of this request will be
> +     *        used. This is the document/element that will get access to the

You don't need to duplicate the docs twice really. Just have one refer to the other. Maybe have this one refer to newChannel since newChannel is more low-level.

@@ +226,5 @@
> +     */
> +    asyncFetch2: function NetUtil_asyncFetch2(aSource,
> +                                              aCallback,
> +                                              aLoadingNode,
> +                                              aLoadingPrincipal,

This function duplicates a lot of asyncFetch. Please make asyncFetch call this function and just pass null for all the new arguments. Except when aSource is a channel in which case you should leave them out.

@@ +246,5 @@
> +        if (aSource instanceof Ci.nsIChannel) {
> +          if ((typeof aLoadingNode != "undefined") ||
> +              (typeof aLoadingPrincipal != "undefined") ||
> +              (typeof aSecurityFlags != "undefined") ||
> +              (typeof aContentPolicyType != "undefined")) {

This is missing triggering principal.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_0_extend_netutiljsm.patch

Incorporated Jonas suggestions but still keeping asyncFetch and asyncFetch2 completely independent from each other. All the dependencies of Bug 1087720 should land within the next days/weeks. Once all of them landed we can completely remove newChannel() as well as asyncFetch() from NetUtil.jsm.

The problem with forwarding calls from asyncFetch to AsyncFetch2 is, that asynFetch2 might call newChannel2(), so we would need to add special code handling that case based on the loadInfo arguments which might be error-prone. I rather keep the duplicate code for the interim time and delete asyncFetch and newChannel once Bug 1087720 cleared.

Comment 10

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

Comment on attachment 8536313 [details] [diff] [review]
js_0_extend_netutiljsm.patch

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

::: netwerk/base/src/NetUtil.jsm
@@ +184,5 @@
> +     * @param aContentPolicyType
> +     *        The contentPolicyType of the channel.
> +     *        Any of the content types defined in nsIContentPolicy.idl
> +     *
> +     * For a more detailed argument description, see NetUtil_newChannel2.

I don't think people will read this far in the docs.

Put this description on each of the arguments instead. Or just lump all the relevant arguments together and put this description on the lump.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_0_extend_netutiljsm.patch

Incorporated Jonas's suggestion regarding the @param list.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b921743d876e

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[:Paolo Amadini]

I believe that such a widely used API should have had another pair of eyes looking at it, i.e. a round of super-review would have been advised before the work to migrate all its consumers started.

I've filed bug 1125618 with my concerns about the current API. There's no need to obsolete it or redo any of the migration work now, but we still have the opportunity of recommending a better API going forward.