Closed Bug 675407 Opened 13 years ago Closed 13 years ago

Remove XPCOM proxies from comm-central

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 10.0

People

(Reporter: jcranmer, Assigned: Bienvenu)

References

Details

Attachments

(7 files, 10 obsolete files)

26.24 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
83.10 KB, patch
standard8
: review+
Details | Diff | Splinter Review
16.31 KB, patch
standard8
: review+
Details | Diff | Splinter Review
45.05 KB, patch
Details | Diff | Splinter Review
2.33 KB, patch
jcranmer
: review+
Details | Diff | Splinter Review
3.96 KB, patch
jcranmer
: review+
Details | Diff | Splinter Review
89.89 KB, patch
standard8
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → dbienvenu
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.
Attached patch remove ldap proxy stuff (obsolete) — Splinter Review
this remove the ldap use of xpcom proxies, in ldap/xpcom, and mailnews/addrbook.
Attachment #552001 - Flags: review?(neil)
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.
Attachment #552001 - Flags: review?(neil) → review+
carrying forward Neil's r+. This patch addresses his comments about the constructor initializers, and removes some more unneeded header files.
Attachment #552001 - Attachment is obsolete: true
Attachment #552506 - Flags: review+
Attachment #552506 - Attachment description: ldap proxy removal v2 → ldap proxy removal v2 - will land after aurora branches
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
Attachment #554999 - Flags: review?(mbanner)
(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.
Attachment #553788 - Flags: review?(mbanner)
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.
Attached patch just the imap changes, wip (obsolete) — Splinter Review
Attachment #556725 - Attachment is obsolete: true
this finishes imap, I believe. There are still a few proxy calls in the main import code that I need to fix.
Attachment #556739 - Attachment is obsolete: true
There's still a bit of cleanup to do for imap, mostly removing unused macros.
Status: NEW → ASSIGNED
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.
Attachment #552506 - Attachment description: ldap proxy removal v2 - will land after aurora branches → ldap proxy removal v2 - will land after aurora branches - checked in
Attached patch cumulative patch (obsolete) — Splinter Review
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.
Attachment #554999 - Flags: review?(mbanner) → review+
Attachment #557606 - Flags: review?(Pidgeot18)
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 ;-) .
Attachment #557606 - Flags: review?(Pidgeot18) → review+
Attachment #554999 - Attachment description: get rid of proxies used in address books, including import → get rid of proxies used in address books, including import - checked in
Attached patch cumulative outstanding diffs (obsolete) — Splinter Review
this is what I have left in my repo, after landing a couple patches, and finding a couple more things I had to fix.
Attachment #557387 - Attachment is obsolete: true
Attachment #557698 - Flags: review?(Pidgeot18)
Attachment #557698 - Flags: review?(Pidgeot18) → review+
Attachment #553788 - Flags: review?(mbanner) → review+
import changes pushed - http://hg.mozilla.org/comm-central/rev/f7fd0a2c188a
Attachment #553788 - Attachment description: remove more proxies from import, including send → remove more proxies from import, including send - checked in.
Attached patch remaining fixes (obsolete) — Splinter Review
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.
Attachment #557625 - Attachment is obsolete: true
Attachment #559667 - Flags: review?(mbanner)
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.
Attached patch fix crash in eudora import (obsolete) — Splinter Review
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...
Attachment #561045 - Flags: review?(mbanner)
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.
Attachment #561045 - Flags: review?(mbanner)
Attachment #561045 - Attachment is obsolete: true
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?
Attachment #559667 - Flags: review?(mbanner) → review-
Attached patch two new files (obsolete) — Splinter Review
I'll try to roll all these into a patch that applies and builds
Attached patch de-bitrotted, with all the files (obsolete) — Splinter Review
I also fixed some PRbool stuff in import
Attachment #559667 - Attachment is obsolete: true
Attachment #565283 - Attachment is obsolete: true
Attachment #565338 - Flags: review?(mbanner)
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.
Attachment #565338 - Flags: review?(mbanner) → review-
Attachment #565338 - Attachment is obsolete: true
Attachment #567311 - Flags: review?(mbanner)
Attachment #567311 - Flags: review?(mbanner) → review+
fixed on trunk - http://hg.mozilla.org/comm-central/rev/9205412330a6
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0
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
Depends on: 723888
Blocks: 142123
No longer depends on: 723888
Regressions: 723888
You need to log in before you can comment on or make changes to this bug.