Closed Bug 1528662 Opened 3 years ago Closed 3 years ago

Clean-up after bug 1452600

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 67.0

People

(Reporter: benc, Assigned: benc)

Details

Attachments

(2 files)

(This is technical debt follow-up arising from Bug 1452600 - low priority, but I wanted to get my notes down for the next time anyone touches this code)

Currently, a bunch of nsIStreamListener callback implementations in TB rely on being able to get the URI from callback parameters.

Before Bug 1452600, we mostly used the context parameter, this seems to be on the way out (M-C already removed it from Open/AsyncOpen() and are looking at removing it from the callbacks, too. See Bug 1525319).

The fix we used was to QI the nsIRequest request parameter into a nsIChannel, which carries a URI on it we can use instead.

However, I'd argue that this is probably working around something more fundamental - the OnStartRequest and OnStopRequest callbacks actually belong to nsIRequestObserver so there's no real expectation that request param must be a channel.
And, in fact, when using nsInputStreamPump, the callbacks will be invoked with a non-channel nsIRequest.

So, if the nsIStreamListener needs the URI, it should already know it by the time the callbacks are triggered (eg it could be set up with it by whatever calls AsyncOpen()).

There are four places that use this QI nsIRequest -> nsIChannel trick:

$ grep -lr "error QI nsIRequest to nsIChannel failed" comm
comm/mailnews/base/src/nsMsgFolderCompactor.cpp
comm/mailnews/base/src/nsCopyMessageStreamListener.cpp
comm/mailnews/base/util/nsMsgMailNewsUrl.cpp
comm/mailnews/local/src/nsParseMailbox.cpp

Truth be told, I don't think this is really worth the effort at the moment, but I wanted to make sure there was a record somewhere!

This adds some comments in places where we rely on getting the URI from the nsIStreamListener callback parameters. The comments point to this bug.

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

Spotted a few places where there were QI-to-nsIChannel calls where the type was already known at compile time.

Didn't do a try build (I think there'll be more important try runs going after the recent bustage), but passes the comm unittests for me in a local xpcshell-test.

Attachment #9044751 - Flags: review?(jorgk)
Comment on attachment 9044744 [details] [diff] [review]
document-streamlistener-kludges-1.patch

OK, thanks.
Attachment #9044744 - Flags: review?(jorgk) → review+
Comment on attachment 9044751 [details] [diff] [review]
remove-superflous-qi-1.patch

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

I'm happy to admit that I added whatever compiled in a hurry, so since those protocols all implement `nsIChannel` I'm happy to lose it again. However, the stuff I didn't add doesn't need to be touched up. I'm referring to the static casts you added.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ -9913,5 @@
> -  QueryInterface(NS_GET_IID(nsIChannel), getter_AddRefs(channel));
> -  if (channel) {
> -    channel->GetURI(getter_AddRefs(uri));
> -    m_channelContext = uri;
> -  }

All my "good" new code gets removed again :-(

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +622,2 @@
>        converter->AsyncConvertData("message/rfc822", "*/*",
> +           aConsumer, static_cast<nsIChannel*>(this), getter_AddRefs(newConsumer));

Somehow the static cast makes me nervous.
Attachment #9044751 - Flags: review?(jorgk) → feedback+

I'll land those patches now, but they are merely clean-up. If we don't intend to do further work here, see comment #1, we should change the summary to "Clean-up after bug 1452600" and be done with it. Up to you.

Keywords: leave-open
Comment on attachment 9044751 [details] [diff] [review]
remove-superflous-qi-1.patch

OK, I'll pull out the static casts.
Attachment #9044751 - Flags: feedback+ → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ae8434414bd7
Bug 1452600 follow-up: add comments documenting some nsIStreamListener kludges that could be tidied up. r=jorgk
https://hg.mozilla.org/comm-central/rev/691bdc9428e1
Bug 1452600 follow-up: remove some superfluous nsIChannel QI conversions. r=jorgk

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

Comment on attachment 9044751 [details] [diff] [review]
::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +622,2 @@

   converter->AsyncConvertData("message/rfc822", "*/*",
  •       aConsumer, static_cast<nsIChannel*>(this), getter_AddRefs(newConsumer));
    

Somehow the static cast makes me nervous.

Yeah, I hate seeing static_cast<>. But I didn't see any better way to do it without the runtime QI conversion (which is unnecessary because we know the type at compile time). I started with just this (the parameter is type nsISupports*), but the compiler got confused about multiple nsISupports in the inheritance tree.

Anyway, it's a moot point - it's a context parameter, and should probably be nullptr anyway (Bug 1528664)

Summary: streamlisteners should get uri without using params passed in in the callback → Clean-up after bug 1452600

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

I'll land those patches now, but they are merely clean-up. If we don't intend to do further work here, see comment #1, we should change the summary to "Clean-up after bug 1452600" and be done with it. Up to you.

Sounds good to me. Should it be set to "RESOLVED WONTFIX" or something?

Why WONTFIX. We did the clean-up, so we're done.

Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 67.0
You need to log in before you can comment on or make changes to this bug.