Make sure loadinfo passed to NewChannel() calls is not ignored
Categories
(Thunderbird :: General, task)
Tracking
(Not tracked)
People
(Reporter: jorgk-bmo, Assigned: benc)
Details
Attachments
(1 file, 1 obsolete file)
2.66 KB,
patch
|
jorgk-bmo
:
review+
MakeMyDay
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•5 years ago
|
||
Sure thing. Looking at it now.
Assignee | ||
Comment 2•5 years ago
|
||
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!
Reporter | ||
Comment 3•5 years ago
|
||
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.
Reporter | ||
Comment 4•5 years ago
|
||
Comment on attachment 9045826 [details] [diff] [review] set-missing-loadInfo-1.patch MMD, please check the Calendar part.
Reporter | ||
Comment 5•5 years ago
|
||
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®exp=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®exp=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+ ;-)
Reporter | ||
Updated•5 years ago
|
Comment 6•5 years ago
|
||
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.
Reporter | ||
Comment 7•5 years ago
|
||
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®exp=false&path=
Comment 8•5 years ago
|
||
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.
Reporter | ||
Updated•5 years ago
|
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
Reporter | ||
Updated•5 years ago
|
Comment 10•5 years ago
|
||
(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.
Updated•5 years ago
|
Description
•