bsmedberg has declared that he wants to remove XPCOM proxies, and he said he intends to land it for Firefox 9 (so we should have a few weeks before our code breaks horribly). grep is telling me that we use XPCOM proxies in: ldap/xpcom/src/nsLDAPConnection.cpp [just an include, it appears] ldap/xpcom/src/nsLDAPSyncQuery.cpp [mOperation->Init proxying] mailnews/addrbook/src/nsAbLDAPReplicationQuery.cpp [ditto] mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp [ditto] mailnews/addrbook/src/nsLDAPAutoCompleteSession.cpp [ditto] mailnews/addrbook/src/nsAbLDAPDirectoryModify.cpp [ditto] mailnews/addrbook/src/nsAbLDAPListenerBase.cpp [ditto] mailnews/addrbook/src/nsAbLDAPReplicationData.cpp [ditto] mailnews/addrbook/src/nsAddrDatabase.cpp [AddListDirNode proxies to parent] mailnews/addrbook/src/nsAbOutlookDirectory.cpp [Query results] mailnews/mapi/mapihook/src/msgMapiHook.cpp [just an include, it appears] mailnews/imap/src/nsImapProtocol.cpp [most of the sinks] mailnews/base/util/nsMsgUtils.h [NS_GetProxyForObject redeclaration] mailnews/import/oexpress/nsOEStringBundle.cpp [string bundle] mailnews/import/winlivemail/nsWMStringBundle.cpp [ditto] mailnews/import/eudora/src/nsEudoraStringBundle.cpp [ditto] mailnews/import/outlook/src/nsOutlookStringBundle.cpp [ditto] mailnews/import/eudora/src/nsEudoraCompose.cpp [needs it for nsIMsgSend] mailnews/import/outlook/src/nsOutlookCompose.cpp [ditto] [uh... why do we need send for import here?] mailnews/import/src/nsImportMail.cpp [folders get proxied] mailnews/import/src/nsImportAddressBooks.cpp [database gets proxied] mailnews/import/src/nsImportStringBundle.cpp [string bundle] mailnews/import/comm4x/src/nsComm4xMailImport.cpp [ditto] mailnews/import/applemail/src/nsAppleMailImport.cpp [ditto] mailnews/extensions/smime/content/certFetchingStatus.js [ldap operation proxy] mailnews/mime/src/mimecms.cpp [nsIMsgSMIMEHeaderSink proxy] Now, if your guess for how well tested most of this code is was "almost none", you are correct (nsImapProtocol is the best tested, but not 100%). We have two major uses of proxies: 1. Proxying nsILDAPMessageListener. nsILDAPConnection and nsILDAPOperation should probably dispatch the calls onto the proper thread themselves. 2. Import's string bundles, not to mention the entire folder/database proxy. These two cases account for all but 9 cases (7, if you exclude the nsIMsgSend deal, and 4 if you only count real uses).

We need send for import because that's how Outlook and Eudora import work - they create a compose window with the message contents, and do a fake send to create the rfc822 message. From my p.o.v., the imap code is the major user of xpcom proxies because it uses blocking proxies, which is more complicated then simply dispatching calls onto the proper thread. We used to do our own hand-rolling of the proxy stuff and it was quite painful.

bug 675221 is the mozilla-central bug. It doesn't look like the PSM stuff has happened, which is by far the hardest piece, so I'm a little doubtful that this will make FF 9.

This is going to be a fair chunk of work (on the order of a couple weeks or more, most likely), so I would suggest that we account for it in any schedules we make.

Sounds like we need a better API for creating RFC 822 envelopes than nsIMsgSend :-). Excluding non-real uses, the easiest one to fix is probably the LDAP stuff: it accounts for about half the use of proxies, and it's pretty much just a single method to change, and it doesn't look like it relies on synchronous callback, although the onInit takes and nsILDAPConnection as callback, it doesn't look like any implementers actually do something with it (?). The import string bundles look like they're for logging, and can probably be fixed by loading all the strings up front or shunting the logging back to the main thread. The other uses of import's proxies are much harder. Considering that the code is essentially broken right now (it mangles non-threadsafe objects from multiple threads, at least for the address book), it may be a good idea to take the time to redesign the import to eliminate the need for proxies and allow us to legally test them. Another point is that the vcard service uses global variables for parsing, so it truly is non-threadsafe--nsAbManager should hold a lock on that or something. That leaves: IMAP - difficult :-( nsAddrDatabase - I think this is related to import's mangling nsAbOutlookDirectory - didn't look too hard, but async dispatch should be fine mimecms - I can't tell (mime isn't terribly lucid), but async looks sufficient? Possibly 3 easy cases and 5 (breaking up the import) difficult cases. This does look like it will take weeks.

now that bug 675221 is auguring in, time to start on this. This does seem like a good time to see what architectural improvements we can make, so that this effort isn't a complete waste of time.

As always, once one starts digging into things, it gets quite a bit more complicated. In the LDAP code, half the callers create an async proxy, the other half create a synchronous proxy for the init callback. Maybe they all can be async, or sync. All of the proxies proxy to the main thread, except for nsAbLDAPReplicationQuery, which proxies to the current thread. I don't think Thunderbird currently does any ldap replication, though an extension could do so, and the replication code is built into Thunderbird. My recollection is that ldap replication did run on a separate thread back when we did support it, because it could be quite cpu intensive. As nice as it would be to rip out the ldap replication code, it's important for enterprises. All of which complicates the proxy replacement code.

Er, my mistake, we do have a UI for ldap replication (the download now button in an ldap directory's property dialog), but that's going to be the UI thread.

most of the ldap init proxies are sync, and I suspect that's because Init can do things like prompt for the ldap password, which needs to be sync. Only autocomplete doesn't do a sync proxy. I'll try making everyone do a sync proxy to start with, and if that causes problems with autocomplete, I'll figure out a way of having the listener know how it should proxy.

import uses the strings for more than just logging, and I think there are a sufficient number of strings that pre-flighting their loading is not a great approach for code-maintainability.

this remove the ldap use of xpcom proxies, in ldap/xpcom, and mailnews/addrbook.

I wonder if the proxies are needed for string bundles at all. They look like they're trying to be thread-safe.

Comment on attachment 552001 [details] [diff] [review] remove ldap proxy stuff >+nsOnLDAPMessageRunnable::nsOnLDAPMessageRunnable(nsILDAPMessageListener *aListener, >+ nsILDAPMessage *aMsg) >+{ >+ m_msg = aMsg; >+ m_listener = aListener; >+} Nit: should use : m_msg(aMsg), m_listener(aListener) {} syntax. >+nsOnLDAPInitMessageRunnable::nsOnLDAPInitMessageRunnable(nsILDAPMessageListener *aListener, >+ nsILDAPConnection *aConn, >+ nsresult aStatus) >+{ >+ m_listener = aListener; >+ m_conn = aConn; >+ m_status = aStatus; Similarly here. r=me with that fixed.

carrying forward Neil's r+. This patch addresses his comments about the constructor initializers, and removes some more unneeded header files.

It's a bit hard to test this, but I don't think we need any of the string bundle proxies in the import code. I'll try to generate a try server build and get some QA help. I'll fix the folder proxy stuff first...

the five users of NS_WITH_PROXIED_SERVICE need to be fixed as well.

ldap/addrbook xpcom proxy use removed - http://hg.mozilla.org/comm-central/rev/8f56b6d5754c

turns out that nsIArray is not thread-safe, so I had to replace the ProxySend use of nsIArray with nsISupportsArray. I'll try to put up a patch with that change in a bit.

Address book import runs on a separate thread, but it does several unsafe things, like accessing the db and pref service off the main thread. I'm strongly tempted to put it back on the UI thread, since it's highly unlikely that address books are going to be sufficiently large to cause an issue imported in a blocking way. If it becomes an issue, the affected address book importer(s) can be tweaked to do timeslicing.

Outlook isn't working on my machine, so I can't test this too much, but it works for ldif import. This patch makes address book import run on the UI thread

(In reply to David :Bienvenu from comment #18) > turns out that nsIArray is not thread-safe, so I had to replace the > ProxySend use of nsIArray with nsISupportsArray. I'll try to put up a patch > with that change in a bit. Given we're meant to be removing nsISupportsArray (bug 394167 and others) I'm a bit concerned by this. It sounds like we should get a core bug on that if nothing else.

I've got to do the message and server sinks, but I've got all the infrastructure in place so that this should be pretty easy to knock out.

this finishes imap, I believe. There are still a few proxy calls in the main import code that I need to fix.

There's still a bit of cleanup to do for imap, mostly removing unused macros.

Comment on attachment 556745 [details] [diff] [review] finish getting rid of imap proxies Review of attachment 556745 [details] [diff] [review]: ----------------------------------------------------------------- More drive-by reviewing... I suspect there are ways that involve less code (e.g., more or less copy the current proxy mechanism, since this is as close as you'd get to an ideal use case), but I doubt we want to maintain that much. ::: mailnews/imap/src/nsSyncRunnableHelpers.cpp @@ +55,5 @@ > +template<typename T> > +struct RefType > +{ > + typedef T& type; > +}; Do you really need the specialization for reference types? According to my copy of the C++ spec typedef T &type; should produce T if T is itself a reference type: If a typedef (7.1.3), a type template-parameter (14.3.1), or a decltype-specifier (7.1.6.2) denotes a type TR that is a reference to a type T, an attempt to create the type “lvalue reference to cv TR” creates the type “lvalue reference to T”, while an attempt to create the type “rvalue reference to cv TR” creates the type TR. @@ +133,5 @@ > + > + > +template<typename Receiver, typename Arg1> > +class SyncRunnable1 : public SyncRunnableBase > +{ I would dearly love to see variadic templates here (especially having discovered that we implicitly use them elsewhere via static analysis failures), but it appears that MSVC still lacks support for variadic templates. Ah well... @@ +135,5 @@ > +template<typename Receiver, typename Arg1> > +class SyncRunnable1 : public SyncRunnableBase > +{ > +public: > + typedef nsresult (NS_STDCALL Receiver::*ReceiverMethod)(Arg1); At second glance, it seems like NS_STDCALL_FUNCPROTO is what you really want here. @@ +713,5 @@ > +NS_SYNCRUNNABLEMETHOD1(ImapServerSink, GetArbitraryHeaders, nsACString &) > +NS_SYNCRUNNABLEMETHOD0(ImapServerSink, ForgetPassword) > +NS_SYNCRUNNABLEMETHOD1(ImapServerSink, GetShowAttachmentsInline, PRBool *) > +NS_SYNCRUNNABLEMETHOD3(ImapServerSink, CramMD5Hash, const char *, const char *, char **) > +NS_SYNCRUNNABLEMETHOD1(ImapServerSink, GetLoginUsername, nsACString &) I'm trying to think if there's any way to avoid this large mess of methods using NS_IMPL_FORWARD or something similar as opposed to manually listing everything, but nothing is coming up immediately. I have some ideas which would be wanton abuse of the C preprocessor, but I suspect those border on the unmaintainable.

The sync runnable macros are taken directly from bsmedberg's patch. If you have a problem with them, I highly suggest commenting on his patch, because I'm going to follow what he does, to minimize the amount of non-productive work this is causing.

this cumulative patch gets rid of all xpcom proxies in the mailnews code, I believe.

apparently, there's one left in certFetchingStatus.js, so I'll need to figure out how to do event dispatching from js threads.

The one in JS is yet another use of the LDAP operation stuff, so I believe that means it can just be removed.

Comment on attachment 554999 [details] [diff] [review] get rid of proxies used in address books, including import - checked in Moving this all to the UI thread is a little scary, but I think its acceptable given the normal sizes of address books we have, and that most of it is already on the UI thread anyway (or should be, especially wrt mork). If we find it an issue, we can always rework later, or batch it up with events. I think we should also have a follow-up bug on removing the ImportAddressThread function & associated support info. I'm sure from past experience that there's a lot of stuff we don't need in there if everything is on the UI thread. >diff --git a/mailnews/import/src/nsImportAddressBooks.cpp b/mailnews/import/src/nsImportAddressBooks.cpp > #include "nsProxiedService.h" nit: We can probably remove this include as well.

https://bugzilla.mozilla.org/attachment.cgi?id=554999 checked in http://hg.mozilla.org/comm-central/rev/2733bbfdc64b

Comment on attachment 557606 [details] [diff] [review] per jcranmer, remove proxy for ldap init I would say "add a test", but this is LDAP and S/MIME, where we really don't have frameworks for any tests yet ;-) .

https://bugzilla.mozilla.org/attachment.cgi?id=557606 checked in - http://hg.mozilla.org/comm-central/rev/ed9d5de23537

this is what I have left in my repo, after landing a couple patches, and finding a couple more things I had to fix.

import changes pushed - http://hg.mozilla.org/comm-central/rev/f7fd0a2c188a

Mostly these are the imap fixes to do hand-rolled proxying to the ui thread, removing some unneeded headers, and fixing the remaining import code to use hand-rolled proxies.

This patch broke parallel builds because nsIImportService.idl #includes nsIMsgSend.idl, but nsIMsgSend.idl is in mailnews/compose which is built in parallel with mailnews/import.

(In reply to Siddharth Agarwal [:sid0] from comment #40) > This patch broke parallel builds because nsIImportService.idl #includes > nsIMsgSend.idl, but nsIMsgSend.idl is in mailnews/compose which is built in > parallel with mailnews/import. Pushed a bustage fix: https://hg.mozilla.org/comm-central/rev/28ce5166f504

(In reply to Siddharth Agarwal [:sid0] from comment #41) > (In reply to Siddharth Agarwal [:sid0] from comment #40) > > This patch broke parallel builds because nsIImportService.idl #includes > > nsIMsgSend.idl, but nsIMsgSend.idl is in mailnews/compose which is built in > > parallel with mailnews/import. > > Pushed a bustage fix: https://hg.mozilla.org/comm-central/rev/28ce5166f504 ... though if there's a way to re-parallelize this, that would be welcome too.

We need this patch either to land on the trunk before the next aurora branch, or we need to land this on the aurora branch after it is cut. Without it, Eudora import can crash...

Comment on attachment 561045 [details] [diff] [review] fix crash in eudora import >diff --git a/mailnews/import/eudora/src/nsEudoraCompose.cpp b/mailnews/import/eudora/src/nsEudoraCompose.cpp >- if (pAttach) >- delete [] pAttach; >- I think I see why you're doing this, but won't it end up leaking memory? Could we delete it in nsProxySendRunnable::Run instead, with a note on the idl as to what's happening?

(In reply to Mark Banner (:standard8) from comment #44) > Comment on attachment 561045 [details] [diff] [review] > fix crash in eudora import > > >diff --git a/mailnews/import/eudora/src/nsEudoraCompose.cpp b/mailnews/import/eudora/src/nsEudoraCompose.cpp > >- if (pAttach) > >- delete [] pAttach; > >- > > I think I see why you're doing this, but won't it end up leaking memory? > > Could we delete it in nsProxySendRunnable::Run instead, with a note on the > idl as to what's happening? It's ref-counted now.

Comment on attachment 561045 [details] [diff] [review] fix crash in eudora import this landed already.

Comment on attachment 559667 [details] [diff] [review] remaining fixes + nsRefPtr<ImapMailFolderSinkProxy> m_imapMailFolderSink; + nsRefPtr<ImapMessageSinkProxy> m_imapMessageSink; + nsRefPtr<ImapServerSinkProxy> m_imapServerSink; + nsRefPtr<ImapProtocolSinkProxy> m_imapProtocolSink; I've unbitrotted this patch, but I can't find the *SinkProxy definitions anywhere, did you miss including a file?

I'll try to roll all these into a patch that applies and builds

I also fixed some PRbool stuff in import

Note to self - make sure that we're using the same mechanism for sync proxying as in bug 675221, the moz-central equivalent of this bug, since the mechanism they're using doesn't involve the event queue. It's not vital that we land with the same mechanism, since the current xpcom proxying uses the event queue, so we're no worse off. But it could be a nice enhancement.

Note: the mechanism in XPCOM proxies involved using an event filter so that we only ran events related to XPCOM proxies and their perhaps nested call stacks. This is necessary because of the possibility of arbitrary reentrancy or deadlock across multiple threads using the proxies. The mechanism for PSM in bug 675221 blocks the calling thread unconditionally and does not allow for any reentrancy, which is perfectly fine in that case because we know the calling patterns and know that the main thread cannot block on the PSM thread. The mechanism you need will depend on the blocking/calling patterns of the code in question. But the point of removing proxies was to remove the event filtering code, so please don't use event filters.

Thx for the info, Benjamin. The imap thread code does not ever get called re-entrantly. The only case where we may require the UI thread to process events in a sync proxy call is when we put up an alert and even that shouldn't be blocking (or even an alert). The password stuff is all async now. If we use the approach that PSM is using, does that mean that the UI event queue won't get run during a sync call? (In reply to Benjamin Smedberg [:bsmedberg] from comment #51) > Note: the mechanism in XPCOM proxies involved using an event filter so that > we only ran events related to XPCOM proxies and their perhaps nested call > stacks. This is necessary because of the possibility of arbitrary reentrancy > or deadlock across multiple threads using the proxies. > > The mechanism for PSM in bug 675221 blocks the calling thread > unconditionally and does not allow for any reentrancy, which is perfectly > fine in that case because we know the calling patterns and know that the > main thread cannot block on the PSM thread. > > The mechanism you need will depend on the blocking/calling patterns of the > code in question. But the point of removing proxies was to remove the event > filtering code, so please don't use event filters.

Which way is the call going?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #53) > Which way is the call going? All our proxy calls go from the imap thread to the UI thread. There's no way to call into the imap thread; it sits waiting on a monitor for things to do, and when notified, it gets the url and runs it.

Then the imap thread would be blocked waiting on a response from the UI thread. The UI thread would spin normally, and there shouldn't be any chance of a deadlock.

Comment on attachment 565338 [details] [diff] [review] de-bitrotted, with all the files Review of attachment 565338 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good, but when run tests, a lot of the imap ones are failing with: ###!!! ASSERTION: couldn't get proxies: 'NS_SUCCEEDED(res)', file /Users/moztest/comm/main/src/mailnews/imap/src/nsImapProtocol.cpp, line 642 nsImapProtocol::SetupSinkProxy()+0x000004B8 [/Users/moztest/comm/main/tb/mozilla/dist/bin/XUL +0x01AF4136] nsImapProtocol::LoadImapUrl(nsIURI*, nsISupports*)+0x000000CC [/Users/moztest/comm/main/tb/mozilla/dist/bin/XUL +0x01AF64EA] ::: mailnews/imap/src/nsImapProtocol.h @@ +404,5 @@ > > + nsRefPtr<ImapMailFolderSinkProxy> m_imapMailFolderSink; > + nsRefPtr<ImapMessageSinkProxy> m_imapMessageSink; > + nsRefPtr<ImapServerSinkProxy> m_imapServerSink; > + nsRefPtr<ImapProtocolSinkProxy> m_imapProtocolSink; nit: might as well make this align with the rest above it.

fixed on trunk - http://hg.mozilla.org/comm-central/rev/9205412330a6

Re comment 50, I think we're already using the same mechanism for dispatching events to the UI thread, and waiting on a monitor, so that comment is moot.

bsmedberg pointed out that we missed one in ldap - http://hg.mozilla.org/comm-central/rev/5f64160772c8