Closed Bug 1095798 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
In fact this bug blocks all of the dependencies of Bug 1087720.
Blocks: 1087720
Attached patch bug_1095798.patch (obsolete) — Splinter Review
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.
Attachment #8519281 - Flags: review?(jonas)
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
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
Attachment #8519281 - Flags: review?(jonas) → review+
(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 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'
Attached patch js_0_extend_netutiljsm.patch (obsolete) — Splinter Review
Carrying over r+ from Jonas. We should still wait before we land this until Bug 1099296 is fixed.
Attachment #8525673 - Flags: review+
Attachment #8519281 - Attachment is obsolete: true
Blocks: 1099587
Blocks: 1099585
Attached patch js_0_extend_netutiljsm.patch (obsolete) — Splinter Review
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!
Attachment #8535757 - Flags: review?(jonas)
Attachment #8525673 - Attachment is obsolete: true
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.
Attachment #8535757 - Flags: review?(jonas) → review+
Attached patch js_0_extend_netutiljsm.patch (obsolete) — Splinter Review
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.
Attachment #8535757 - Attachment is obsolete: true
Attachment #8536313 - Flags: review+
Blocks: 1087730
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.
Incorporated Jonas's suggestion regarding the @param list.
Attachment #8536313 - Attachment is obsolete: true
Attachment #8536718 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b921743d876e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 1125618
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.
You need to log in before you can comment on or make changes to this bug.