1167053 - Convert NetUtil.newChannel2 callsites to use new API

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1167053_convert_netutiljsm.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1167053_convert_netutiljsm_asyncFetch2_callsites.patch

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1167053_convert_netutiljsm_newChannel2_callsites.patch

Comment 4

[:Paolo Amadini]

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.

Comment 5

[:Paolo Amadini]

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.

Comment 6

[:Paolo Amadini]

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?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1167053_convert_netutiljsm_asyncFetch2_callsites.patch

Thanks Paolo for the quick turnaround time - incorporated all of your suggestions.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1167053_convert_netutiljsm_newChannel2_callsites.patch

Comment 9

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

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

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

Push this last

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b3d8621fe58
https://hg.mozilla.org/integration/mozilla-inbound/rev/eccbddbe4761
https://hg.mozilla.org/integration/mozilla-inbound/rev/09f33ee98374

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9b3d8621fe58
https://hg.mozilla.org/mozilla-central/rev/eccbddbe4761
https://hg.mozilla.org/mozilla-central/rev/09f33ee98374

Comment 13

[[github robot]]

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)