1099585 - Make JS callers of ios.newChannel call ios.newChannel2 in suite/

Status: RESOLVED FIXED

(SeaMonkey :: Security, defect)

Description

[Philip Chee]

(From Bug 1087720 comment #0)
> We have to change all JS callers of ioservice.newChannel to call
> ioservice.newChannel2. Since there are so many, we categorize and split them
> into different sub-bugs and use this bug as a tracking bug:

Comment 1

[Edmund Wong (:ewong)]

Attachment: patch (v1)

Comment 2

[Philip Chee]

You missed out a few?
http://mxr.mozilla.org/comm-central/search?string=.newChannel%28&find=%2Fsuite%2F&findi=&filter=^%5B^\0%5D*%24&hitlimit=&tree=comm-central

Comment 3

[[e-mail]]

Comment on attachment 8527127 [details] [diff] [review]
patch (v1)

rs=me on any changes needed to keep tests working.

Comment 4

[[e-mail]]

Comment on attachment 8527127 [details] [diff] [review]
patch (v1)

(would help if I clicked the right button...)

Comment 5

[Edmund Wong (:ewong)]

Attachment: patch (v2)

Missed some other instances of newchannel.

Comment 6

[[e-mail]]

Comment on attachment 8527395 [details] [diff] [review]
patch (v2)

I don't think these are right but this is all ported code so I'd have to wait for the Firefox versions to be updated to be sure.

Comment 7

[:aceman]

It appears to me newChannel() takes 8 arguments. How do you get away with 7? I followed this patch in bug 1099587 but it is not working. I get test failures, some of them really having exceptions about "too few arguments".

Comment 8

[Edmund Wong (:ewong)]

(In reply to :aceman from comment #7)
> It appears to me newChannel() takes 8 arguments. How do you get away with 7?
> I followed this patch in bug 1099587 but it is not working. I get test
> failures, some of them really having exceptions about "too few arguments".

My bad.  It takes 9 arguments.

+
+NS_IMETHODIMP
+nsIOService::NewChannel2(const nsACString& aSpec,
+                         const char* aCharset,
+                         nsIURI* aBaseURI,
+                         nsIDOMNode* aLoadingNode,
+                         nsIPrincipal* aLoadingPrincipal,
+                         nsIPrincipal* aTriggeringPrincipal,
+                         uint32_t aSecurityFlags,
+                         uint32_t aContentPolicyType,
+                         nsIChannel** result)

which is probably why it's not working.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

I strongly encourage you to wait till Bug 1087442 lands which has information/documentation about all of these 8 arguments. You then also have to convert NetUtil.newChannel() calls. Bug 1087442 should hopefully make it into the codebase this week.

Please note, there should only be 8 arguments in JS.
"NSIChannel** result" is the resulting outgoing argument.

Comment 10

[:aceman]

Yes, it seems bug 1095798 will add the needed newChannel2() to NetUtil.jsm.

Comment 11

[[e-mail]]

(In reply to Edmund Wong from comment #8)
> My bad.  It takes 9 arguments.
> 
> +
> +NS_IMETHODIMP
> +nsIOService::NewChannel2(const nsACString& aSpec,
> +                         const char* aCharset,
> +                         nsIURI* aBaseURI,
> +                         nsIDOMNode* aLoadingNode,
> +                         nsIPrincipal* aLoadingPrincipal,
> +                         nsIPrincipal* aTriggeringPrincipal,
> +                         uint32_t aSecurityFlags,
> +                         uint32_t aContentPolicyType,
> +                         nsIChannel** result)

You should read the .idl, not the C++.

Comment 12

[Edmund Wong (:ewong)]

Attachment: proposed patch (v3)

Comment 13

[:aceman]

Does this consider changes to the API again in bug 1125618 ?

Comment 14

[[e-mail]]

Comment on attachment 8571335 [details] [diff] [review]
proposed patch (v3)

>+    var channel = Services.io.newChannel2(linkURL, null, null, null,
>+                                          Services.scriptSecurityManager.getSystemPrincipal(),
This needs to be this.target.nodePrincipal

>   newChannel: function(aURI) {
>     return this.newChannel2(aURI, null);
>   },
> 
>   newChannel2: function(aURI, aLoadInfo) {
Eek! This is already wrong, aLoadInfo is a parmeter on newChannel.

>     var chan = loadinfo ? ios.newChannelFromURIWithLoadInfo(newURI, loadinfo) :
>-                          ios.newChannelFromURI(newURI);
>+                          ios.newChannelFromURI2(newURI, null,
>+                                                 principal,
Not sure about this, I think the principal might be null here.

>+    var channel = Services.io.newChannel2(FEEDHANDLER_URI, null, null, null,
>+                                          Services.scriptSecurityManager.getSystemPrincipal(),
[Not that it matters, but Firefox uses the null principal here.]

Comment 15

[Edmund Wong (:ewong)]

Attachment: proposed patch (v4)

updating patch first.

Comment 16

[Edmund Wong (:ewong)]

Attachment: proposed patch (v4)

Comment 17

[Philip Chee]

> +++ b/suite/common/nsContextMenu.js
>      // set up a channel to do the saving
> -    var channel = Services.io.newChannel(linkURL, null, null);
> +    var channel = Services.io.newChannel2(linkURL, null, null, null,
> +                                          Services.scriptSecurityManager.getSystemPrincipal(),
> +                                          null,
> +                                          Components.interfaces.nsILoadInfo.SEC_NORMAL,

This is the Firefox version:
>     // set up a channel to do the saving
>     var ioService = Cc["@mozilla.org/network/io-service;1"].
>                     getService(Ci.nsIIOService);
>     var channel = ioService.newChannelFromURI2(makeURI(linkURL),
>                                                null, // aLoadingNode
>                                                this.principal, // aLoadingPrincipal
>                                                null, // aTriggeringPrincipal
>                                                Ci.nsILoadInfo.SEC_NORMAL,
>                                                Ci.nsIContentPolicy.TYPE_OTHER);


Need to update newChannelFromURI() to newChannelFromURI2() as well. See:
Bug 1099587 - Make JS callers of ios.newChannel call ios.newChannel2 in mail/ and mailnews/
http://hg.mozilla.org/comm-central/rev/322e381cce2c#l3.1

For FeedWriter.js and FeedConverter.js please see:
Bug 1116278 - Make JS callers of ios.newChannel call ios.newChannel2 in browser/components/feeds
https://hg.mozilla.org/mozilla-central/rev/f15991d2f3b3
https://hg.mozilla.org/mozilla-central/rev/fe823d167766

> +++ b/suite/feeds/src/FeedWriter.js
> -    var resolvedURI = Services.io.newChannel(FEEDHANDLER_URI, null, null).URI;
> +    var channel = Services.io.newChannel2(FEEDHANDLER_URI, null, null, null,
> +                                          nullPrincipal, null,
You forgot to define nullPrincipal!

Comment 18

[Edmund Wong (:ewong)]

Attachment: proposed patch (v2)

Comment 19

[[e-mail]]

Comment on attachment 8609132 [details] [diff] [review]
proposed patch (v2)

>-      var ioService = Services.io;
>+      var ios = Services.io;
Why this change?

>+      var channel = ios.newChannelFromURI2(targetUrl, null,
>+                                          Services.scriptSecurityManager.getSystemPrincipal(),
Nit: indentation is wrong (also somewhere else)

>-    var channel = Services.io.newChannel(linkURL, null, null);
>+    var channel = ios.newChannelFromURI2(linkURL, null,
newChannelFromURI2 doesn't replace newChannel, newChannel2 does (also somewhere else)

>-        var channel = Services.io.newChannel(prereq_file, null, null);
>+        var ios = Services.io;
>+        var channel = ios.newChannelFromURI2(prereq_file, null,
This doesn't really make the line significantly shorter...

>-                  Services.io.newChannelFromURI(newURI);
>+                  Services.io.newChannelFromURI2(newURI);
newChannelFromURI2 is not a direct replacement for newChennelFromURI.

>-    var ios = Components.classes["@mozilla.org/network/io-service;1"]
>-                        .getService(Components.interfaces.nsIIOService);
>+    var ios = Services.io;
Services hasn't been loaded.

>+        chromeChannel = Services.io.newChannelFromURIWithLoadInfo(chromeURI, null);
You need to pass in the load info from the original request (QI to nsIChannel).

Comment 20

[Edmund Wong (:ewong)]

(In reply to [e-mail] from comment #19)
> Comment on attachment 8609132 [details] [diff] [review]
> proposed patch (v2)
> 
> >-      var ioService = Services.io;
> >+      var ios = Services.io;
> Why this change?
> 
mainly to shorten the newChannelFromURI2 even more.

Comment 21

[Edmund Wong (:ewong)]

Attachment: proposed patch (v3)

Comment 22

[Edmund Wong (:ewong)]

Attachment: proposed patch (v4)

minor nit change.

Comment 23

[[e-mail]]

Comment on attachment 8614466 [details] [diff] [review]
proposed patch (v4)

>-        let imageCache = Components.classes["@mozilla.org/image/tools;1"]
>-                                   .getService(Components.interfaces.imgITools)
>-                                   .getImgCacheForDocument(doc)
>+        let imageCache = Components.classes["@mozilla.org/image/tools;1"]
>+                                   .getService(Components.interfaces.imgITools)
>+                                   .getImgCacheForDocument(doc)
As you're fixing the line endings anyway, night as well add in the missing semicolon on that last line.

>       if (result.doc) {
> 
>         // Store the result in the result service so that the display
>         // page can access it.
>         feedService.addFeedResult(result);
> 
>         // Now load the actual XUL document.
>         var chromeURI = Services.io.newURI(FEEDHANDLER_URI, null, null);
>-        chromeChannel = Services.io.newChannelFromURI(chromeURI, null);
>+        var oldChannel = this._request.QueryInterface(Components.interfaces.nsIChannel);
>+        var loadInfo = oldChannel.loadInfo;
>+        chromeChannel = Services.io.newChannelFromURIWithLoadInfo(chromeURI, loadInfo);
>         chromeChannel.owner = Services.scriptSecurityManager
>                                       .getNoAppCodebasePrincipal(chromeURI);
>         chromeChannel.originalURI = result.uri;
>       }
>       else
>-        chromeChannel = Services.io.newChannelFromURI(result.uri, null);
>+        chromeChannel = Services.io.newChannelFromURIWithLoadInfo(result.uri, loadInfo);
Almost but not quite: you need to set loadInfo earlier so that it will work here.

Comment 24

[Edmund Wong (:ewong)]

Attachment: proposed patch (v5)

Comment 25

[Philip Chee]

Comment on attachment 8616534 [details] [diff] [review]
proposed patch (v5)

a=me for CLOSED TREE SeaMonkey

Comment 26

[Edmund Wong (:ewong)]

Comment on attachment 8616534 [details] [diff] [review]
proposed patch (v5)

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/0fc1f876fec1

Comment 27

[[e-mail]]

Comment on attachment 8616534 [details] [diff] [review]
proposed patch (v5)

>-      var channel = ioService.newChannel(targetUrl, null, null);
>+      var channel = ios.newChannel2(targetUrl, null, null, null,
...
>-    var resolvedURI = Services.io.newChannel(FEEDHANDLER_URI, null, null).URI;
>+    var ios = Services.io;
>+    var channel = ios.newChannelFromURI2(FEEDHANDLER_URI, null,
Oops, this line was still wrong, it should have been newChannel2...

Comment 28

[Edmund Wong (:ewong)]

(In reply to [e-mail] from comment #27)
> Comment on attachment 8616534 [details] [diff] [review]
> proposed patch (v5)
> 
> >-      var channel = ioService.newChannel(targetUrl, null, null);
> >+      var channel = ios.newChannel2(targetUrl, null, null, null,
> ...
> >-    var resolvedURI = Services.io.newChannel(FEEDHANDLER_URI, null, null).URI;
> >+    var ios = Services.io;
> >+    var channel = ios.newChannelFromURI2(FEEDHANDLER_URI, null,
> Oops, this line was still wrong, it should have been newChannel2...

oh dang. Sorry.  will get a fix for review

Comment 29

[Edmund Wong (:ewong)]

Attachment: proposed patch fix(v1)

Comment 30

[Edmund Wong (:ewong)]

Attachment: proposed fix(v1)

Comment 31

[Philip Chee]

Comment on attachment 8635923 [details] [diff] [review]
proposed fix(v1)

a=me for comm-central CLOSED TREE SeaMonkey

Comment 32

[Philip Chee]

Comment on attachment 8635923 [details] [diff] [review]
proposed fix(v1)

(In reply to Philip Chee from comment #31)
> Comment on attachment 8635923 [details] [diff] [review]
> proposed fix(v1)

> a=me for comm-central CLOSED TREE SeaMonkey
a=me for comm-aurora CLOSED TREE SeaMonkey

Comment 33

[Edmund Wong (:ewong)]

Comment on attachment 8635923 [details] [diff] [review]
proposed fix(v1)

Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/9ff406085529

Comment 34

[Edmund Wong (:ewong)]

Comment on attachment 8635923 [details] [diff] [review]
proposed fix(v1)

Pushed to comm-aurora:
https://hg.mozilla.org/releases/comm-aurora/rev/07458a671842