Closed
Bug 1099592
Opened 10 years ago
Closed 9 years ago
Make JS callers of ios.newChannel call ios.newChannel2 in calendar/
Categories
(Calendar :: Security, defect)
Calendar
Security
Tracking
(Not tracked)
RESOLVED
FIXED
4.3
People
(Reporter: philip.chee, Assigned: ssitter)
References
Details
Attachments
(2 files, 4 obsolete files)
1006 bytes,
patch
|
Fallen
:
review-
|
Details | Diff | Splinter Review |
23.67 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
+++ 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:
I'm not really sure what it is doing, but I have done what https://bugzilla.mozilla.org/attachment.cgi?id=8527395 does.
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)
Comment 4•9 years ago
|
||
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.
Updated•9 years ago
|
Assignee: nobody → acelists
Status: NEW → ASSIGNED
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 6•9 years ago
|
||
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+
Sure, thanks.
Attachment #8538031 -
Attachment is obsolete: true
Attachment #8539596 -
Flags: review+
Keywords: checkin-needed
Comment 8•9 years ago
|
||
http://hg.mozilla.org/comm-central/rev/4ec1ad82f006
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 3.9
Comment 9•9 years ago
|
||
Backed out because of XPCShell failures: https://hg.mozilla.org/comm-central/rev/30915a36f815
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 14•9 years ago
|
||
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
Reporter | ||
Comment 15•9 years ago
|
||
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•9 years ago
|
||
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 hidden (Legacy TBPL/Treeherder Robot) |
Comment 18•9 years ago
|
||
Do we need this for 4.0, or is it just deprecated?
Flags: needinfo?(acelists)
Comment 19•9 years ago
|
||
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)
Reporter | ||
Comment 20•9 years ago
|
||
(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•9 years ago
|
||
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)
Reporter | ||
Comment 22•9 years ago
|
||
(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)
Comment 23•9 years ago
|
||
(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?
Reporter | ||
Comment 24•9 years ago
|
||
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•9 years ago
|
||
Looks like even newChannel2 is deprecated suddenly (bug 1125618) so this needs to be reworked :(
Comment 26•9 years ago
|
||
So let's try this, please run it through calendar tests.
Attachment #8539596 -
Attachment is obsolete: true
Attachment #8571512 -
Flags: review?(philipp)
Comment 27•9 years ago
|
||
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-
Assignee | ||
Comment 28•9 years ago
|
||
See Bug 1165315 Comment #2. Seems the deprecation warnings were now changed to assertions causing debug builds to fail.
Assignee | ||
Comment 29•9 years ago
|
||
> ###!!! 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•9 years ago
|
||
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
Assignee | ||
Comment 31•9 years ago
|
||
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 32•9 years ago
|
||
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+
Assignee | ||
Comment 33•9 years ago
|
||
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 → ---
Comment 34•9 years ago
|
||
Pushed to comm-central changeset cec8a41e067d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago → 9 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.
Description
•