Closed Bug 1099592 Opened 5 years ago Closed 4 years ago

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

Categories

(Calendar :: Security, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: philip.chee, Assigned: ssitter)

References

Details

Attachments

(2 files, 4 obsolete files)

+++ This bug was initially created as a clone of Bug #1099585 +++

(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 1099592-LT.patch (obsolete) — Splinter Review
Sorry, untested.
Attachment #8530970 - Flags: review?(philipp)
I'm not really sure what it is doing, but I have done what https://bugzilla.mozilla.org/attachment.cgi?id=8527395 does.
Attached patch 1099592-LT.patch v2 (obsolete) — Splinter Review
Adding the missing 8th argument, see bug 1099587 and bug 1099585.
Attachment #8530970 - Attachment is obsolete: true
Attachment #8530970 - Flags: review?(philipp)
Attachment #8530983 - Flags: review?(philipp)
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.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attached patch patch v3 (obsolete) — Splinter Review
Please try it out as I do not run calendar tests.
Attachment #8530983 - Attachment is obsolete: true
Attachment #8530983 - Flags: review?(philipp)
Attachment #8538031 - Flags: review?(philipp)
Comment on attachment 8538031 [details] [diff] [review]
patch v3

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

r=philipp with this change:

::: calendar/test/unit/test_webcal.js
@@ +36,4 @@
>          do_check_true(Components.isSuccessCode(status));
>          run_next_test();
> +    }, null, null,
> +    Services.scriptSecurityManager.getSystemPrincipal(),

Can you use these variables to trim down the line lengths and make it a bit more readable?

const SEC_NORMAL = Ci.nsILoadInfo.SEC_NORMAL;
const TYPE_OTHER = Ci.nsIContentPolicy.TYPE_OTHER;
let principal = Services.scriptSecurityManager.getSystemPrincipal();
Attachment #8538031 - Flags: review?(philipp) → review+
Attached patch patch v3.1 (obsolete) — Splinter Review
Sure, thanks.
Attachment #8538031 - Attachment is obsolete: true
Attachment #8539596 - Flags: review+
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/4ec1ad82f006
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 3.9
Backed out because of XPCShell failures:
https://hg.mozilla.org/comm-central/rev/30915a36f815
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Maybe we need to play with loadInfo at http://mxr.mozilla.org/comm-central/source/calendar/base/src/calProtocolHandler.js#45, e.g. like Services.io.newChannelFromURIWithLoadInfo(uri, aLoadInfo), see http://mxr.mozilla.org/comm-central/source/chat/components/src/smileProtocolHandler.js#32
In bug 1115572 I am doing it this way:
> -  newChannel: function(aURI, aLoadInfo) {
> +  newChannel: function(aURI) {
> +    return this.newChannel2(aURI, null);
> +  },
> +
> +  newChannel2: function(aURI, aLoadInfo) {
>      var module = this.getModule(aURI);
> -    var channel = Services.io.newChannel(this[module + "URI"], null, null);
> +    var newURI = Services.io.newURI(this[module + "URI"], null, null);
> +    var channel = aLoadInfo ?
> +                  Services.io.newChannelFromURIWithLoadInfo(newURI, aLoadInfo) :
> +                  Services.io.newChannelFromURI(newURI);
>      channel.originalURI = aURI;
>      if (this[module + "Flags"] & UNTRUSTED)
>        channel.owner = null;
>      return channel;
>    },
I'm surprised this is actually crashing the process, you may want to also file a core bug to not make it do that.
Do we need this for 4.0, or is it just deprecated?
Flags: needinfo?(acelists)
I think it is currently deprecated because not all users have migrated. I assume they will remove the old functions (without 2 suffix) when possible. So they are waiting for it, see https://bugzilla.mozilla.org/show_bug.cgi?id=1087720#c8 . Maybe it won't happen in time for 38.

Should we try comment 15 ?
Flags: needinfo?(acelists)
(In reply to :aceman from comment #19)
> Should we try comment 15 ?
Currently comment 15 is:

  newChannel: function(aURI) {
    return this.newChannel2(aURI, null);
  },

  newChannel2: function(aURI, aLoadInfo) {
    var module = this.getModule(aURI);
    var newURI = Services.io.newURI(this[module + "URI"], null, null);
    var channel = aLoadInfo ?
                  Services.io.newChannelFromURIWithLoadInfo(newURI, aLoadInfo) :
                  Services.io.newChannelFromURI2(newURI,
                                                 null,      // aLoadingNode
                                                 Services.scriptSecurityManager.getSystemPrincipal(),
                                                 null,      // aTriggeringPrincipal
                                                 Components.interfaces.nsILoadInfo.SEC_NORMAL,
                                                 Components.interfaces.nsIContentPolicy.TYPE_OTHER);


    channel.originalURI = aURI;
    if (this[module + "Flags"] & UNTRUSTED)
      channel.owner = null;
    return channel;
  },
Somehow that looks similar to what my patch does (there is no loadinfo).
How do you then pass that channel to Services.io.newChannel2 ? Do you also use NetUtil.asyncFetch2 somewhere down the line?
Flags: needinfo?(philip.chee)
(In reply to :aceman from comment #21)
> Somehow that looks similar to what my patch does (there is no loadinfo).
Your patch looks reasonable to me.

> How do you then pass that channel to Services.io.newChannel2 ? Do you also
> use NetUtil.asyncFetch2 somewhere down the line?
Based on the patches that have landed on mozilla-central, I believe that we should convert from NetUtil.asyncFetch() to NetUtil.asyncFetch2()
Flags: needinfo?(philip.chee)
(In reply to Philip Chee from comment #22)
> (In reply to :aceman from comment #21)
> > Somehow that looks similar to what my patch does (there is no loadinfo).
> Your patch looks reasonable to me.
And it doesn't work :( Maybe they fixed the m-c code that it will no longer crash?

> > How do you then pass that channel to Services.io.newChannel2 ? Do you also
> > use NetUtil.asyncFetch2 somewhere down the line?
> Based on the patches that have landed on mozilla-central, I believe that we
> should convert from NetUtil.asyncFetch() to NetUtil.asyncFetch2()
I already do it in the patch, but I have not seen how/where you do it in SM. Do you have a sample?
Looking at the source of NetUtil.jsm
http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/NetUtil.jsm
It seems that asyncFetch2 is deprecated.

In /suite/ there is only one call to NetUtil.asyncFetch() and since we are passing a nsIChannel in the first argument we don't need to change this to asyncFetch2().
Looks like even newChannel2 is deprecated suddenly (bug 1125618) so this needs to be reworked :(
Attached patch patch v4Splinter Review
So let's try this, please run it through calendar tests.
Attachment #8539596 - Attachment is obsolete: true
Attachment #8571512 - Flags: review?(philipp)
Comment on attachment 8571512 [details] [diff] [review]
patch v4

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

This doesn't work for me, I get ERROR NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments [nsIIOService.newChannel]. Looking at the IDL interface I don't see that you can pass the parameters as an object.
Attachment #8571512 - Flags: review?(philipp) → review-
See Bug 1165315 Comment #2. Seems the deprecation warnings were now changed to assertions causing debug builds to fail.
> ###!!! ASSERTION: Deprecated, use NewChannel2 providing loadInfo arguments!
> ###!!! ASSERTION: Deprecated, use NewChannelFromURI2 providing loadInfo arguments!

In addition to the one file touched by the patch above I think the following places have to be fixed too: https://mxr.mozilla.org/comm-central/search?string=NewChannelFromURI&find=/calendar/
Blocks: 1165315
Looks like this is getting messy. Now suddenly NewChannel2 is no longer deprecated again? They should make up their minds. I can't blindly work on this without building calendar. Anybody having calendar building please take this.
Assignee: acelists → nobody
This patch fixes the assertions for me and unit tests pass if the patch from bug 1165002 is applied too.
Attachment #8606633 - Flags: review?(philipp)
Comment on attachment 8606633 [details] [diff] [review]
use newChannelFromURI2 in calendar/

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

Looks good to me, r=philipp. Do we need this on aurora? a=me if so.
Attachment #8606633 - Flags: review?(philipp) → review+
I'm not sure if this is required on comm-aurora. Once Bug 1165002 has landed we can check the aurora builders if they show the same failure as in Bug 1165315 or Bug 1165497.
Assignee: nobody → ssitter
Blocks: 1165497
Status: REOPENED → ASSIGNED
Keywords: checkin-needed
Target Milestone: 3.9 → ---
Blocks: 1165728
Pushed to comm-central changeset cec8a41e067d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.3
You need to log in before you can comment on or make changes to this bug.