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

Status: RESOLVED FIXED

(Calendar :: Security, defect)

Description

[Philip Chee]

+++ 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:

Comment 1

[:aceman]

Attachment: 1099592-LT.patch

Sorry, untested.

Comment 2

[:aceman]

I'm not really sure what it is doing, but I have done what https://bugzilla.mozilla.org/attachment.cgi?id=8527395 does.

Comment 3

[:aceman]

Attachment: 1099592-LT.patch v2

Adding the missing 8th argument, see bug 1099587 and bug 1099585.

Comment 4

[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 5

[:aceman]

Attachment: patch v3

Please try it out as I do not run calendar tests.

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

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();

Comment 7

[:aceman]

Attachment: patch v3.1

Sure, thanks.

Comment 8

[Richard Marti (:Paenglab)]

http://hg.mozilla.org/comm-central/rev/4ec1ad82f006

Comment 9

[Richard Marti (:Paenglab)]

Backed out because of XPCShell failures:
https://hg.mozilla.org/comm-central/rev/30915a36f815

Comment 10

[Legacy TBPL/Treeherder Robot]

submit_timestamp: 2015-01-02T04:09:06
log: https://treeherder.mozilla.org/ui/logviewer.html#?repo=comm-central&job_id=6295
repository: comm-central
who: richard[dot]marti[at]gmail[dot]com
machine: tst-linux64-spot-196
buildname: Ubuntu VM 12.04 x64 comm-central opt test xpcshell
revision: 4ec1ad82f006

TEST-UNEXPECTED-FAIL | calendar/test/unit/test_webcal.js | xpcshell return code: -11
PROCESS-CRASH | calendar/test/unit/test_webcal.js | application crashed [@ nsIOService::NewChannelFromURIWithProxyFlagsInternal(nsIURI*, nsIURI*, unsigned int, nsILoadInfo*, nsIChannel**)]
Return code: 1

Comment 11

[Legacy TBPL/Treeherder Robot]

submit_timestamp: 2015-01-02T04:09:06
log: https://treeherder.mozilla.org/ui/logviewer.html#?repo=comm-central&job_id=6293
repository: comm-central
who: richard[dot]marti[at]gmail[dot]com
machine: tst-linux32-spot-752
buildname: Ubuntu VM 12.04 comm-central opt test xpcshell
revision: 4ec1ad82f006

TEST-UNEXPECTED-FAIL | calendar/test/unit/test_webcal.js | xpcshell return code: -11
PROCESS-CRASH | calendar/test/unit/test_webcal.js | application crashed [@ nsIOService::NewChannelFromURIWithProxyFlagsInternal(nsIURI*, nsIURI*, unsigned int, nsILoadInfo*, nsIChannel**)]
Return code: 1

Comment 12

[Legacy TBPL/Treeherder Robot]

submit_timestamp: 2015-01-02T04:09:06
log: https://treeherder.mozilla.org/ui/logviewer.html#?repo=comm-central&job_id=6315
repository: comm-central
who: richard[dot]marti[at]gmail[dot]com
machine: t-xp32-ix-013
buildname: TB Windows XP 32-bit comm-central opt test xpcshell
revision: 4ec1ad82f006

TEST-UNEXPECTED-FAIL | calendar/test/unit/test_webcal.js | xpcshell return code: 1
PROCESS-CRASH | calendar/test/unit/test_webcal.js | application crashed [@ nsIOService::NewChannelFromURIWithProxyFlagsInternal(nsIURI *,nsIURI *,unsigned int,nsILoadInfo *,nsIChannel * *)]
Return code: 1

Comment 13

[Legacy TBPL/Treeherder Robot]

submit_timestamp: 2015-01-02T04:09:06
log: https://treeherder.mozilla.org/ui/logviewer.html#?repo=comm-central&job_id=6297
repository: comm-central
who: richard[dot]marti[at]gmail[dot]com
machine: tst-linux64-spot-150
buildname: Ubuntu VM 12.04 x64 comm-central debug test xpcshell
revision: 4ec1ad82f006

TEST-UNEXPECTED-FAIL | calendar/test/unit/test_webcal.js | xpcshell return code: -11
PROCESS | 731 | Assertion failure: loadInfo, at /builds/slave/tb-c-cen-l64-d-000000000000000/build/mozilla/netwerk/base/src/nsIOService.cpp:687
PROCESS-CRASH | calendar/test/unit/test_webcal.js | application crashed [@ nsIOService::NewChannelFromURIWithProxyFlagsInternal(nsIURI*, nsIURI*, unsigned int, nsILoadInfo*, nsIChannel**)]
Return code: 1

Comment 14

[:aceman]

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

Comment 15

[Philip Chee]

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;
>    },

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

I'm surprised this is actually crashing the process, you may want to also file a core bug to not make it do that.

Comment 17

[Legacy TBPL/Treeherder Robot]

submit_timestamp: 2015-01-04T03:02:22
log: https://treeherder.mozilla.org/ui/logviewer.html#?repo=comm-central&job_id=6301
repository: comm-central
who: mozilla[at]kewis[dot]ch
machine: tst-linux32-spot-088
buildname: Ubuntu VM 12.04 comm-central debug test xpcshell
revision: 4ec1ad82f006

TEST-UNEXPECTED-FAIL | calendar/test/unit/test_webcal.js | xpcshell return code: -11
PROCESS | 21455 | Assertion failure: loadInfo, at /builds/slave/tb-c-cen-lx-d-0000000000000000/build/mozilla/netwerk/base/src/nsIOService.cpp:687
PROCESS-CRASH | calendar/test/unit/test_webcal.js | application crashed [@ nsIOService::NewChannelFromURIWithProxyFlagsInternal(nsIURI*, nsIURI*, unsigned int, nsILoadInfo*, nsIChannel**)]
Return code: 1

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

Do we need this for 4.0, or is it just deprecated?

Comment 19

[:aceman]

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 ?

Comment 20

[Philip Chee]

(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;
  },

Comment 21

[:aceman]

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?

Comment 22

[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()

Comment 23

[:aceman]

(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?

Comment 24

[Philip Chee]

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().

Comment 25

[:aceman]

Looks like even newChannel2 is deprecated suddenly (bug 1125618) so this needs to be reworked :(

Comment 26

[:aceman]

Attachment: patch v4

So let's try this, please run it through calendar tests.

Comment 27

[Philipp Kewisch [:Fallen][📱📆]]

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.

Comment 28

[Stefan Sitter [:ssitter]]

See Bug 1165315 Comment #2. Seems the deprecation warnings were now changed to assertions causing debug builds to fail.

Comment 29

[Stefan Sitter [:ssitter]]

> ###!!! 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/

Comment 30

[:aceman]

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.

Comment 31

[Stefan Sitter [:ssitter]]

Attachment: use newChannelFromURI2 in calendar/

This patch fixes the assertions for me and unit tests pass if the patch from bug 1165002 is applied too.

Comment 32

[Philipp Kewisch [:Fallen][📱📆]]

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.

Comment 33

[Stefan Sitter [:ssitter]]

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.

Comment 34

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central changeset cec8a41e067d