Rebased to bug 1520868 and https://phabricator.services.mozilla.com/D16885.

Looks like now, unlike before, M-C are planning not only to remove open/asyncopen, but also to rename open2/asyncopen2 to the "not-2" names.

Minor tweaks were necessary in RDF :-(

This patch simply scraps the context argument which is quite wrong, we have some tips on how to fix this in bug 1520868 comment #7.

Here's a "property bag" implementation Boris suggested in bug 1520868 comment #7.

I ran our
mach xpcshell-test comm/mailnews/imap/test/unit/test_saveTemplate.js

and that failed with
0:01.25 pid:1536 Assertion failure: false (channel needs to have loadInfo to perform security checks), at c:/mozilla-source/comm-central/dom/security/nsContentSecurityManager.cpp:800

when the nsContentSecurityManager::doContentSecurityCheck() calls weren't commented out. So here they are all commented out again, as we used the "unsecure" open/asyncopen before. With those calls commented out, a few tests I tried pass.

So what's the way forward here after we fixed the context issue?

Try with this patch:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a689c8672e3077cf4a24a76d078ba202e3ec26d2

So what's the way forward here after we fixed the context issue?

Make sure your channels have loadinfo?

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #12)

Make sure your channels have loadinfo?

Absolutely, but how? Boris un-CC'ed himself, so who could answer this question? Channel is a network thing, so maybe Valentin.

Hmm, a bunch of tests are still failing, but not the ones I tried before.

mach xpcshell-test comm/mailnews/news/test/unit/test_internalUris.js
dies on nsMsgProtocol.cpp, line 332. Looks like rv = m_channelListener->OnStartRequest(this, m_channelContext);returned a bad status which suggests that the property bag trick hasn't worked. However, doing

    if (bag)
      bag->GetPropertyAsInterface(NS_LITERAL_STRING("context"), NS_GET_IID(nsIURI),
                                  getter_AddRefs(m_channelContext));
    else
      printf("=== no bag\n");

doesn't print the debug which suggests that it works. Strange.

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

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #12)

Make sure your channels have loadinfo?

Absolutely, but how? Boris un-CC'ed himself, so who could answer this question? Channel is a network thing, so maybe Valentin.

newChannel2: function (aURI, aLoadInfo) {}
The aLoadInfo argument should not be ignored.
Before returning the channel, we should call channel.loadInfo = aLoadInfo;
For example here, but I see there are lots of other protocol handlers.
https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/ldap/xpcom/src/nsLDAPProtocolHandler.js#36

Thanks, Valentin, we'll fix this in a follow-up. For now, we need to fix the test failures which seem unrelated.

I missed a spot where the context also needs to be supplied.

Running
mach xpcshell-test comm/mailnews/imap/test/unit/test_imapFilterActions.js

I see:
0:02.97 pid:9512 [9512, Main Thread] ###!!! ASSERTION: ahah...someone didn't pass in the expected context!!!: 'NS_SUCCEEDED(rv)', file c:/mozilla-source/comm-central/comm/mailnews/base/src/nsCopyMessageStreamListener.cpp, line 96

That's also in nsCopyMessageStreamListener::OnStartRequest() and might explain the error mentioned in comment #15.

Sorry, I'll have to call it a night at 12:45 AM.

Looking at this again, it won't fly. The aContext parameter of asyncOpen() was passed to all the methods of the listener, so also OnStartRequest(). This isn't happening any more, so we are hosed. Looks like we need look at all OnStartRequest() implementations in Mailnews. Not fun.

Boris, with your suggestions from bug 1520868 comment #7, how did you envisage for the context to get passed to the listener methods?

how did you envisage for the context to get passed to the listener methods?

Which of the three options from that comment are you thinking about specifically?

Boris, the property bag one as per comment #11.

I'll land this now so people can at least compile. This is missing the context stuff (marked XXX TODO) and the missing loadinfo will also lead to additional failures.

Refreshed property bag patch coming up.

OK, this is the property bag solutions if I understood Boris' suggestion correctly. The patch takes care of the 12 "XXX TODO" call sites marked in the previous patch.

However, the issue of passing the context to OnStartRequest() is unresolved.

Also unresolved is that nsContentSecurityManager::doContentSecurityCheck() fails since we ignore the load info, see comment #16 and above. Those calls could just be commented out and fixed in part 3.

Summary:
Part 1: landed, make it compile.
Part 2: take care of the URL passed in the context, not working.
Part 3: take care of the load flags. Will come later.

OK, so in OnStartRequest you have the request. You QI it to propertybag, and get the property from it, no? Assuming that your channels are not doing anything too weird, in which case they are the thing passed to OnStartRequest.

Thanks, Boris.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #26)

OK, so in OnStartRequest you have the request. You QI it to propertybag, and get the property from it, no? Assuming that your channels are not doing anything too weird, in which case they are the thing passed to OnStartRequest.

OK, I was trying to dig through the class hierarchy. I had even started a patch like this:

 NS_IMETHODIMP nsCopyMessageStreamListener::OnStartRequest(nsIRequest * request, nsISupports *ctxt)
 {
   nsCOMPtr<nsIMsgDBHdr> message;
   nsresult rv = NS_OK;
-  nsCOMPtr<nsIURI> uri = do_QueryInterface(ctxt, &rv);
+  nsCOMPtr<nsIURI> uri;
+  nsCOMPtr<nsIChannel> channel = do_QueryInterface(this);
+  nsCOMPtr<nsIWritablePropertyBag2> bag = do_QueryInterface(channel);
+  if (bag)
+    bag->GetPropertyAsInterface(NS_LITERAL_STRING("context"), NS_GET_IID(nsIURI),
+                                getter_AddRefs(uri));
 
-  NS_ASSERTION(NS_SUCCEEDED(rv), "ahah...someone didn't pass in the expected context!!!");
+  NS_ASSERTION(uri, "No URL available in nsCopyMessageStreamListener::OnStartRequest() !!!");

That gave some compile errors and then I got side-tracked into something else. Looks like the compiler didn't like the direct QI, maybe I need to use this version QueryInterface(NS_GET_IID(xxx), getter_AddRefs(yyy));.

OK, I fixed the QIs and added

 NS_IMETHODIMP nsCopyMessageStreamListener::OnStartRequest(nsIRequest * request, nsISupports *ctxt)
 {
   nsCOMPtr<nsIMsgDBHdr> message;
   nsresult rv = NS_OK;
-  nsCOMPtr<nsIURI> uri = do_QueryInterface(ctxt, &rv);
+  nsCOMPtr<nsIURI> uri;
+  nsCOMPtr<nsIWritablePropertyBag2> bag;
+  request->QueryInterface(NS_GET_IID(nsIWritablePropertyBag2), getter_AddRefs(bag));
+  if (bag)
+    bag->GetPropertyAsInterface(NS_LITERAL_STRING("context"), NS_GET_IID(nsIURI),
+                                getter_AddRefs(uri));
 
-  NS_ASSERTION(NS_SUCCEEDED(rv), "ahah...someone didn't pass in the expected context!!!");
+  NS_ASSERTION(uri, "No URL available in nsCopyMessageStreamListener::OnStartRequest() !!!");

Adding more debug, I can see that bag is null here :-(

OK, following Boris' suggestion in nsMsgProtocol.cpp fixes some tests, but not all. There's still an issue with "no bag" in nsCopyMessageStreamListener.cpp.

Also, not only OnStartRequest() but also OnStopRequest() and OnDataAvailable() need the treatment if they look at the context.

Here's another try with what I have:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6c891b2a3f3c9184301a280748394cc0c434ef05

Looks like these need to be looked at:
https://dxr.mozilla.org/comm-central/search?q=queryinterface(ctxt&redirect=false
nsCopyMessageStreamListener.cpp, nsMsgFolderCompactor.cpp, nsMsgProtocol.cpp, nsParseMailbox.cpp

In some function definitions the parameter is called aCtxt, aSupport or aContext, so there is more to inspect, but we're only interested when we see something related to uri/url on the LHS which can be found in the source files mentioned below:

https://dxr.mozilla.org/comm-central/search?q=queryinterface(actxt&redirect=false
none

https://dxr.mozilla.org/comm-central/search?q=queryinterface(asupport&redirect=false
nsMsgMailNewsUrl.cpp

https://dxr.mozilla.org/comm-central/search?q=queryinterface(acontext&redirect=false
none

• nsCOMPtr<nsIChannel> channel = do_QueryInterface(this);

You want to QI request, not this. And then do_QueryInterface will work (unlike with this, which may be multiply-inheriting nsISupports).

If bag is still null when QIing request, you may want to look in a debugger at what the concrete type of request is and whether that matches the concrete type of the thing you're looking at when you add the property to the bag.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #31)

• nsCOMPtr<nsIChannel> channel = do_QueryInterface(this);
  You want to QI request, not this. And then do_QueryInterface will work (unlike with this, which may be multiply-inheriting nsISupports).

Thanks, that was in a previous version, now the code is like in comment #28 and channel has disappeared altogether.

If bag is still null when QIing request, you may want to look in a debugger at what the concrete type of request is and whether that matches the concrete type of the thing you're looking at when you add the property to the bag.

Debugging on Windows is tricky since clang-cl doesn't show proper typed, see bug 1490743. That's said, the stack is tricky in the case were the bag is null:

xul.dll!nsCopyMessageStreamListener::OnStartRequest(nsIRequest * request, nsISupports * ctxt) Line 102
at c:\mozilla-source\comm-central\comm\mailnews\base\src\nsCopyMessageStreamListener.cpp(102)
xul.dll!mozilla::net::nsStreamListenerTee::OnStartRequest(nsIRequest * request, nsISupports * context) Line 17
at c:\mozilla-source\comm-central\netwerk\base\nsStreamListenerTee.cpp(17)
xul.dll!`anonymous namespace'::SyncRunnable2<nsIStreamListener,nsIRequest *,nsISupports *>::Run() Line 149
at c:\mozilla-source\comm-central\comm\mailnews\imap\src\nsSyncRunnableHelpers.cpp(149)
xul.dll!nsThread::ProcessNextEvent(bool aResult, bool *) Line 1149
at c:\mozilla-source\comm-central\xpcom\threads\nsThread.cpp(1149)
xul.dll!nsThreadManager::SpinEventLoopUntilInternal(nsINestedEventLoopCondition * aCondition, bool) Line 487
at c:\mozilla-source\comm-central\xpcom\threads\nsThreadManager.cpp(487)

made especially difficult by nsSyncRunnableHelpers.cpp and maybe also the Tee, I don't know.

Debugging on Windows is tricky

Do you have a non-Windows option?

Trying to get the concrete type from the stack sounds painful. You want to just look it up at the point where you have the object...

This replaces the last QIs of the context with the bag trick.

Running more of the failing tests manually, for example
mach xpcshell-test comm/mailnews/local/test/unit/test_undoDelete.js
I see more of the same error in nsCopyMessageStreamListener::OnStartRequest():

0:01.16 pid:12044 [12044, Main Thread] ###!!! ASSERTION: No URL available in nsCopyMessageStreamListener::OnStartRequest() !!!: 'uri', file c:/mozilla-source/comm-central/comm/mailnews/base/src/nsCopyMessageStreamListener.cpp, line 102
0:01.94 pid:12044 #01: nsInputStreamPump::OnStateStart (c:\mozilla-source\comm-central\netwerk\base\nsInputStreamPump.cpp:487)
0:01.94 pid:12044 #02: nsInputStreamPump::OnInputStreamReady (c:\mozilla-source\comm-central\netwerk\base\nsInputStreamPump.cpp:396)
0:01.95 pid:12044 #03: mozilla::SlicedInputStream::OnInputStreamReady (c:\mozilla-source\comm-central\xpcom\io\SlicedInputStream.cpp:414)
0:01.97 pid:12044 #04: nsInputStreamReadyEvent::Run (c:\mozilla-source\comm-central\xpcom\io\nsStreamUtils.cpp:93)

Here we don't have a Tee but a SlicedInputStream which is used in various places around C-C.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from bug 1520868 comment #7)

I don't quite understand the Thunderbird pushback here.

Well, the pushback was that we now have to push an elephant through the eye of a needle in 20 y/o code that no one is familiar with. There's nothing wrong with getting rid of the insecure methods, but something like AsyncOpenWithContext() taking the old two parameters would have been handy.

Here another try, although I doubt that it will fix anything since the root cause is that the channel that gets the property bag attached is not presented to the OnStartRequest() callbacks and their friends, at least not in all cases.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=71aa6d1bcb0c4966e1d0a46a6a79bba102a63cbb

Maybe we want to dedicate another resource to it since frankly I'm tired.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #33)

Do you have a non-Windows option?

Well, I can switch to MSVC, that can do it properly. For Linux, I've have to boot up an old machine which had a Linux build environment about a year ago. I estimate a day's work to get that up and running again, maybe less. But then, I never did any debugging on Linux :-( - MSVC would be the safest option, or delegating the job ;-)

For the record, the second patch makes these tests I tried at random pass:
mach xpcshell-test comm/mailnews/local/test/unit/test_pop3Duplicates.js
mach xpcshell-test comm/mailnews/compose/test/unit/test_smtp8bitMime.js

Sigh, the test_smtp8bitMime.js crashes now, but it certainly worked with a prior version of the second patch, hmm.

Also, in nsMsgMailboxParser::OnDataAvailable() rv should be initialised to NS_ERROR_FAILURE since that will be returned when the URL can't be obtained.

Of course we could try one of the other of Boris' suggestions from bug 1520868 comment #7, in particular No. 3 to get the URL from the request ... not sure how, but anyway.

I guess you can add an interface such as:

interface nsIURIHolder : nsISupports {
  void setURI(in nsIURI uri);
};

• Implement nsIURIHolder for classes that implement nsIStreamListener and require the context parameter.
• Whenever you want to call AsyncOpen with a non-null context, QI nsIURIHolder and call setURI if the QI succeeded.
• Use the set URI instead of the context parameter in OnStartRequest/OnDataAvailable/OnStopRequest.

I assume that the context parameter is used only for URIs. If not, nsIContextHolder would be required that would take an nsISupports.

Thanks for the comment. This would be No. 2 from bug 1520868 comment #7.

For the record: No. 1 seems problematic since we don't know what the request parameter of the OnStartRequest/OnDataAvailable/OnStopRequest actually is. It's something that implements nsIRequest, that's all we know. Boris seems to suggest that the channel is actually passed in there, but that doesn't appear to be true.

Basic framework for approach No. 2. I won't be able to return to this for a while now.

OK... my (still somewhat shaky) take on the various approaches, using the suggestions in bug 1520868 comment #7:

3 seems worth pursuing, but relies on the request param of the On[Start|Stop|Data]Request callbacks being QI-able to nsIChannel, so we can get at the URI. Presumably we can do this easily enough for all the TB-specific protocols - there don't seem to be too many to go through. But assumes that URI is the only thing we're passing via ctxt.

2 could be handy, but seems wrong to force a listener to be coupled to a specific request like that. But maybe they're already pretty closely coupled.

1 is problematic because while you likely know what the concrete type of the channel is when you call AsyncOpen(), the listener doesn't know the concrete type - it just sees a nsIRequest. If the listeners can assume particular types of channels, then they can QI back to a concrete type and ask it for the extra context, but that seems a little iffy.
I think the nsIURIHolder approach falls into this category.

Handing over for the evening - here's what I've got so far:

I think it's possible this is simpler than it looks.

1. as far as I've seen so far, all the TB protocols pass a uri as the context field to the nsIStreamListener callbacks.
2. the TB protocols inherit from nsMsgProtocol (an exception is nsImapMockChannel. Are there others?)
3. nsMsgProtocol has a nice handy m_channelContext protected member which is (usually) passed to the listener callbacks.

So as long as we make sure the TB protocols all put the URL into m_channelContext we should be OK.

I've just been concentrating on comm/mailnews/news/test/unit/test_internalUris.js, and this very hacky patch is enough to get it passing for me.

I'll pick it up again tomorrow.

Thanks Ben, I've tweaked your parch slightly.

In general, it's a good observation that for protocols, we can use the m_channelContext member. Sadly, there are cases of other listeners which are not protocols. You need to look at my patch with the property bag to see all the call sites were context passed to a listener was converted to a URL.

I think No. 2 where we generically equip listeners with an extra URL field via a mix-in class is more promising.

Tweaked comments where XXX TODO was no longer necessary.

BTW, we also need to remove the nsContentSecurityManager::doContentSecurityCheck() calls for now since the fail for other reasons.

I'm about half way changing bags to holders and discovered that channels have readonly attribute nsIURI URI; so we can use that for the protocols, I hope since it's clashing with our own Get/SetURI(), unless I want to call this something differently.

I'll continue in a few hours.

This is the complete URI holder patch. Where we dealing with channels, we can get the URI from the channel.

The patch removes the 12 "XXX TODO" labels and adds one a new one that may need further inspection.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b6d92130583eac03da423a4024d204be8d991246

OK, so actually using the holder for the purpose it was created for actually helps, that is use the SetURI() function.

That made more tests pass.
mach xpcshall-test comm/mailnews/imap/test/unit/test_imapFilterActions.js
still fails, and it's likely that I didn't set the correct URI. But we get the idea of what needs to be done.

I guess the content policy tests fail since I disabled security. So we need to un-comment those call sites and fix the loadinfo.

Over to the night shift ;-)

Slight variation.

Yes, indeed. I'll fix this in the next round.

Dump of what I've got on the loadinfo stuff so far.
(keep in mind this is me cargo-culting code by grepping through M-C, I really don't have a good grasp on the security model yet)

I think we need to replace our calls to NewChannel() with something like:

      nsCOMPtr<nsIPrincipal> nullPrincipal =
        mozilla::NullPrincipal::CreateWithoutOriginAttributes();

      nsCOMPtr<nsILoadInfo> loadInfo = new mozilla::net::LoadInfo(
        nullPrincipal,
        nullptr,
        nullptr,
        nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,
        nsIContentPolicy::TYPE_OTHER);
      if (!loadInfo) {
        return NS_ERROR_UNEXPECTED;
      }

      rv = NewChannel2(url, loadInfo, getter_AddRefs(channel));
      NS_ENSURE_SUCCESS(rv, rv);

Seems like a lot of boilerplate, sigh.
And in fact still doesn't work - a cross-origin check is failing.
I think it's because of CreateWithoutOriginAttributes().

There is another function which might do the trick:

mozilla::NullPrincipal::Create(
    const OriginAttributes& aOriginAttributes, nsIURI* aURI)

But I'm out of time for the day, so haven't investigated further.

Oh, need to #include "mozilla/LoadInfo.h" and "mozilla/NullPrincipal.h".

OK. There are three principal "classes": Null principal, codebase principal and system principal. We use whatever gives the permissions we need. Getting the system principal is a single call: nsContentUtils::GetSystemPrincipal(); in C++ code.

As an aside, this morning I was experimenting with a way simpler alternative fix to all the AsyncOpen() bustage. The idea is that the listener callbacks can get at the URI by QIing nsIRequest into nsIChannel, and ignore their context parameter entirely.

I'd written it off as a failure, but I just did a try build on the off chance and it's actually looking pretty good...
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=04f3bd29cb2b27cde4d2ed206af0a93fe6319a4a

That's without the urlholder patch.
Most of the remaining unittest fails are due to LoadInfo/security asserts, I think. Not looked at the mozmill ones.

So once the dust settles, I think it'd be worth revisiting and seeing if there's any simplification on offer.

A few remarks:

1. This is Boris' suggestion No. 3.
2. You should have turned off the security checking.
3. You should have covered all call sites where the context is used, like I have.
4. I already use the URI on the channel in various places here: https://hg.mozilla.org/comm-central/rev/fe6c03adc0f01839a2688bccf19ac3073c010def
5. Are you sure that for example nsCopyMessageStreamListener can be QI'ed to a channel? Do you see: "error QI nsIRequest to nsIChannel failed". What is the class hierarchy?

Looks like on your try there are more MozMill failures, but that might be due to points 2) and 3) above.

I think Ben's approach is far preferable if we can get the URL from the channel. I have therefore taken his patch from try applied it on top of a backout of my "holder" stuff. But I completed it by switching off security checks for now and covering all call sites where the context is QI'ed. That was already in my original "holder" patch (which sometimes queried the channel).

As net effect, all holder stuff was removed and replaced with QI of the request, existing QI of the request was maintained, and the switch-off of the security stuff was also maintained.

Here is what was landed.

OK, I'm not using m_url any more but query the channel instead. Let's see:

hg qnew -m "try: -b do -p macosx64,linux64 -u all" try && hg push -f -r tip cc-try && hg qpop && hg qdelete try

If Ben is right, this should be a whole lot better. Since I've disabled Calendar due to heaps of unrelated failures, we can compare the currently landed approach from
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=9ea7d639422237da1420880a874a8673759387a4

with the try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=05a3198014a32fbe0d2cf27cb69b43d48ba9fb0b

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

I think Ben's approach is far preferable if we can get the URL from the channel.

... because it too hard to work out which URL and and where it needs to be set in the holder. If the channel knows it, use it :-)

This looks pretty good, I think I'll land this now. There are still a bunch of Xpcshell test failures, but MozMill looks OK:

Failing are:
composition/test-image-display.js - most likely security related
content-policy/test-js-content-policy.js - most likely security related
folder-display/test-invalid-db-folder-load.js - who knows
folder-display/test-display-name.js
pref-window/test-font-chooser.js

I'll disable those.

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

Comment on attachment 9044235 [details] [diff] [review]
1452600-switch-to-QI.patch

Review of attachment 9044235 [details] [diff] [review]:

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +297,5 @@

// OnDataAvailable. We then read and process the incoming data from the input stream.
NS_IMETHODIMP nsMsgProtocol::OnDataAvailable(nsIRequest *request, nsISupports *ctxt, nsIInputStream *inStr, uint64_t sourceOffset, uint32_t count)
{
// right now, this really just means turn around and churn through the state machine

• nsCOMPtr<nsIURI> uri = m_url; // XXX: We could get the URL from the request/channel.

On second thought: I'm not sure that all protocols set m_url, I know some do
since I added that assignment once.

Might be much safer to query the channel here.

I originally did try to QI to the channel here, but it didn't work because it was being invoked from somewhere down in the net code (InputMessagePump? something like that) which was derived from nsIRequest, but not nsIChannel.

So I don't think we can QI to the channel in our protocol code (ie listener callbacks embedded into our protocols), because those callbacks can be called from lower-level net code which isn't channel-based.

However, most of our non-protocol listeners are only ever invoked by the TB nsMsgProtocol-based protocols, which are based upon nsIChannel. So in the five or six places where non-protocol listeners were using the URI from the context param, they can get an nsIChannel from the request instead.

Hmm. But your try build looks pretty promising (with the QI to channel in the nsMsgProtocol listener implementation), so maybe I just read my errors wrong!

Yes, the try looks good, landing on C-C we have more bustage, likely from bug 1528041 and one from bug 1423487 :-(

We need to decide on a way forward here. As we can see, too much bustage piled on top of each other becomes messy very quickly. Sadly all the Xpcshell test failures will need to be analysed individually.

Looking at comm/mailnews/local/test/unit/test_pop3FilterActions.js, I see:
error QI nsIRequest to nsIChannel failed: 'NS_SUCCEEDED(rv)', file m:/mozilla-source/comm-central/comm/mailnews/local/src/nsParseMailbox.cpp, line 93
So sadly your QI code doesn't work there :-(

comm/mailnews/local/test/unit/test_pop3MultiCopy2.js fails very weirdly:
NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file m:/mozilla-source/comm-central/comm/mailnews/base/src/nsCopyMessageStreamListener.cpp, line 115
EndCopy() got passed in a URI and there wasn't a failure retrieving it unless the channel can return a null URI without a bad status. We might want to check that.

I really don't want to return to the holder approach, but right now I see cased were there isn't a channel or it doesn't give a useful URI.

Looks like the protocols are the least of our worries, and I'm pretty sure m_url doesn't always get set:
https://searchfox.org/comm-central/search?q=m_url+%3D&case=false&regexp=false&path=
Like you can see that NNTP doesn't use it, but we could add that. Feel free to do a try with the landed patch backed out and instead the previous version from comment #60 which used m_url.

The other thing we need to do is move the security checks forward, so we can re-enable the MozMill tests I've switched off.

OK - Saturday here. I'll be offline most of the day, but I'll check in this evening (in about 7hrs) and see what I can get done.

On the security checks:
Your nsContentUtils::GetSystemPrincipal(); suggestion for the loadinfo looks good. Did the rest of code snippet I posted (new mozilla::net::LoadInfo(.....) and NewChannel2()) look like a sane approach?

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

Looks like the protocols are the least of our worries, and I'm pretty sure m_url doesn't always get set:
https://searchfox.org/comm-central/search?q=m_url+%3D&case=false&regexp=false&path=
Like you can see that NNTP doesn't use it, but we could add that.

incredibly enough, i once looked at this: Bug 980451.

I believe this is the only place in js that used context to get a uri; latest pull broke gloda testing, this patch makes message indexing happen again..

(In reply to Ben Campbell from comment #72)

On the security checks:
Your nsContentUtils::GetSystemPrincipal(); suggestion for the loadinfo looks good. Did the rest of code snippet I posted (new mozilla::net::LoadInfo(.....) and NewChannel2()) look like a sane approach?
I think so. Note: We always want to use the least "potent" principal. Please also note comment #16 which I think will apply to more property handlers.

Yes, I saw last night that we're still passing contexts around when they are all null:
https://searchfox.org/comm-central/search?q=actxt&case=false&regexp=false&path=imap

Please note my latest try which should be pretty good thanks to Aceman fixing unrelated bustage:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9ed23d6f088488c137212f682a96bedcf922b618

Here's my patch to reinstate the security checks and add missing loadInfo objects to our protocols.
Don't think it fixes any more xpcshell tests, but at least doesn't seem to cause any more to fail :-)

Try build (with those 7 mozmill tests re-enabled):
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=304d026acd6a10c31b6d6221d1210290f1cde1be

Not happy with my lack of knowledge about how the loadinfo stuff really works, so don't assume any competency in this patch!

Haven't had a chance to look at any of the specific remaining xpcshell test fails, but will try and sneak a couple of hours on them tomorrow.

Thanks, we still need to address comment #16.

Actually, looking at this closer, NewChannel() is be deprecated and replaced by NewChannel2():
https://searchfox.org/mozilla-central/search?q=-%3Enewchannel(&case=false&regexp=false&path=
All M-C implementation I've seen just called NewChannel2() with a nullptr. Maybe we're on the wrong boat here. Let's just re-instate the security calls and see what happens.

BTW, new in the Mozilla environment is infallible, control won't return if it fails.

Try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c3af52f368019b5f89286853532d52ede034deeb

Also, as a summary, here the list of remaining Xpcshell test nive failures:
TEST-UNEXPECTED-FAIL | comm/chat/protocols/irc/test/test_setMode.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/xpcshell/test_ext_messages.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_copyToInvalidDB.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_folderCompact.js | xpcshell return code: 1
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_quarantineFilterMove.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_pop3FilterActions.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_pop3MultiCopy2.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_over4GBMailboxes.js | xpcshell return code: 0

Some of them are already discussed in comment #71 and won't be easy to fix since we don't seem to have a channel wit URI available.

This should be it.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=629804c01e7086719740107c86c2bbb2e14b0286

https://hg.mozilla.org/comm-central/rev/9cdf0964163cd3d0486f6e822f643658bc9d75a5
Reinstate doContentSecurityCheck() calls and replace NewChannel calls with NewChannel2. r=jorgk
https://hg.mozilla.org/comm-central/rev/bb0761050764addee4ae0360177696ebdd81bad7
Re-enable four MozMill tests. a=backout

Summary of remaining failures:
Xpcshell:
TEST-UNEXPECTED-FAIL | comm/chat/protocols/irc/test/test_setMode.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/xpcshell/test_ext_messages.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_copyToInvalidDB.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_folderCompact.js | xpcshell return code: 1
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_quarantineFilterMove.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_pop3FilterActions.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_pop3MultiCopy2.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_over4GBMailboxes.js | xpcshell return code: 0

Mozmill (disabled):
test-invalid-db-folder-load.js
test-display-name.js

The latter two are something for Aceman to look at, please ;-)

OK, test-display-name.js was broken by bug 1423487. Did I say it's hard to manage with multiple bustages :-(

test-display-name fixed, but I don't see any test problem in test-invalid-db-folder-load.js. May the backend patches here have a problem?
When the test enters the folder and it should automatically repair the DB, I see:
[18871, Main Thread] WARNING: error QI nsIRequest to nsIChannel failed: 'NS_SUCCEEDED(rv)', file mailnews/local/src/nsParseMailbox.cpp, line 93
[18871, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004002: file mailnews/local/src/nsParseMailbox.cpp, line 94
[18871, Main Thread] WARNING: error QI nsIRequest to nsIChannel failed: 'NS_SUCCEEDED(rv)', file mailnews/local/src/nsParseMailbox.cpp, line 69
[18871, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file mailnews/local/src/nsParseMailbox.cpp, line 70

Thanks for looking. Yes, that's one of the errors we see in the Xpcshell tests, see comment #71. I think I'll take a little break and let Ben investigate those failures.

Yes, that warning was added in attachment 9044239 [details] [diff] [review], so the code block may need a rework.

Well, the problem is sadly much deeper :-( - M-C cut away the context parameter of AsyncOpen() which carried URL information to the callback. Now we're struggling to get this information to were we need it. The so far safest bet has been to QI the request to a channel and get its URI. Now we're left with a few cases where that doesn't work for some reason. This is crusty old Mailnews code, about 20 years old :-(

Looking at the remaining test failures I noticed that in one case we don't even need the URL. Just a little clean-up that doesn't fix anything since we still need it in OnStartRequest().

nsInputStreamPump (the implementation of nsIInputStreamPump) passes this as a request parameter that is not a channel.
https://searchfox.org/mozilla-central/rev/e6520f0a4dd5d7474c32a1164744953ea59be0d0/netwerk/base/nsInputStreamPump.cpp#487
https://searchfox.org/mozilla-central/rev/e6520f0a4dd5d7474c32a1164744953ea59be0d0/netwerk/base/nsInputStreamPump.h#20
https://searchfox.org/mozilla-central/rev/e6520f0a4dd5d7474c32a1164744953ea59be0d0/netwerk/base/nsIInputStreamPump.idl#27
https://searchfox.org/mozilla-central/rev/e6520f0a4dd5d7474c32a1164744953ea59be0d0/xpcom/io/nsIAsyncInputStream.idl#95
https://searchfox.org/mozilla-central/rev/e6520f0a4dd5d7474c32a1164744953ea59be0d0/netwerk/base/nsIThreadRetargetableRequest.idl#18

I think you can create a class such as:

class PumpListenerProxy final : public nsIStreamListner {
 public:
  NS_DECL_ISUPPORTS

  PumpListenerProxy(nsIStreamListener* aListener, nsIChannel* aChannel)
      : mListener(aListener), mChannel(aChannel) {
    MOZ_ASSERT(aListener);
    MOZ_ASSERT(aChannel);
  }

  NS_IMETHOD
  OnDataAvailable(nsIRequest* aRequest, nsISupports* aContext,
                  nsIInputStream* aIStream, uint64_t aSourceOffset,
                  uint32_t aLength) override {
    // Proxy OnDataAvailable using the internal channel
    return mListener->OnDataAvailable(mChannel, aContext, aIStream,
                                      aSourceOffset, aLength);
  }

  NS_IMETHOD
  OnStartRequest(nsIRequest* aRequest, nsISupports* aContext) override {
    // Proxy OnStartRequest using the internal channel
    return mListener->OnStartRequest(mChannel, aContext);
  }

  NS_IMETHOD
  OnStopRequest(nsIRequest* aRequest, nsISupports* aContext,
                nsresult aStatusCode) override {
    // Proxy OnStopRequest using the inernal channel
    return mListener->OnStopRequest(mChannel, aContext, aStatusCode);
  }

 private:
  ~PumpListenerProxy() {}
  nsCOMPtr<nsIStreamListener> mListener;
  nsCOMPtr<nsIChannel> mChannel;
};

And use it such as:

- rv = pump->AsyncRead(this, urlSupports);
+ RefPtr<PumpListenerProxy> proxy = new PumpListenerProxy(this, this);
+ rv = pump->AsyncRead(proxy, urlSupports);

(Borrowed an idea from Chris, see comment #7)

Just another tidying-up-after-the-dust-has-settled-thought:

What are NewChannel2() implementations supposed to do if they are passed a nullptr loadInfo?
The options seem to be:

1. set the channel's loadinfo to nullptr
2. construct a default loadinfo and set that on the channel
3. fail, complaining about a missing loadinfo.

I've seen examples of 1 and 2.

If the correct answer is 2, then attachment 9044418 [details] [diff] [review] could be tidied up by moving the loadinfo construction down into NewChannel2() rather than doing it in the caller.

But the comments on the IDL don't seem to help much, one way or the other...

https://searchfox.org/mozilla-central/source/netwerk/base/nsIProtocolHandler.idl#112

    /**
     * Constructs a new channel from the given URI for this protocol handler and
     * sets the loadInfo for the constructed channel.
     */
    nsIChannel newChannel2(in nsIURI aURI, in nsILoadInfo aLoadinfo);

Thanks, Masatoshi-san, I'll play with this tonight, otherwise Monday.

(In reply to Ben Campbell from comment #101)

What are NewChannel2() implementations supposed to do if they are passed a nullptr loadInfo?
The options seem to be:

1. set the channel's loadinfo to nullptr
2. construct a default loadinfo and set that on the channel
3. fail, complaining about a missing loadinfo.

I've seen examples of 1 and 2.

I found some examples of 3 in m-c:
https://searchfox.org/mozilla-central/rev/e6520f0a4dd5d7474c32a1164744953ea59be0d0/chrome/nsChromeProtocolHandler.cpp#100
https://searchfox.org/mozilla-central/rev/e6520f0a4dd5d7474c32a1164744953ea59be0d0/netwerk/protocol/res/SubstitutingProtocolHandler.cpp#233

Frankly, I wouldn't change our implementations of NewChannel2(), especially No. 2 doesn't look right. Either complain or pass it on. Where did you see examples of No. 2?

That said, we should focus on getting the bustage fixed, so to try the suggestion in comment #97. I didn't get to it today, but we have a "day shift" working in NZ already ;-)

(In reply to Ben Campbell from comment #78)

(and it wouldn't surprise me if M-C phased out the context params entirely.
If you can't pass a context into AsyncOpen() then I can't see the point of
having a context passed to OnStartRequest et al, as you're now relying on
each channel implementation to decide what to pass as context, which seems
silly).

Ahh, yes - Bug 1525319 covers getting rid of the context param and points out how inconsistently it's used.
It's not a definite plan, but I can't see how they'd conclude context param makes any sense to keep now.

So, if at all possible, we should avoid using the context mechanism, just to be prepared.

I was implemented Masatoshi's fix, then realised that I could shortcut things.

Fixes all the xpcshell unit tests except test_filters.js on my local run... let's see how the try server likes it...

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2e68c2c44753eca4b28f759ad57937bc4f4b936c

See bug 1528677. M-c will soon assume that NewChannel2() callers will always pass a (non-null) loadInfo.

You meant mailnews/news/test/unit/test_filter.js? That passes locally and on try. mailnews/news/test/unit/test_bug695309.js appears to be failing intermittently, but we can look at this in a new bug, similar to bug 1299562 which got abandoned, so perhaps time to look at it again.

MozMill folder-display/test-invalid-db-folder-load.js also passes. testLocalICS.js still fails, but I'm not convinced we're at fault here. I'll file another bug for it.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ca537479f4e2
fix nsMailboxProtocol streamlistener callback bustage. r=jorgk
https://hg.mozilla.org/comm-central/rev/04655f7dd330
Re-enable all tests disabled by this bug (backout 78a88fcf97de, 30d43084cbc1). a=backout

Just for reference, I filed a couple of followups - nothing urgent, just notes on possible tidy-ups:

Bug 1528662
Bug 1528664

Thanks. Some of https://hg.mozilla.org/comm-central/rev/ca537479f4e2 was labelled a "temp hack". So where do we intend to fix this?

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

Thanks. Some of https://hg.mozilla.org/comm-central/rev/ca537479f4e2 was labelled a "temp hack". So where do we intend to fix this?

That'd fall under Bug 1528662. Mainly getting rid of the QI nsIRequest to nsIChannel stuff. However, it'd be a non-trivial change and what's there is solid enough. I don't think it'll break any time soon.

So my plan is to just add in some comments pointing back to Bug 1528662 for the benefit of anyone working there in future. That'll include smoothing out those "temp hack" comments with something more useful :-)