Last Comment Bug 856914 - Add a mail services C++ helper
: Add a mail services C++ helper
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 23.0
Assigned To: Joshua Cranmer [:jcranmer]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-01 19:59 PDT by Joshua Cranmer [:jcranmer]
Modified: 2013-07-06 13:09 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Add the services (7.32 KB, patch)
2013-04-01 20:33 PDT, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Review
Better handling (7.00 KB, patch)
2013-04-02 15:41 PDT, Joshua Cranmer [:jcranmer]
neil: review+
Details | Diff | Review
Listen at xpcom-shutdown-threads (7.52 KB, patch)
2013-04-06 14:33 PDT, Joshua Cranmer [:jcranmer]
neil: review+
Details | Diff | Review

Description Joshua Cranmer [:jcranmer] 2013-04-01 19:59:40 PDT
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.
Comment 1 Joshua Cranmer [:jcranmer] 2013-04-01 20:33:57 PDT
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.
Comment 2 :aceman 2013-04-01 23:56:43 PDT
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++?
Comment 3 neil@parkwaycc.co.uk 2013-04-02 03:01:30 PDT
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 :aceman 2013-04-02 03:43:40 PDT
Do we use it like this?
#include "mozilla/Services.h"
nsCOMPtr<nsIObserverService> observerService =
  mozilla::services::GetObserverService();
NS_ENSURE_TRUE(observerService, NS_ERROR_UNEXPECTED);
Comment 5 Joshua Cranmer [:jcranmer] 2013-04-02 08:13:08 PDT
(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. :-)
Comment 6 Joshua Cranmer [:jcranmer] 2013-04-02 15:41:14 PDT
Created attachment 732569 [details] [diff] [review]
Better handling

I think I addressed all the comments.
Comment 7 neil@parkwaycc.co.uk 2013-04-03 06:48:59 PDT
Comment on attachment 732569 [details] [diff] [review]
Better handling

>+} // namespace services
>+
>+namespace mailnews {
[So, why some stuff in services and some in mailnews?]
Comment 8 Joshua Cranmer [:jcranmer] 2013-04-04 20:53:39 PDT
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).
Comment 9 Joshua Cranmer [:jcranmer] 2013-04-06 12:45:11 PDT
(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.
Comment 10 Joshua Cranmer [:jcranmer] 2013-04-06 14:33:44 PDT
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).
Comment 11 Joshua Cranmer [:jcranmer] 2013-04-13 10:15:36 PDT
Finally pushed:
https://hg.mozilla.org/comm-central/rev/aa06f3a652c1
Comment 12 :aceman 2013-04-13 12:35:12 PDT
jcranmer, can you now please tell how we should use this?
Comment 13 Joshua Cranmer [:jcranmer] 2013-04-13 14:17:59 PDT
(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 :aceman 2013-04-13 14:56:38 PDT
OK, thanks.
Comment 15 :aceman 2013-07-05 06:09:19 PDT
How is error checking done on this now? rv from do_GetService() is probably out, so only check the returned service for null?
Comment 16 Joshua Cranmer [:jcranmer] 2013-07-06 13:09:07 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.