1512698 - Make nsMsgMailNewsUrl (with six derived mailbox, IMAP, NNTP, SMTP, POP3, JsAccount URL) and other three comm-central URLs (address book, mailto and LDAP) future-proof

Status: NEW

(MailNews Core :: General, task)

Description

[Jorg K (CEST = GMT+2)]

C-C maintains four URL classes for address book, mailto, LDAP and a generic superclass nsMsgMailNewsUrl from which mailbox, IMAP, NNTP, POP3, SMTP and JsAccount URLs are derived. Note that JsAccount URLs are the basis for various add-ons that add protocols to TB, most notably to connect to MS Exchange servers. As the name says, JsAccount is implemented in JS.

There has been a lot of work undertaken in M-C to make nsIURI immutable and tread-safe.

Most M-C bugs caused great C-C bustage and the resulting fixes left us in a less then desirable state. Here some details with more to follow:

nsMsgMailNewsUrl is not really clone-able:
https://searchfox.org/comm-central/rev/16003487aabb9202fe21e4fc43777478d0dd916e/mailnews/base/util/nsMsgMailNewsUrl.cpp#604-627
since most of its members don't get cloned:
https://searchfox.org/comm-central/rev/16003487aabb9202fe21e4fc43777478d0dd916e/mailnews/base/util/nsMsgMailNewsUrl.h#105-130

It's also not serialisable:
https://hg.mozilla.org/comm-central/rev/e993eb9d7c1b74cf3506fe74be8b44fa5dfac86e#l1.54

which has to do with the derived classes that attach even more data to the object:
https://searchfox.org/comm-central/rev/16003487aabb9202fe21e4fc43777478d0dd916e/mailnews/imap/src/nsImapUrl.h#68-123
https://searchfox.org/comm-central/rev/16003487aabb9202fe21e4fc43777478d0dd916e/mailnews/compose/src/nsSmtpUrl.h#123-142

To set properties of the nsMsgMailNewsUrl we maintain various "direct access methods" that change members of the object:
https://searchfox.org/comm-central/rev/04a8c686bed0b09441be889250d32740d086522e/mailnews/base/public/nsIMsgMailNewsUrl.idl#30-39
Using a mutator to change those properties would call the incomplete clone which loses object properties, leading to logic errors. See usage here:
https://searchfox.org/comm-central/search?q=setspecinternal&case=false&regexp=false&path=mailnews%2F

Most recently, not being able to serialise has caused a total message display and hence MozMill failure in bug 1512356 where rudimentary serialisation now got implemented.

In bug 1512356 comment #26 Valentin stated:

  So, the main cause for all these issues is that nsIURIs have certain
  properties in m-c, that c-c URIs currently don't. m-c nsIURIs are
  completely immutable, and may be used off-main-thread. Sooner or
  later we will get to a point where a c-c implemented nsIURI is used
  off-main-thread on m-c code.

And referring to the "extra data":

  We had a similar issue with nsHostObjectURI (bug 1416791) which
  we fixed by removing the mutable/non-serializable members from the
  URI implementations - and moving them to another hashtable. So instead
  of uri.m_imapServerSink you'd have something like
  GetAssociatedInfo(uri)->m_imapServerSink and you might not even need to
  have any nsIURI implementations in c-c code.

For reference, a few bugs were we followed M-C:
Bug 1431913, bug 1432204, bug 1440693, bug 1470046, bug 1447272 and now bug 1512356.

In bug 1432204 I've already tried to fix the clone situation, but we came to the conclusion that it wasn't worth the effort, at least at the time, see bug 1432204 comment #30 and further comment from Kent James, author of JsAccount, in that bug (30, 31, 34, 36).

I think the entire area of URL use in Mailnews needs to be revisited, but it cannot be done under bustage-fix pressure.

Comment 1

[Ben Bucksch (:BenB)]

> So instead of uri.m_imapServerSink you'd have something like GetAssociatedInfo(uri)->m_imapServerSink and
> you might not even need to have any nsIURI implementations in c-c code.

Sounds good to me, if that works for us.

Comment 2

[Ben Bucksch (:BenB)]

> Sooner or later we will get to a point where a c-c implemented nsIURI is used off-main-thread on m-c code.

That means: We need to implement this soon. Otherwise, we may get very hard to debug race conditions that happen for some users sometimes, but cannot be reproduced by the tester nor developer, and they likely take a lot of time to be detected and then to be debugged, until we realize that the cause is the lack of this bug here. So, implementing this sooner rather than later is a good time investment for the project, and delaying this bug for long is not a good idea.

Comment 3

[Ben Campbell]

Is there any useful background on `nsMsgMailNewsUrl` etc all and the intention behind them?

I found:
https://developer.mozilla.org/en-US/docs/Mozilla/Thunderbird/MailNews_protocols
which helps a little.

Coming into it cold, I'd expect URL/URI classes to just handle parsing and manipulating URLs (with the specialised derived classes having various functions tailored to URLs of the related protocol).
But skimming through the source, it looks like there's a bunch of other data hanging off there too. I'm guessing most of that is the problematic mutable stuff?

And am I right in thinking that the issue is that sometime we might find that URLs originating in C-C are being used somewhere in another thread, while C-C is potentially still mutating them?

Comment 4

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

(In reply to Ben Campbell from comment #3)
> Coming into it cold, I'd expect URL/URI classes to just handle parsing and
> manipulating URLs (with the specialised derived classes having various
> functions tailored to URLs of the related protocol).
> But skimming through the source, it looks like there's a bunch of other data
> hanging off there too. I'm guessing most of that is the problematic mutable
> stuff?
> 
> And am I right in thinking that the issue is that sometime we might find
> that URLs originating in C-C are being used somewhere in another thread,
> while C-C is potentially still mutating them?

That is my concern as well.
It seems to me that these URI classes don't/rarely do any special parsing, so we could just replace them with nsSimpleURI/nsStandardURL and put the mutable fields in hashtables - the hash key could be the spec, or even the pointer to the URI object, if we make sure to update the table when we clone/destroy the object.

Comment 5

[Jorg K (CEST = GMT+2)]

So here's the little article I wanted to write for a long time.

Mailnews addresses messages in local mailboxes, on IMAP and NNTP servers are more recently accessed through protocols implemented in JS (JsAccount) through URLs (Uniform Resource Locators). Curiously enough it also addresses SMTP and POP3 servers. To implement that, we have a super-class nsMsgMailNewsUrl and six derived classes:
nsSmtpUrl, nsImapUrl, JaBaseCppUrl, nsMailboxUrl, nsPop3URL, nsNntpUrl.
These classes inherit directly from nsMsgMailNewsUrl:
https://dxr.mozilla.org/comm-central/search?q=public+nsMsgMailNewsUrl&redirect=false

These six classes are typically instantiated via do_CreateInstance() and some factory magic implemented in mailnews/base/util/nsMsgMailNewsUrl.cpp. Until recently, it wasn't possible to instantiate the super-class nsMsgMailNewsUrl, but more about that below (**).

nsMsgMailNewsUrl also implements various interfaces:
https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/base/util/nsMsgMailNewsUrl.h#41
most importantly nsIMsgMailNewsUrl which itself implements nsIURL:
https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/base/public/nsIMsgMailNewsUrl.idl#30
which implements nsIURI. Hence we have this collection here:
https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/base/util/nsMsgMailNewsUrl.h#47
Now, implementing the nsIURL interface means, that nsMsgMailNewsUrl must provide all the interfaces nsIURL requires.

nsMsgMailNewsUrl for this purpose holds this member variable: nsCOMPtr<nsIURL> m_baseURL;. This gets populated with a pointer to a nsStandardURL object here:
https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/base/util/nsMsgMailNewsUrl.cpp#403
NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID).SetSpec(aSpec).Finalize(m_baseURL);

All other manipulations of m_baseURL in that file take its immutability into account:
https://dxr.mozilla.org/comm-central/search?q=.Finalize(m_baseURL)+file%3AnsMsgMailNewsUrl.cpp&redirect=false
like for example: NS_MutateURI(m_baseURL).SetHost(aHost).Finalize(m_baseURL);

Note that nsMsgMailNewsUrl is *not* derived from nsStandardURL, the "extension" of that object is achieved via composition (https://en.wikipedia.org/wiki/Composition_over_inheritance).

The same is true for the other three URLs Mailnews and LDAP implement which are nsAddbookUrl, nsMailtoUrl and nsLDAPURL. Those only implement nsIURI. Address book and mailto only use a "simple URI" (nsSimpleURI()):
https://dxr.mozilla.org/comm-central/search?q=NS_SIMPLEURIMUTATOR_CONTRACTID

(**) Recently it has become a requirement for the URLs associated with DOM/docshell to be serialisable. So we needed to implement nsMsgMailNewsUrl::Write()/Read() and GetClassID() to enable the object factory to do its magic in creating an object. On this occasion I implemented the slight hack that nsMsgMailNewsUrl can now be instantiated in a generic fashion without being a mailbox, IMAP, NNTP, etc. object:
https://hg.mozilla.org/comm-central/rev/e0c27feaf097
See bug 1512356 for details.

So far the facts.

The next question is whether a composition of two objects needs to be made immutable if one of them is made immutable. Clearly the nsMsgMailNewsUrl and its derived objects carry a whole lot of extra information which is used for mail specific purposes. We should also keep in mind that one URL type, the one for JsAccount, is implemented via JS. Moving this off the main thread will be an interesting exercise. We could of course drop JsAccount, but I can already hear add-on authors screaming if we do. Further reading:
http://lists.thunderbird.net/pipermail/maildev_lists.thunderbird.net/2018-March/001052.html

Kent looked at this question in bug 1432204 comment #30 and further in that bug (30, 31, 34, 36). Picking a line from bug 1432204 comment #34:
  I suppose it would be worth at some point going through our code
  and refactoring it to more closely follow what they do in core Mozilla,
  but that is a big project, and it has little if any payoff as long as
  most of our protocols are main thread.

Comment 6

[Magnus Melin [:mkmelin]]

I wonder if it wouldn't be possible to just have an nsMsgMailNewsUrl type of object that wouldn't inherit anything, but just have the nsIURL member + other properties needed to store what we need?

Comment 7

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

(In reply to Magnus Melin [:mkmelin] from comment #6)
> I wonder if it wouldn't be possible to just have an nsMsgMailNewsUrl type of
> object that wouldn't inherit anything, but just have the nsIURL member +
> other properties needed to store what we need?

If that's possible it would be great!

Comment 8

[Jorg K (CEST = GMT+2)]

I'm not sure what you mean by "inherit". As I detailed above, nsMsgMailNewsUrl doesn't inherit. It extends and implements the interfaces required by nsIURI.

Other than that, I don't think so. I you want to tell Gecko to open/display a document for you, you need to give it something can be QI'ed to nsIURI, see all the LoadURI() calls we have:
https://dxr.mozilla.org/comm-central/search?q=loaduri+path%3Acomm%2F&redirect=false

Comment 9

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

(In reply to Jorg K (GMT+1) from comment #8)
> I'm not sure what you mean by "inherit". As I detailed above,
> nsMsgMailNewsUrl doesn't inherit. It extends and implements the interfaces
> required by nsIURI.
> 
> Other than that, I don't think so. I you want to tell Gecko to open/display
> a document for you, you need to give it something can be QI'ed to nsIURI,
> see all the LoadURI() calls we have:
> https://dxr.mozilla.org/comm-central/
> search?q=loaduri+path%3Acomm%2F&redirect=false

I think he is asking what if nsMsgMailNewsUrl didn't implement nsIURI. Do things still work if you pass m_baseURL to LoadURI?

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to Valentin Gosu [:valentin] from comment #9)
> I think he is asking what if nsMsgMailNewsUrl didn't implement nsIURI. Do
> things still work if you pass m_baseURL to LoadURI?
I don't think so. C-C calls into M-C with the object that implements C-C functionality as well as nsIURI. M-C then calls back into C-C code with that object and we QI it to the interface we need. The callback needs to get the object we started with.

Comment 11

[Ben Campbell]

I've been digging about in the NNTP code recently, so this is flavoured by that, but I think the general approach applies roughly across all the protocols we have URI types for. I'll just talk about NNTP, but extrapolate that out to IMAP, POP3 etc...

NNTP operations are executed upon nsNNTPProtocol objects, which are connections, persistent over many operations.
There is also nsNntpMockChannel, which is used to queue up operations when a connection isn't immediately available.
It's a little more complicated by the fact that both nsNNTPProtocol and nsMockChannel can be used either as nsIChannels, or to issue commands directly, via LoadUrl()... but in general there's this concept of persistent, poolable objects to manage the connection/parsing/statemachine/whatever (nsNNTPProtocol), and one-shot objects which represent a single operation/request (nsNntpMockChannel).

In this case, I'd say that the correct thing would be to move all the news URL state out into nsNntpMockChannel.
(among a whole heap of other NNTP cleanups/refactorings ;- )
Of course it's complicated because a lot of the implementation is down in the base classes (nsMsgMailNewsUrl and nsMsgProtocol) so you'd have to start there. Meaning all the protocols would be impacted at once, which is a bit of an arse.

But I think all the protocols could be massaged to separate out representation of the connection from representation of a individual request/operation/action/whatever. This would enable us to refactor - starting with the base classes - and move all the request state tracking out of the URI classes to allow them to become immutable, like the M-C URIs.