The default bug view has changed. See this FAQ.

Add a mail services C++ helper

RESOLVED FIXED in Thunderbird 23.0

Status

MailNews Core
Backend
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

Trunk
Thunderbird 23.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Mozilla-central has mozilla/Services.h, so comm-central should get a mozilla/mailnews/Services.h as well.

A simple grep of our codebase indicates that the following contracts are used more than once:
189 NS_PREFSERVICE_CONTRACTID
 90 NS_MSGACCOUNTMANAGER_CONTRACTID
 55 NS_ABMANAGER_CONTRACTID
 53 NS_MSGMAILSESSION_CONTRACTID
 43 NS_IMAPSERVICE_CONTRACTID
 36 NS_MSGNOTIFICATIONSERVICE_CONTRACTID
 27 NS_MSGDB_SERVICE_CONTRACTID
 27 NS_MAILNEWS_MIME_HEADER_PARSER_CONTRACTID
 25 NS_IMPORTSERVICE_CONTRACTID
 24 NS_MSGCOPYSERVICE_CONTRACTID
 21 NS_WINDOWWATCHER_CONTRACTID
 21 kRDFServiceCID
 18 "@mozilla.org/rdf/rdf-service;1"
 13 NS_MIME_CONVERTER_CONTRACTID
 13 kCImapHostSessionListCID
 12 NS_MSGCOMPOSESERVICE_CONTRACTID
 12 NS_CHARSETCONVERTERMANAGER_CONTRACTID
 10 NS_SMTPSERVICE_CONTRACTID
 10 NS_MSGFILTERSERVICE_CONTRACTID
  9 NS_NNTPSERVICE_CONTRACTID
  9 NS_AUTOSYNCMANAGER_CONTRACTID
  8 NS_MIMESERVICE_CONTRACTID
  8 NS_LOGINMANAGER_CONTRACTID
  8 "@mozilla.org/streamConverters;1"
  7 NS_CATEGORYMANAGER_CONTRACTID
  6 NS_WINDOWMEDIATOR_CONTRACTID
  6 NS_ADDRDATABASE_CONTRACTID
  5 NS_POP3SERVICE_CONTRACTID1
  5 NS_PARSERUTILS_CONTRACTID
  5 NS_MSGTAGSERVICE_CONTRACTID
  5 NS_MIMEHEADERPARAM_CONTRACTID
  5 NS_DIRECTORY_SERVICE_CONTRACTID
  5 "@mozilla.org/msg-trait-service;1"
  5 kCPop3ServiceCID
  4 NS_PROMPTSERVICE_CONTRACTID
  4 NS_MSGASYNCPROMPTER_CONTRACTID
  4 NS_MESSENGERWINDOWSERVICE_CONTRACTID
  4 NS_LOCALESERVICE_CONTRACTID
  4 MSGVCARDSERVICE_CONTRACT_ID
  4 "@mozilla.org/observer-service;1"
  3 NS_X509CERTDB_CONTRACTID
  3 NS_URI_LOADER_CONTRACTID
  3 NS_STREAMTRANSPORTSERVICE_CONTRACTID
  3 NS_PLATFORMCHARSET_CONTRACTID
  3 NS_NETUTIL_CONTRACTID
  3 NS_MORK_CONTRACTID
  3 NS_ABDIRFACTORYSERVICE_CONTRACTID
  3 "@mozilla.org/abmanager;1"
  3 kCImapHostSessionList
  2 NS_STRINGBUNDLE_CONTRACTID
  2 NS_SOCKETTRANSPORTSERVICE_CONTRACTID
  2 NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX "http"
  2 NS_MSGPURGESERVICE_CONTRACTID
  2 NS_MSGCOMPUTILS_CONTRACTID
  2 NS_MSGBIFFMANAGER_CONTRACTID
  2 NS_MESSENGEROSINTEGRATION_CONTRACTID
  2 NS_ITEXTTOSUBURI_CONTRACTID
  2 NS_IOSERVICE_CONTRACTID
  2 NS_EXTERNALPROTOCOLSERVICE_CONTRACTID
  2 NS_APPSTARTUP_CONTRACTID
  2 NS_ALERTSERVICE_CONTRACTID
  2 NS_ABLDIFSERVICE_CONTRACTID
  2 "@mozilla.org/newsblog-feed-downloader;1"
  2 "@mozilla.org/file/directory_service;1"
  2 "@mozilla.org/atom-service;1"
  2 "@mozilla.org/addressbook/ldap-attribute-map-service;1"
  2 kCMsgComposeServiceCID

(Clearly, there are some people who could use mozilla::Preferences or other m-c-Services.h here, although we probably need an external API copy of mozilla::Preferences before migrating).

For starters, I've copied the list from mailServices.jsm into a ServiceList.h for comm-central. The junk mail plugin is insufficiently used to justify inclusion in the C++ version; the message DB service and the import service are the most heavily used interfaces not included on that list. After those, the next-highest pure comm-central interfaces (excluding IMAP's CID-only interface) is the address database, which is arguably below the interesting threshold.

It's debatable if we want to add accessors for m-c interfaces we use heavily that aren't in the m-c Services.h.
(Assignee)

Comment 1

4 years ago
Created attachment 732171 [details] [diff] [review]
Add the services

This adds the list from mailServices.js, plus the database and import service additions and minus the junk mail plugin.

I don't use it anywhere yet, but I'm using this in my current patches for the nsIMsgHeaderParser bug, so I can at least tell you that at least nsIMsgHeaderParser works and that I don't leak anything at the end. I figure aceman or Archaeopteryx can go crazy with rewriting the codebase.

[Note: I would like to ask anyone who does the conversions to avoid converting nsIMsgHeaderParser uses for now, as I have my own plans in patches to be posted hopefully very shortly].

I've eschewed the mailServices names and generally gone with the "base name" of the interface (the interface minus nsI[Msg]), with the exception of mozINewMailNotificationService and nsIMsgFolderNotificationService, since those names are so long. I reused MFNService from mailServices for the later (I couldn't think of anything else, so at least it's relatively consistent), and MailNotifyService I hope captures the intent of the other one.

Part of the patch could probably use some more detailed explanation. I use mozilla::services to make it transparent as to whether an interface is in mozilla/ServiceList.h or mozilla/mailnews/ServiceList.h. The original implementation hooks directly into the XPCOM shutdown process to work properly, which we don't have the luxury of doing in comm-central, so I used an observer instead.

The choice of the topic isn't the usual xpcom-shutdown notification (which could cause it to die while we're shutting down mail accounts) but rather a later shutdown notification that appears to be fairly close to when the observer service itself shuts down. It might be possible to get a more accurate shutdown time by using the observer's destructor instead, but I reckon this is close enough and moderately less hacky.

Discussion on IRC lead to the possibility of making a more general mozilla-central solution for this sort of stuff; migration would be easy if one is added, but I'm not trying to add it now to get it in the tree faster.
Attachment #732171 - Flags: review?(neil)

Comment 2

4 years ago
Is the goal of this the same as the importing of mailServices.js in js files - sharing of the objects and speed improvement? If so, can you describe here how these new services are to be used from C++?
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk

Comment 3

4 years ago
Comment on attachment 732171 [details] [diff] [review]
Add the services

>+// IWYU pragma: private, include "mozilla/mailnews/Services.h"
How about
#ifndef MOZ_SERVICE
#error Use #include "mozilla/mailnews/Services.h" instead.
#endif

>+MOZ_SERVICE(AbManager,         nsIAbManager, "@mozilla.org/abmanager;1")
Nit: wrap this like the others for consistency.

>+class ServiceShutdownObserver MOZ_FINAL : public nsIObserver {
Why not hook into msgMailNewsModuleDtor()?

>+      nsCOMPtr<TYPE> os = do_GetService(CONTRACT_ID); \
>+      g##NAME = os.forget().get(); \
[I wonder why thy didn't use CallGetService(CONTRACT_ID, &g##NAME); or at the very least os.forget(&g##NAME);]

>+mozilla::DebugOnly<bool> ServiceShutdownObserver::sShuttingDown = false;
Doesn't this create a static constructor/destructor?

>+void ServiceShutdownObserver::RegisterShutdown()
>+{
>+  MOZ_ASSERT(!sShuttingDown, "It is illegal to use this code after shutdown!");
[Doesn't do_GetService stop working after some point anyway?]

Comment 4

4 years ago
Do we use it like this?
#include "mozilla/Services.h"
nsCOMPtr<nsIObserverService> observerService =
  mozilla::services::GetObserverService();
NS_ENSURE_TRUE(observerService, NS_ERROR_UNEXPECTED);
(Assignee)

Comment 5

4 years ago
(In reply to neil@parkwaycc.co.uk from comment #3)
> >+// IWYU pragma: private, include "mozilla/mailnews/Services.h"
> How about
> #ifndef MOZ_SERVICE
> #error Use #include "mozilla/mailnews/Services.h" instead.
> #endif

I copied this line from the m-c ServiceList.h file. It's for the "include-what-you-use" tool, which could end up suggesting including mozilla/mailnews/ServiceList.h for the Get*Service methods instead of mozilla/mailnews/Services.h

> >+class ServiceShutdownObserver MOZ_FINAL : public nsIObserver {
> Why not hook into msgMailNewsModuleDtor()?

Hmm, that would clean up the code and probably runs at the right time as well.

> >+      nsCOMPtr<TYPE> os = do_GetService(CONTRACT_ID); \
> >+      g##NAME = os.forget().get(); \
> [I wonder why thy didn't use CallGetService(CONTRACT_ID, &g##NAME); or at
> the very least os.forget(&g##NAME);]
> 
> >+mozilla::DebugOnly<bool> ServiceShutdownObserver::sShuttingDown = false;
> Doesn't this create a static constructor/destructor?

It looks like -O1 with clang eliminates the static constructor (the destructor is still trivial), but the entire purpose of ::DebugOnly is to stop warnings about unused variables, so it looks like I can drop it altogether.

> >+void ServiceShutdownObserver::RegisterShutdown()
> >+{
> >+  MOZ_ASSERT(!sShuttingDown, "It is illegal to use this code after shutdown!");
> [Doesn't do_GetService stop working after some point anyway?]

Yes, it does... but the shutdown here could be running before that portion of xpcom shutdown. Since I'm moving it to the end of the msgMailNewsDtor(), it should be less of a problem, but catching footguns earlier is better. :-)
(Assignee)

Comment 6

4 years ago
Created attachment 732569 [details] [diff] [review]
Better handling

I think I addressed all the comments.
Attachment #732171 - Attachment is obsolete: true
Attachment #732171 - Flags: review?(neil)
Attachment #732569 - Flags: review?(neil)

Comment 7

4 years ago
Comment on attachment 732569 [details] [diff] [review]
Better handling

>+} // namespace services
>+
>+namespace mailnews {
[So, why some stuff in services and some in mailnews?]
Attachment #732569 - Flags: review?(neil) → review+
(Assignee)

Comment 8

4 years ago
mozilla::services is in case we ever move the ServicesList.h stuff to the main one by some app-specific hook. I put the ShutdownServices in mailnews because it's only shutting down mailnews services (and I don't want to chance colliding with a m-c name).
(Assignee)

Comment 9

4 years ago
(In reply to Joshua Cranmer [:jcranmer] from comment #5)
> > >+class ServiceShutdownObserver MOZ_FINAL : public nsIObserver {
> > Why not hook into msgMailNewsModuleDtor()?
> 
> Hmm, that would clean up the code and probably runs at the right time as
> well.

It turns out that this isn't quite the right place either, since if I do it here, a JS-implemented service crashes on shutdown thanks to xpconnect being shutdown already.
(Assignee)

Comment 10

4 years ago
Created attachment 734295 [details] [diff] [review]
Listen at xpcom-shutdown-threads

Per discussion with bsmedberg over IRC, xpcom-shutdown-threads appears to be the best place to listen to this. A rough timeline of XPCOM shutdown for reference:

xpcom-will-shutdown
xpcom-shutdown
(Startup cache shuts down)
xpcom-shutdown-threads
(Cycle collector, timer, thread manager threads shut down, spinning event loop)
(Observer service shuts down, libxul-internal ClearOnShutdown happens, mozilla::services shut down)

Services freed, cycle collector shuts down
xpcom-shutdown-loaders (yes, after the observer service dies)
Write poisoning starts
Everything else shuts down..

Note that most of the mailnews code shuts down on xpcom-shutdown, so none of our code should be running during shutdown-threads (the IMAP thread shouldn't be running, but it might be, though).
Attachment #732569 - Attachment is obsolete: true
Attachment #734295 - Flags: review?(neil)

Updated

4 years ago
Attachment #734295 - Flags: review?(neil) → review+
(Assignee)

Comment 11

4 years ago
Finally pushed:
https://hg.mozilla.org/comm-central/rev/aa06f3a652c1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0

Comment 12

4 years ago
jcranmer, can you now please tell how we should use this?
(Assignee)

Comment 13

4 years ago
(In reply to :aceman from comment #12)
> jcranmer, can you now please tell how we should use this?

nsCOMPtr<nsIMsgAccountManager> am = mozilla::services::GetAccountManager();

Basically, replace do_GetService with mozilla::services::Get*. That said, hold off on switching nsIMsgHeaderParser stuff since my own patches on bug 842632 tackle all of those callsites.

Comment 14

4 years ago
OK, thanks.

Comment 15

4 years ago
How is error checking done on this now? rv from do_GetService() is probably out, so only check the returned service for null?
Flags: needinfo?(Pidgeot18)
(Assignee)

Comment 16

4 years ago
(In reply to :aceman from comment #15)
> How is error checking done on this now? rv from do_GetService() is probably
> out, so only check the returned service for null?

In general, you shouldn't need to check for errors, but null-check if you really want to.
Flags: needinfo?(Pidgeot18)
You need to log in before you can comment on or make changes to this bug.