Closed Bug 1099585 Opened 5 years ago Closed 4 years ago

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

Categories

(SeaMonkey :: Security, defect)

defect
Not set

Tracking

(seamonkey2.38 fixed)

RESOLVED FIXED
seamonkey2.38
Tracking Status
seamonkey2.38 --- fixed

People

(Reporter: philip.chee, Assigned: ewong)

References

Details

Attachments

(2 files, 9 obsolete files)

(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:
Attached patch patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #8527127 - Flags: review?(neil)
Comment on attachment 8527127 [details] [diff] [review]
patch (v1)

rs=me on any changes needed to keep tests working.
Comment on attachment 8527127 [details] [diff] [review]
patch (v1)

(would help if I clicked the right button...)
Attachment #8527127 - Flags: review?(neil) → review+
Attached patch patch (v2) (obsolete) — Splinter Review
Missed some other instances of newchannel.
Attachment #8527127 - Attachment is obsolete: true
Attachment #8527395 - Flags: review?(neil)
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.
Attachment #8527395 - Flags: review?(neil)
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".
(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.
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.
Yes, it seems bug 1095798 will add the needed newChannel2() to NetUtil.jsm.
Depends on: 1095798, 1087442
(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++.
Depends on: 1125618
Depends on: 1115572
Attached patch proposed patch (v3) (obsolete) — Splinter Review
Attachment #8527395 - Attachment is obsolete: true
Attachment #8571335 - Flags: review?(neil)
Does this consider changes to the API again in bug 1125618 ?
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.]
Attached patch proposed patch (v4) (obsolete) — Splinter Review
updating patch first.
Attachment #8571335 - Attachment is obsolete: true
Attachment #8571335 - Flags: review?(neil)
Attached patch proposed patch (v4) (obsolete) — Splinter Review
Attachment #8573272 - Attachment is obsolete: true
Attachment #8573278 - Flags: review?(neil)
> +++ 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!
Attached patch proposed patch (v2) (obsolete) — Splinter Review
Attachment #8573278 - Attachment is obsolete: true
Attachment #8573278 - Flags: review?(neil)
Attachment #8609132 - Flags: review?(neil)
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).
Attachment #8609132 - Flags: review?(neil) → review-
(In reply to neil@parkwaycc.co.uk 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.
Attached patch proposed patch (v3) (obsolete) — Splinter Review
Attachment #8609132 - Attachment is obsolete: true
Attachment #8611189 - Flags: review?(neil)
Attached patch proposed patch (v4) (obsolete) — Splinter Review
minor nit change.
Attachment #8611189 - Attachment is obsolete: true
Attachment #8611189 - Flags: review?(neil)
Attachment #8614466 - Flags: review?(neil)
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.
Attachment #8614466 - Flags: review?(neil) → review-
Attachment #8614466 - Attachment is obsolete: true
Attachment #8616534 - Flags: review?(neil)
Attachment #8616534 - Flags: review?(neil) → review+
Comment on attachment 8616534 [details] [diff] [review]
proposed patch (v5)

a=me for CLOSED TREE SeaMonkey
Comment on attachment 8616534 [details] [diff] [review]
proposed patch (v5)

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/0fc1f876fec1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.38
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...
(In reply to neil@parkwaycc.co.uk 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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch proposed patch fix(v1) (obsolete) — Splinter Review
Attachment #8635921 - Flags: review?(neil)
Attachment #8635921 - Flags: review?(neil)
Attached patch proposed fix(v1)Splinter Review
Attachment #8635921 - Attachment is obsolete: true
Attachment #8635923 - Flags: review?(neil)
Status: REOPENED → ASSIGNED
Attachment #8635923 - Flags: review?(neil) → review+
Comment on attachment 8635923 [details] [diff] [review]
proposed fix(v1)

a=me for comm-central CLOSED TREE SeaMonkey
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 on attachment 8635923 [details] [diff] [review]
proposed fix(v1)

Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/9ff406085529
Status: ASSIGNED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.