Last Comment Bug 1099592 - Make JS callers of ios.newChannel call ios.newChannel2 in calendar/
: Make JS callers of ios.newChannel call ios.newChannel2 in calendar/
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Security (show other bugs)
: unspecified
: All All
-- normal (vote)
: 4.3
Assigned To: Stefan Sitter
:
:
Mentors:
Depends on:
Blocks: 1087720 1165315 1165497 1165728
  Show dependency treegraph
 
Reported: 2014-11-15 03:20 PST by Philip Chee
Modified: 2015-05-19 05:38 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
1099592-LT.patch (1.13 KB, patch)
2014-12-01 13:36 PST, :aceman
no flags Details | Diff | Splinter Review
1099592-LT.patch v2 (1.14 KB, patch)
2014-12-01 14:26 PST, :aceman
no flags Details | Diff | Splinter Review
patch v3 (1.36 KB, patch)
2014-12-17 12:25 PST, :aceman
philipp: review+
Details | Diff | Splinter Review
patch v3.1 (1.25 KB, patch)
2014-12-19 17:09 PST, :aceman
acelists: review+
Details | Diff | Splinter Review
patch v4 (1006 bytes, patch)
2015-03-02 11:48 PST, :aceman
philipp: review-
Details | Diff | Splinter Review
use newChannelFromURI2 in calendar/ (23.67 KB, patch)
2015-05-16 06:30 PDT, Stefan Sitter
philipp: review+
Details | Diff | Splinter Review

Description User image Philip Chee 2014-11-15 03:20:30 PST
+++ 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 User image :aceman 2014-12-01 13:36:27 PST
Created attachment 8530970 [details] [diff] [review]
1099592-LT.patch

Sorry, untested.
Comment 2 User image :aceman 2014-12-01 13:43:56 PST
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 User image :aceman 2014-12-01 14:26:24 PST
Created attachment 8530983 [details] [diff] [review]
1099592-LT.patch v2

Adding the missing 8th argument, see bug 1099587 and bug 1099585.
Comment 4 User image Christoph Kerschbaumer [:ckerschb] 2014-12-01 16:58:42 PST
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 User image :aceman 2014-12-17 12:25:16 PST
Created attachment 8538031 [details] [diff] [review]
patch v3

Please try it out as I do not run calendar tests.
Comment 6 User image Philipp Kewisch [:Fallen] 2014-12-19 04:01:52 PST
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 User image :aceman 2014-12-19 17:09:47 PST
Created attachment 8539596 [details] [diff] [review]
patch v3.1

Sure, thanks.
Comment 8 User image Richard Marti (:Paenglab) 2015-01-02 01:57:39 PST
http://hg.mozilla.org/comm-central/rev/4ec1ad82f006
Comment 9 User image Richard Marti (:Paenglab) 2015-01-02 04:05:57 PST
Backed out because of XPCShell failures:
https://hg.mozilla.org/comm-central/rev/30915a36f815
Comment 10 User image Treeherder Robot 2015-01-02 04:09:07 PST
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 User image Treeherder Robot 2015-01-02 04:09:08 PST
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 User image Treeherder Robot 2015-01-02 04:09:10 PST
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 User image Treeherder Robot 2015-01-02 04:09:12 PST
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 User image :aceman 2015-01-02 08:25:28 PST
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 User image Philip Chee 2015-01-02 08:45:53 PST
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 User image Philipp Kewisch [:Fallen] 2015-01-02 16:21:05 PST
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 User image Treeherder Robot 2015-01-04 03:02:23 PST
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 User image Philipp Kewisch [:Fallen] 2015-02-12 17:44:50 PST
Do we need this for 4.0, or is it just deprecated?
Comment 19 User image :aceman 2015-02-16 07:14:45 PST
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 User image Philip Chee 2015-02-16 07:43:46 PST
(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 User image :aceman 2015-02-16 10:10:04 PST
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 User image Philip Chee 2015-02-18 08:31:59 PST
(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 User image :aceman 2015-02-18 08:38:58 PST
(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 User image Philip Chee 2015-02-23 10:19:37 PST
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 User image :aceman 2015-03-02 11:37:53 PST
Looks like even newChannel2 is deprecated suddenly (bug 1125618) so this needs to be reworked :(
Comment 26 User image :aceman 2015-03-02 11:48:22 PST
Created attachment 8571512 [details] [diff] [review]
patch v4

So let's try this, please run it through calendar tests.
Comment 27 User image Philipp Kewisch [:Fallen] 2015-03-02 15:17:37 PST
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 User image Stefan Sitter 2015-05-15 07:52:05 PDT
See Bug 1165315 Comment #2. Seems the deprecation warnings were now changed to assertions causing debug builds to fail.
Comment 29 User image Stefan Sitter 2015-05-15 08:04:39 PDT
> ###!!! 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 User image :aceman 2015-05-15 13:33:43 PDT
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 User image Stefan Sitter 2015-05-16 06:30:39 PDT
Created attachment 8606633 [details] [diff] [review]
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 User image Philipp Kewisch [:Fallen] 2015-05-16 14:31:20 PDT
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 User image Stefan Sitter 2015-05-17 03:04:32 PDT
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 User image Philipp Kewisch [:Fallen] 2015-05-19 05:38:44 PDT
Pushed to comm-central changeset cec8a41e067d

Note You need to log in before you can comment on or make changes to this bug.