Closed Bug 1529509 Opened 5 years ago Closed 5 years ago

Make sure loadinfo passed to NewChannel() calls is not ignored

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 67.0

People

(Reporter: jorgk-bmo, Assigned: benc)

Details

Attachments

(1 file, 1 obsolete file)

As per bug 1452600 comment #16 we need to make sure that loadinfo is not ignored, for example here:

https://hg.mozilla.org/comm-central/rev/d8c70b022d838eb550dcea5c82708946f12e9d4b#l4.17

This changeset give as good overview over all the NewChannel() implementations in the system, some/most appear to ignore the loadinfo passed in.

This one here
https://hg.mozilla.org/comm-central/rev/d8c70b022d838eb550dcea5c82708946f12e9d4b#l2.20
doesn't pass the loadinfo on.

Ben, do you have some cycles to get this cleaned up? If in doubt, ask :Valentin for help.

Flags: needinfo?(benc)

Sure thing. Looking at it now.

A grep for newchannel turns up about 60 files in C-C.
Of those about 14 are NewChannel() implementations (the rest are callers).
And of those, only 2 looked like they weren't handling loadInfo properly.
Which hopefully, this patch sorts out!

Assignee: nobody → benc
Flags: needinfo?(benc)
Attachment #9045826 - Flags: review?(jorgk)

Thanks.

(In reply to Jorg K (GMT+1) from comment #0)

https://hg.mozilla.org/comm-central/rev/d8c70b022d838eb550dcea5c82708946f12e9d4b#l4.17
This changeset give as good overview over all the NewChannel() implementations in the system, some/most appear to ignore the loadinfo passed in.

This comment actually pointed to the implementations. I'll check it when the current bustage is over.

Comment on attachment 9045826 [details] [diff] [review]
set-missing-loadInfo-1.patch

MMD, please check the Calendar part.
Attachment #9045826 - Flags: review?(makemyday)

So let's look at the 12 implementations excluding suite/, BTW, all referenced in https://hg.mozilla.org/comm-central/rev/d8c70b022d838eb550dcea5c82708946f12e9d4b#l4.17:

Five in JS:
https://searchfox.org/comm-central/search?q=newChannel%3A&case=true&regexp=false&path=

https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/calendar/base/src/calProtocolHandler.js#30 - uses Services.io.newChannelFromURIWithLoadInfo()
https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/calendar/lightning/components/calItipProtocolHandler.js#65 - will be fixed in the patch
https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/chat/components/src/smileProtocolHandler.js#31 - uses Services.io.newChannelFromURIWithLoadInfo()
https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/ldap/xpcom/src/nsLDAPProtocolHandler.js#32 - will be fixed in the patch
https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/mailnews/compose/src/nsSMTPProtocolHandler.js#31 - not implemented

Seven in C++:
https://searchfox.org/comm-central/search?q=%3A%3ANewChannel(&case=true&regexp=false&path=

https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/mailnews/addrbook/src/nsAddbookProtocolHandler.cpp#122 - Seems OK, one code path doesn't use the loadinfo, but only if it's null.
https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/mailnews/base/src/nsCidProtocolHandler.cpp#55 - not implemented
https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/mailnews/compose/src/nsSmtpService.cpp#311 - same as AB.
https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/mailnews/imap/src/nsImapService.cpp#2642 - creates a "mock channel" and uses SetLoadInfo(aLoadInfo)
https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/mailnews/local/src/nsMailboxService.cpp#578 - uses SetLoadInfo(aLoadInfo);
https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/mailnews/local/src/nsPop3Service.cpp#466 - uses SetLoadInfo(aLoadInfo);
https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/mailnews/news/src/nsNntpService.cpp#1235 - uses SetLoadInfo(aLoadInfo);

So the patch should be r+ ;-)

Attachment #9045826 - Flags: review?(jorgk) → review+
Comment on attachment 9045826 [details] [diff] [review]
set-missing-loadInfo-1.patch

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

Thanks, r+ with the nit fixed.

::: calendar/lightning/components/calItipProtocolHandler.js
@@ +18,3 @@
>      this.wrappedJSObject = this;
>      this.URI = this.originalURI = URI;
> +    this.loadInfo = loadInfo;

Please make the argument aLoadInfo instead of loadInfo.
Attachment #9045826 - Flags: review?(makemyday) → review+

Hmm, looking at this again, I don't think we need the Calendar hunk. Or maybe we keep the value for future use. In any case, a property loadAttributes makes no sense at all:
https://searchfox.org/comm-central/search?q=loadAttributes&case=true&regexp=false&path=

Attachment #9046167 - Flags: review?(makemyday)
Comment on attachment 9046167 [details] [diff] [review]
set-missing-loadInfo-1.patch (JK)

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

::: calendar/lightning/components/calItipProtocolHandler.js
@@ +23,5 @@
>      QueryInterface: ChromeUtils.generateQI([Ci.nsIChannel, Ci.nsIRequest]),
>      classID: Components.ID("{643e0328-36f6-411d-a107-16238dff9cd7}"),
>  
>      contentType: ITIP_HANDLER_MIMETYPE,
> +    loadInfo: null,

You don't need to define loadInfo in the prototype, since it's always set if the channel is created by the passed argument above.

While loadAttributes could be removed here, the whole implementation doesn't seem to be really in sync with the interface definitions of nsIChannel and nsIRequest - lots of new methods/attributes. We should fix that in a separate calendar bug, I assume this is true for other implementaions, too.
Attachment #9046167 - Attachment is obsolete: true
Attachment #9046167 - Flags: review?(makemyday)

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fe9b16f59680
set loadInfo property in newChannel() for nsLDAPProtocolHandler and ItipProtocolHandler. r=jorgk,MakeMyDay

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 67.0

(In reply to [:MakeMyDay] from comment #8)

While loadAttributes could be removed here, the whole implementation doesn't
seem to be really in sync with the interface definitions of nsIChannel and
nsIRequest - lots of new methods/attributes. We should fix that in a
separate calendar bug, I assume this is true for other implementaions, too.

I filed bug 1530224 for that.

Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: