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

RESOLVED FIXED in 1.6

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: philip.chee, Assigned: aceman)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
+++ 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:
(Assignee)

Comment 1

4 years ago
Posted patch 1099588-IM.patch (obsolete) — Splinter Review
Attachment #8530969 - Flags: review?(florian)
(Assignee)

Comment 2

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

Comment 3

4 years ago
Posted patch 1099588-IM.patch v2 (obsolete) — Splinter Review
Adding the missing 8th argument, see bug 1099587 and bug 1099585.
Attachment #8530969 - Attachment is obsolete: true
Attachment #8530969 - Flags: review?(florian)
(Assignee)

Updated

4 years ago
Attachment #8530980 - Flags: review?(florian)
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 on attachment 8530980 [details] [diff] [review]
1099588-IM.patch v2

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

This fixes our current emoticon crash for me. aceman: does this need more work now that bug 1087442 has landed?
Attachment #8530980 - Flags: feedback+
Flags: needinfo?(acelists)
(Assignee)

Comment 6

4 years ago
I need to see if you use NetUtil.newChannel() somewhere. I plan to look at all the modules in a a few hours.
Flags: needinfo?(acelists)
(Assignee)

Comment 7

4 years ago
Posted patch patch v3 (obsolete) — Splinter Review
Please try this as I do not run IM tests.
Attachment #8530980 - Attachment is obsolete: true
Attachment #8530980 - Flags: review?(florian)
Attachment #8538024 - Flags: review?(florian)
Attachment #8538024 - Flags: feedback?(clokep)
Comment on attachment 8538024 [details] [diff] [review]
patch v3

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

rs=me if someone can confirm that this works :).
Attachment #8538024 - Flags: review?(florian) → review+

Comment 9

4 years ago
Comment on attachment 8538024 [details] [diff] [review]
patch v3

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

Added some drive-by comments.  I've only seriously looked at the smileProtocolHandler changes (since it causes crashes), and either form should fix the crashing part.

Thanks for working on this! :)

::: chat/components/src/smileProtocolHandler.js
@@ +34,5 @@
> +    let channel = Services.io.newChannel2(getSmileRealURI(smile), null, null, null,
> +                                          Services.scriptSecurityManager.getSystemPrincipal(),
> +                                          null,
> +                                          Ci.nsILoadInfo.SEC_NORMAL,
> +                                          Ci.nsIContentPolicy.TYPE_OTHER);

You probably want something like this instead:
    let uri = Services.io.newURI(getSmileRealURI(smile), null, null);
    let channel = Services.io.newChannelFromURIWithLoadInfo(uri, aLoadInfo);
(That method is new, but exists in today's nightly)

::: chat/protocols/xmpp/xmpp.jsm
@@ +1328,5 @@
> +    }).bind(this), null, null,
> +    Services.scriptSecurityManager.getSystemPrincipal(),
> +    null,
> +    Ci.nsILoadInfo.SEC_NORMAL,
> +    Ci.nsIContentPolicy.TYPE_OTHER);

Would that be TYPE_IMAGE rather than TYPE_OTHER here?
Flags: needinfo?(acelists)
(Assignee)

Comment 10

4 years ago
Posted patch patch v4Splinter Review
OK, thanks.
Flags: needinfo?(acelists)
Attachment #8539593 - Flags: review?(philipp)
(Assignee)

Updated

4 years ago
Attachment #8539593 - Flags: review?(philipp) → review?(florian)
Attachment #8538024 - Flags: feedback?(clokep)

Updated

4 years ago
Attachment #8538024 - Attachment is obsolete: true
Attachment #8539593 - Flags: review?(florian) → review+
Thanks! Sorry for taking so long to check this in. I changed the spacing slightly before committing this.

https://hg.mozilla.org/comm-central/rev/a466603a0dde
Assignee: nobody → acelists
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Depends on: 1122514
You need to log in before you can comment on or make changes to this bug.