Convert NetUtil.newChannel2 callsites to use new API

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

We should convert all callsites of NetUtil.newChannel2 to actually use the new API [1] and eliminate newChannel2. We should further remove asyncFetch2 before people start using it.

Further, once we start moving security checks into AsyncOpen2 (see Bug 1143922) we need to pass a security flag (e.g. REQUIRE_SAME_ORIGIN, REQUIRE_CORS, etc). Using the API defined here [1] allows us to use a default security flag without for most of the callsites without having to update all the callsites over and over.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#372
(Assignee)

Updated

4 years ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Depends on: 1125618, 1087720
Attachment #8608502 - Flags: review?(paolo.mozmail)
Attachment #8608502 - Flags: review?(jonas)
Attachment #8608503 - Flags: review?(paolo.mozmail)
Attachment #8608503 - Flags: review?(jonas)
Attachment #8608504 - Flags: review?(paolo.mozmail)
Attachment #8608504 - Flags: review?(jonas)

Comment 4

4 years ago
Comment on attachment 8608502 [details] [diff] [review]
bug_1167053_convert_netutiljsm.patch

Thanks for doing this work. Much improved code base!

If you're landing this as multiple changesets, ideally this one should land last.
Attachment #8608502 - Flags: review?(paolo.mozmail) → review+

Comment 5

4 years ago
Comment on attachment 8608503 [details] [diff] [review]
bug_1167053_convert_netutiljsm_asyncFetch2_callsites.patch

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

r+ with changes.

::: toolkit/devtools/transport/tests/unit/test_client_server_bulk.js
@@ +249,5 @@
>  
>    // Ensure output file contents actually match
>    let compareDeferred = promise.defer();
> +  NetUtil.asyncFetch({
> +    uri: NetUtil.newURI(getTestTempFile("bulk-input")),

That should be "bulk-output".

::: toolkit/devtools/transport/tests/unit/test_queue.js
@@ +151,5 @@
>  
>    // Ensure output file contents actually match
>    let compareDeferred = promise.defer();
> +  NetUtil.asyncFetch({
> +    uri: NetUtil.newURI(getTestTempFile("bulk-input")),

Also here.
Attachment #8608503 - Flags: review?(paolo.mozmail) → review+

Comment 6

4 years ago
Comment on attachment 8608504 [details] [diff] [review]
bug_1167053_convert_netutiljsm_newChannel2_callsites.patch

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

r+ with changes.

::: addon-sdk/source/lib/sdk/net/url.js
@@ +34,5 @@
>  function readURI(uri, options) {
>    options = options || {};
>    let charset = options.charset || 'UTF-8';
>  
> +  let channel = NetUtil.newChannel({uri: uri, loadUsingSystemPrincipal: true});

In this case (and below) you need NetUtil.newURI(uri, charset) to honor the charset option.

(I'm not sure how may callsites use it however, it's probably close to zero).

::: addon-sdk/source/lib/toolkit/loader.js
@@ +178,5 @@
>      uri = proto.resolveURI(nsURI);
>    }
>  
> +  let stream = NetUtil.newChannel(
> +    {uri: uri, loadUsingSystemPrincipal: true}).open();

nit: In cases like this you may use the shorthand { uri, loadUsingSystemPrincipal: true }.

::: browser/devtools/performance/modules/io.js
@@ +77,5 @@
>      let deferred = promise.defer();
>  
> +    let channel = NetUtil.newChannel({
> +      uri: NetUtil.newURI(file),
> +      loadUsingSystemPrincipal: true});

Hehe, it's curious how this module called "performance/modules/io.js" still does main-thread I/O itself :-)

Can you file a "[good next bug]" to convert this (and maybe other devtools sites) to use OS.File? The functions are already async so it should be easy for a new contributor.

::: browser/extensions/pdfjs/content/PdfStreamConverter.jsm
@@ +158,5 @@
>  
>  function createNewChannel(uri, node, principal) {
> +  return NetUtil.newChannel({
> +    uri: uri,
> +    loadingPrincipal: principal,

Missing loadingNode.

::: dom/apps/TrustedHostedAppsUtils.jsm
@@ +206,5 @@
> +    let mRequestChannel = NetUtil.newChannel({
> +      uri: aApp.manifestURL,
> +      loadingPrincipal: principal,
> +      contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER})
> +      .QueryInterface(Ci.nsIHttpChannel);

nit: closing brace on next line aligned with "let" (same below)

::: dom/apps/Webapps.jsm
@@ +3527,5 @@
>                        appURI, aNewApp.localId, false);
>  
>      if (aIsLocalFileInstall) {
> +      requestChannel = NetUtil.newChannel({
> +        uri: aFullPackagePath,

I assume aFullPackagePath is an nsIURI or URI string?
Attachment #8608504 - Flags: review?(paolo.mozmail) → review+
Thanks Paolo for the quick turnaround time - incorporated all of your suggestions.
Attachment #8608503 - Attachment is obsolete: true
Attachment #8608504 - Attachment is obsolete: true
Attachment #8608503 - Flags: review?(jonas)
Attachment #8608504 - Flags: review?(jonas)
Attachment #8608830 - Flags: review?(jonas)
Comment on attachment 8608502 [details] [diff] [review]
bug_1167053_convert_netutiljsm.patch

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

Push this last
Attachment #8608502 - Flags: review?(jonas) → review+
Comment on attachment 8608830 [details] [diff] [review]
bug_1167053_convert_netutiljsm_asyncFetch2_callsites.patch

Chattet with Jonas on IRC - he is fine with what we are doing here and the reviews of Paolo.
Attachment #8608830 - Flags: review?(jonas) → review+
(Assignee)

Updated

4 years ago
Attachment #8608832 - Flags: review?(jonas) → review+

Comment 13

4 years ago
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/137928d76afd4e95860629370d5481bf9ad44754
Bug 1167053 - Convert NetUtil.newChannel2 callsites to use new API - update asyncFetch2 (r=sicking,paolo)

https://github.com/mozilla/addon-sdk/commit/749b2b7fe5901ef1fd90e08e1c331f88420e422d
Bug 1167053 - Convert NetUtil.newChannel2 callsites to use new API - update newChannel2 (r=sicking,paolo)
You need to log in before you can comment on or make changes to this bug.