1452600 - Port |Bug 1520868 - Remove nsIChannel::Open/AsyncOpen| (Move open2 and asyncOpen2 to being the only implementaion)

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Comment 1

[Masatoshi Kimura [:emk]]

Although C++ changes have to be synchronized with m-c changes, JS changes don't have to wait for m-c update. open2/asyncOpen2 are already present.

Comment 2

[Jorg K (CEST = GMT+2)]

See bug 1411609 comment #11 (and above) for details.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-asyncopen.patch - WIP (missing a lot, see comment #8)

This compiles, I haven't tried running it.

The problem appears to be that nsMsgProtocol::AsyncOpen(), nsImapMockChannel::AsyncOpen() and nsNNTPProtocol::AsyncOpen() set a member variable m_channelContext from ctxt, nsNntpMockChannel::AsyncOpen() sets m_context from ctxt.

It's quite hard to work out what happens later with those member. Just looking at m_context, that is used in nsNntpMockChannel.cpp:
rv = protocol.LoadNewsUrl(m_url, m_context);
m_channelListener->OnStopRequest(this, m_context, rv);

The channel listener appears to be the NNTP protocol, and nsNNTPProtocol::OnStopRequest() calls  nsMsgProtocol::OnStopRequest() which QI's the context parameter to the message URL.

Equally nsMsgProtocol::OnStartRequest() queries the context parameter to the message URL.

You can see in the patch that the context is typically populated with the URL being run, for example 
  nsCOMPtr<nsISupports> aCtxt = do_QueryInterface(aUrl);
in nsNntpService::GetMessageFromUrl().

I haven't dug through the IMAP code, but I believe things will be similar there, the context mostly holds the URL being run.

I'm neither author nor expert on any of this protocol code, but I think we'd have a hard time if we can't use AsyncOpen() since we need its context parameter. Am I missing something?

Comment 4

[Masatoshi Kimura [:emk]]

Who is actually using the context (other than passing around)?

Comment 5

[Jorg K (CEST = GMT+2)]

Providers:
https://dxr.mozilla.org/comm-central/search?q=regexp%3Actxt.*query&redirect=false
(and I think the calendar thing is a consumer)

Consumers:
https://dxr.mozilla.org/comm-central/search?q=regexp%3Aquery.*ctxt&redirect=false

You could remove M-C uses of Open/AsyncOpen and just leave the infrastructure for TB in place.

I believe the difference between AsyncOpen and AsyncOpen2 are:
The former has an extra parameter and the latter does a security check.

How about adding a |nsISupports *ctxt = nullptr| parameter to AsyncOpen2 or create AsyncOpen2WithContext to replace AsyncOpen. Would that still simplify M-C code?

Comment 6

[Masatoshi Kimura [:emk]]

The listener context parameter was removed from asyncOpen2 because Jonas suggested removing it and Christoph agreed in bug 1143922. Christoph, could you tell us what is the alternative?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Masatoshi Kimura [:emk] from comment #6)
> The listener context parameter was removed from asyncOpen2 because Jonas
> suggested removing it and Christoph agreed in bug 1143922. Christoph, could
> you tell us what is the alternative?

In most cases the the second argument aContext of asyncOpen() is never really used, though passed around as an opaque argument. Consumers are on OnStartRequest/OnStopRequest/OnDataAvailable. I would image (though haven't closesly looked at your patch) that you are fine just removing that argument.

In case you really do need to pass aContext. You could write a simple proxy, similar to the one we provided here [1] which just acts as a shim and passes the context along to OnStartRequest/OnStopRequest/OnDataAvailable.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8734241&action=diff

Comment 8

[Jorg K (CEST = GMT+2)]

Sadly we *need* the context, see https://dxr.mozilla.org/comm-central/search?q=regexp%3Aquery.*ctxt&redirect=false, so instead of using those proxies/shims (and use yet another technique for JS code), it would be nice if the platform already provided that facility. Reading through bug 1143922 (bug 1143922 comment #24), there was never any strong argument against the context argument, only that it didn't appear useful. Well, it can be. So providing a AsyncOpen2WithContext() would be a few lines of code in M-C saving us a lot of obscure code.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-asyncopen.patch - WIP

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.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 9041929 [details] [diff] [review]
1452600-asyncopen.patch - WIP

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

Expect lots of failures.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-asyncopen.patch - WIP v2

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Make sure your channels have loadinfo?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9043400 [details] [diff] [review]
1452600-asyncopen.patch - WIP v2

I don't know when I'll have time to look at this, sorry.  Please find someone else...

Comment 14

[Jorg K (CEST = GMT+2)]

(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.

Comment 15

[Jorg K (CEST = GMT+2)]

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.

Comment 16

[Valentin Gosu [:valentin] (he/him)]

(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

Comment 17

[Jorg K (CEST = GMT+2)]

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

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-asyncopen.patch - WIP v3

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.

Comment 19

[Jorg K (CEST = GMT+2)]

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?

Comment 20

[Boris Zbarsky [:bzbarsky]]

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?

Comment 21

[Jorg K (CEST = GMT+2)]

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

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 9041929 [details] [diff] [review]
1452600-asyncopen.patch - WIP

I'll split out the property bag stuff into a separate patch since it's not clear how this will fly.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-asyncopen.patch - part 1 [landed in comment #24]

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.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f3c9121fff48
Port bug 1411609: replace use of nsIChannel::Open/AsyncOpen and rename Open2/AsyncOpen to the former. rs=bustage-fix CLOSED TREE DONTBUILD

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-asyncopen-bag.patch

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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

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.

Comment 27

[Jorg K (CEST = GMT+2)]

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));.

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-asyncopen-bag.patch - WIP

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 :-(

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-asyncopen-bag.patch - WIP (v2)

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

Comment 30

[Jorg K (CEST = GMT+2)]

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

Comment 31

[Boris Zbarsky [:bzbarsky]]

• 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.

Comment 32

[Jorg K (CEST = GMT+2)]

(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.

Comment 33

[Boris Zbarsky [:bzbarsky]]

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...

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-asyncopen-bag.patch - WIP (v3)

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.

Comment 35

[Jorg K (CEST = GMT+2)]

(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 ;-)

Comment 36

[Jorg K (CEST = GMT+2)]

Comment on attachment 9043572 [details] [diff] [review]
1452600-asyncopen.patch - part 1 [landed in comment #24]

Note that for a landed patch r- is not really an option especially since backing it out would break us completely. Please file any suggestions/improvements/corrections as a new patch.

Comment 37

[Jorg K (CEST = GMT+2)]

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.

Comment 38

[Jorg K (CEST = GMT+2)]

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.

Comment 39

[Masatoshi Kimura [:emk]]

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.

Comment 40

[Jorg K (CEST = GMT+2)]

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.

Comment 41

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-uri-holder.patch

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

Comment 42

[Ben Campbell]

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.

Comment 43

[Ben Campbell]

Attachment: channelcontext_hackery.patch

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.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: channelcontext_hackery.patch (v2)

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.

Comment 45

[Jorg K (CEST = GMT+2)]

Attachment: channelcontext_hackery.patch (v2a)

Tweaked comments where XXX TODO was no longer necessary.

Comment 46

[Jorg K (CEST = GMT+2)]

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

Comment 47

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-uri-holder.patch - WIP

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.

Comment 48

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-uri-holder.patch (v2)

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

Comment 49

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-uri-holder.patch (v3)

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 ;-)

Comment 50

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-uri-holder.patch (v3b)

Slight variation.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fe6c03adc0f0
Port bug 1411609: Pass context/URI on listener's URI holder or use channel URI otherwise. rs=bustage-fix CLOSED TREE

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/13f98ba018ec
Port bug 1411609: Follow-up: Add missing GetURI() call. rs=bustage-fix CLOSED TREE

Comment 53

[Masatoshi Kimura [:emk]]

Comment on attachment 9044053 [details] [diff] [review]
1452600-uri-holder.patch (v3b)

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

::: mailnews/base/src/nsCopyMessageStreamListener.cpp
@@ +106,5 @@
> +  nsCOMPtr<nsIURI> uri;
> +  nsCOMPtr<nsIURIHolder> holder;
> +  QueryInterface(NS_GET_IID(nsIURIHolder), getter_AddRefs(holder));
> +  if (holder)
> +    holder->GetURI(getter_AddRefs(uri));

You don't have to call `QI` and `GetURI` in the implementation class. You can just access `mURI` directly (with changing access specifiers to `protected` if derived classes need an access). It is much faster and less error prone.

Comment 54

[Jorg K (CEST = GMT+2)]

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

Comment 55

[Ben Campbell]

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.

Comment 56

[Ben Campbell]

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

Comment 57

[Jorg K (CEST = GMT+2)]

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.

Comment 58

[Ben Campbell]

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.

Comment 59

[Jorg K (CEST = GMT+2)]

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.

Comment 60

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-switch-to-QI.patch

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.

Comment 61

[Jorg K (CEST = GMT+2)]

Comment on attachment 9043681 [details] [diff] [review]
1452600-asyncopen-bag.patch - WIP (v3)

I think the bag experiment didn't fly.

Comment 62

[Jorg K (CEST = GMT+2)]

Comment on attachment 9043843 [details] [diff] [review]
channelcontext_hackery.patch

I think this isn't desirable either.

Comment 63

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-uri-holder.patch (v4) [landed in comment 51+52]

Here is what was landed.

Comment 64

[Jorg K (CEST = GMT+2)]

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.

Comment 65

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-switch-to-QI.patch (v2) [landed in comment #68]

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

Comment 66

[Jorg K (CEST = GMT+2)]

(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 :-)

Comment 67

[Jorg K (CEST = GMT+2)]

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.

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/73b859d6c7df
Port bug 1411609: Remove nsIURIHolder and always use channel URI. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/30d43084cbc1
disable six failing MozMill tests. rs=bustage-fix

Comment 69

[Ben Campbell]

(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.

Comment 70

[Ben Campbell]

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!

Comment 71

[Jorg K (CEST = GMT+2)]

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.

Comment 72

[Ben Campbell]

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?

Comment 73

[alta88]

(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.

Comment 74

[alta88]

Attachment: channel.patch [landed in comment #80]

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..

Comment 75

[Jorg K (CEST = GMT+2)]

Comment on attachment 9044392 [details] [diff] [review]
channel.patch [landed in comment #80]

Wow, you're helping us with this terrible stuff, much appreciated!

Comment 76

[Jorg K (CEST = GMT+2)]

Comment on attachment 9043572 [details] [diff] [review]
1452600-asyncopen.patch - part 1 [landed in comment #24]

Ben, someone needs to look over these changes.

Comment 77

[Jorg K (CEST = GMT+2)]

(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.

Comment 78

[Ben Campbell]

Comment on attachment 9043572 [details] [diff] [review]
1452600-asyncopen.patch - part 1 [landed in comment #24]

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

This all looks sane to me. Only comments are ideas that came to me as I was looking through it and wanted to record :-)

::: mailnews/compose/src/nsMsgQuote.cpp
@@ +193,5 @@
>                                                  getter_AddRefs(convertedListener));
>    if (NS_FAILED(rv)) return rv;
>  
>    //  now try to open the channel passing in our display consumer as the listener
> +  rv = mQuoteChannel->AsyncOpen(convertedListener);  // XXX TODO: Provide context, newURI.

Don't need these TODO comments now, but I think they've already been removed.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +9906,5 @@
>    if (NS_FAILED(rv))
>        return rv;
>  
>    // set the stream listener and then load the url
> +  // XXX TODO: Set m_channelContext.

It just occurred to me that if we're not relying on the context parameter in listeners, we should aim to make all our base `nsMsgProtocol` send a `nullptr` context, so new code isn't lulled into making assumptions about what context might contain.

But that'd be for a separate cleanup bug, I think.
We could remove `nsMsgProtocol::m_channelContext` entirely.

(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).

Comment 79

[Jorg K (CEST = GMT+2)]

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

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7b9b036824bc
Get uri.spec from channel for nsIStreamListener methods in Gloda's mimemsg.js. r=jorgk
https://hg.mozilla.org/comm-central/rev/fcc93825ccfd
disable one more failing MozMill tests. rs=bustage-fix CLOSED TREE

Comment 81

[Ben Campbell]

Attachment: reinstate_docontentsecuritycheck.patch

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.

Comment 82

[Jorg K (CEST = GMT+2)]

Thanks, we still need to address comment #16.

Comment 83

[Jorg K (CEST = GMT+2)]

Comment on attachment 9044407 [details] [diff] [review]
reinstate_docontentsecuritycheck.patch

This looks good and as far as I can see, fixes all but two MozMill tests.

Comment 84

[Jorg K (CEST = GMT+2)]

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.

Comment 85

[Jorg K (CEST = GMT+2)]

Comment on attachment 9044407 [details] [diff] [review]
reinstate_docontentsecuritycheck.patch

Sorry, we were on the wrong boat. Enabling the security checks make five MozMill tests pass, still broken are two:
folder-display/test-display-name.js | test-display-name.js::test_from_display_name_multiple
folder-display/test-invalid-db-folder-load.js | test-invalid-db-folder-load.js::test_load_folder_with_invalidDB

That said, just enabling the security check without the tweaks to `NewChannel()` gives a bunch more mostly IMAP related test failures. All saying "channel needs to have loadInfo to perform security checks".

OK, I got it. Instead of prepping up the deprecated `NewChannel()` calls, we need to replace these with `NewChannel2()`:
https://searchfox.org/comm-central/search?q=%3D+NewChannel(&case=true&regexp=false&path=

Comment 86

[Jorg K (CEST = GMT+2)]

Attachment: reinstate_docontentsecuritycheck.patch [landed on comment #88]

This should be it.

Comment 87

[Jorg K (CEST = GMT+2)]

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

Comment 88

[Jorg K (CEST = GMT+2)]

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

Comment 89

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 90

[Jorg K (CEST = GMT+2)]

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

Comment 91

[:aceman]

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

Comment 92

[Jorg K (CEST = GMT+2)]

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.

Comment 93

[:aceman]

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

Comment 94

[Jorg K (CEST = GMT+2)]

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 :-(

Comment 95

[Jorg K (CEST = GMT+2)]

Attachment: 1452600-ProcessMailboxInputStream.patch [landed in comment #106]

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().

Comment 96

[Masatoshi Kimura [:emk]]

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

Comment 97

[Masatoshi Kimura [:emk]]

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)

Comment 98

[Masatoshi Kimura [:emk]]

class PumpListenerProxy final : nsIStreamListner {

Forgot public:

class PumpListenerProxy final : public nsIStreamListner {

Comment 99

[Ben Campbell]

Comment on attachment 9044456 [details] [diff] [review]
1452600-ProcessMailboxInputStream.patch [landed in comment #106]

Looks solid to me. Always nice seeing code deleted :-)

Comment 100

[Ben Campbell]

Comment on attachment 9044418 [details] [diff] [review]
reinstate_docontentsecuritycheck.patch [landed on comment #88]

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

This looks good to me.

Comment 101

[Ben Campbell]

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);

Comment 102

[Jorg K (CEST = GMT+2)]

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

Comment 103

[Masatoshi Kimura [:emk]]

(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

Comment 104

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 105

[Ben Campbell]

(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.

Comment 106

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/34890cd4037f
Remove unneeded code: ProcessMailboxInputStream() doesn't use URI parameter. r=benc
https://hg.mozilla.org/comm-central/rev/78a88fcf97de
Switch off 8 failing Xpcshell tests. rs=bustage-fix

Comment 107

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/acd9b2c0ecac
Switch off failing Mozmill test; rs=bustage-fix

Comment 108

[Ben Campbell]

Attachment: fix-nsmailboxprotocol-callbacks.patch [landed in comment #112]

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

Comment 109

[Masatoshi Kimura [:emk]]

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

Comment 110

[Jorg K (CEST = GMT+2)]

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.

Comment 111

[Jorg K (CEST = GMT+2)]

Comment on attachment 9044536 [details] [diff] [review]
fix-nsmailboxprotocol-callbacks.patch [landed in comment #112]

Let's go with this and tidy up in a new bug.

Comment 112

[Pulsebot]

Pushed by [e-mail]:
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

Comment 113

[Ben Campbell]

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

Bug 1528662
Bug 1528664

Comment 114

[Jorg K (CEST = GMT+2)]

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

Comment 115

[Ben Campbell]

(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 :-)

Comment 116

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c3935de1a0e4
Re-enable test that was forgotten when re-enabling. r=jorgk