388532 - Crash trying to type address for composition [@ nsMorkFactoryFactory::GetMdbFactory] [@ ns_if_addref<nsIMdbFactory*>]

Status: VERIFIED FIXED

(MailNews Core :: Database, defect)

Description

[Andrew Schultz]

Attachment: stacktrace

With current trunk SeaMonkey, if I start typing in an address in the compose window, SeaMonkey crashes.  This started between 2007-07-14-11-trunk and 2007-07-15-11-trunk (bug 388101).  Stacktrace is attached.

Comment 1

[David :Bienvenu]

this wfm in TB - are the steps to reproduce simply:

open SM. Open a compose window, type a few chars in the address widget, crash?

Is it searching against a local AB? multiple local AB's?

Comment 2

[Andrew Schultz]

> Open a compose window, type a few chars in the address widget, crash?

Yes.

> Is it searching against a local AB? multiple local AB's?

Hmm, not sure.  I just created a clean profile, created an email acconut and tried to compose an email.  It didn't crash.  I quit seamonkey and re-opened with that same profile and it crashed.  So, probably not multiple local AB's.  Are there printfs I could add or stack frames I could poke with gdb to see more what it's trying to do?

Comment 3

[Mark Banner (:standard8)]

I'm not sure the code is right:

nsAddrDatabase::GetMDBFactory and nsMsgDatabase::GetMDBFactory are both now using a nsCOMPtr for the factor.

However, nsMorkFactoryFactory::GetMdbFactory still uses a static instance of the factory pointer.

So nsAddrDatabase (or nsMsgDatabase) will use the factory, then when they get destroyed, they will release it as no one will be holding a pointer to it because the nsMorkFactoryFactory version isn't retaining it.

I'm guessing that it may depend on the particular setup/instances as to whether or not it crashes.

Comment 4

[Mark Banner (:standard8)]

I didn't complete the paragraph...

(In reply to comment #3)
> So nsAddrDatabase (or nsMsgDatabase) will use the factory, then when they get
> destroyed, they will release it as no one will be holding a pointer to it
> because the nsMorkFactoryFactory version isn't retaining it.

So when nsAddrDatabase comes to use it again, it will be given the old pointer by nsMorkFactoryFactory, and obviously in some cases that is causing a crash.

Comment 5

[Scott MacGregor]

Hmm, I'll take a look at this.

Comment 6

[Scott MacGregor]

Yes this isn't quite right.

You know, I almost wonder if a better approach would be to make nsIMdbFactoryFactory a service, forcing callers to use do_GetService instead of createInstance. The whole purpose of the class is to create a singleton nsIMdbFactory object and pass that back to everyone. Or maybe even cut the factoryfactory out of the equation altogether and have consumers directly access the nsIMdbFactory as a service.

Comment 7

[Scott MacGregor]

Attachment: WIP - make nsIMdbFactoryFactory a service

What do folks think of this solution.

Make nsIMdbFactoryFactory a service. The service holds an instance of the mdb factory.

Callers get the factory from the service. The callers must also hold onto the mdb factory as I learned the hard way, the mdbfactory can't go away until the database is closed.

If we're happy with this approach I can patch camino and firefox which also use nsIMdbFactoryFactory.

Comment 8

[David :Bienvenu]

I think that's OK - originally DavidMc wanted to use xpcom as little as possible in Mork, but I think it'll help get around this problem.

Comment 9

[Scott MacGregor]

Attachment: camino change

Comment 10

[Scott MacGregor]

Attachment: toolkit changes

Comment 11

[Scott MacGregor]

Comment on attachment 272847 [details] [diff] [review]
WIP - make nsIMdbFactoryFactory a service

It's not shown here, but I also changed the interface ID.

Comment 12

[Scott MacGregor]

Comment on attachment 272847 [details] [diff] [review]
WIP - make nsIMdbFactoryFactory a service

Mark, what do you think of this approach?

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 272847 [details] [diff] [review]
WIP - make nsIMdbFactoryFactory a service

Scott, you're missing the changes for nsMsgDatabase.cpp. I tried applying what I thought was correct by hand:

Index: mailnews/db/msgdb/src/nsMsgDatabase.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp,v
retrieving revision 1.384
diff -u -p -r1.384 nsMsgDatabase.cpp
--- mailnews/db/msgdb/src/nsMsgDatabase.cpp     14 Jul 2007 21:57:38 -0000      1.384
+++ mailnews/db/msgdb/src/nsMsgDatabase.cpp     19 Jul 2007 18:29:54 -0000
@@ -974,7 +974,7 @@ nsIMdbFactory *nsMsgDatabase::GetMDBFact
 {
   if (!mMdbFactory)
   {
-    nsCOMPtr <nsIMdbFactoryFactory> factoryfactory = do_CreateInstance(NS_MORK_CONTRACTID);
+    nsCOMPtr <nsIMdbFactoryService> factoryfactory = do_GetService(NS_MORK_CONTRACTID);
     if (factoryfactory)
       factoryfactory->GetMdbFactory(getter_AddRefs(mMdbFactory));
   }

and when I tried loading mailnews (in seamonkey) it loaded ok, but when I tried to change my current pop3 folder I got:

failed to get view & sort values.  ex = [Exception... "Component returned failure code: 0x80550006 [nsIMsgFolder.getMsgDatabase]"  nsresult: "0x80550006 (<unknown>)"  location: "JS frame :: chrome://messenger/content/commandglue.js :: FolderPaneSelectionChange :: line 862"  data: no]
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file /mozilla/master/mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 153
Error loading with many headers to download: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMsgFolder.updateFolder]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://messenger/content/commandglue.js :: ChangeFolderByURI :: line 255"  data: no]

on the console. So either my change wasn't right or I'm missing something else.

The address book seemed to run ok though, and the general ideas also seem ok.

Comment 14

[Scott MacGregor]

Attachment: include my database changes this time...

sorry for not including the mailnews/db changes in the earlier patch.

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #14)
> sorry for not including the mailnews/db changes in the earlier patch. 

np. The patch is now better and appears to work, but I'm still getting various errors on the console that I wouldn't expect and may be related (see below for a variety). I haven't got time to rebuild without the patch tonight, so I don't know if they are existing problems or not, but we probably still need to fix them anyway.

failed to get view & sort values.  ex = [Exception... "Component returned failure code: 0x80550006 [nsIMsgFolder.getMsgDatabase]"  nsresult: "0x80550006 (<unknown>)"  location: "JS frame :: chrome://messenger/content/commandglue.js :: FolderPaneSelectionChange :: line 862"  data: no]
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file /mozilla/master/mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 153
Error loading with many headers to download: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMsgFolder.updateFolder]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://messenger/content/commandglue.js :: ChangeFolderByURI :: line 255"  data: no]
failed to get view & sort values.  ex = [Exception... "Component returned failure code: 0x80550006 [nsIMsgFolder.getMsgDatabase]"  nsresult: "0x80550006 (<unknown>)"  location: "JS frame :: chrome://messenger/content/commandglue.js :: FolderPaneSelectionChange :: line 862"  data: no]
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file /mozilla/master/mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 153
Error loading with many headers to download: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMsgFolder.updateFolder]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://messenger/content/commandglue.js :: ChangeFolderByURI :: line 255"  data: no]
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file /mozilla/master/mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 153
###!!! ASSERTION: bad size recorded: 'aInstanceSize == 0 || entry->GetClassSize() == aInstanceSize', file /mozilla/master/mozilla/xpcom/base/nsTraceRefcntImpl.cpp, line 433
++DOMWINDOW == 9
###!!! ASSERTION: uid validity seems to have changed, blowing away db: 'PR_FALSE', file /mozilla/master/mozilla/mailnews/imap/src/nsImapMailFolder.cpp, line 2446
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file /mozilla/master/mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 153

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

An easy way for me to reproduce on my Windows XP VM instance is:

1. Create an IMAP account (I used my @iusb.edu one)
2. Set Thunderbird as the default email client
3. From Internet Explorer or the Run field in the Start menu, type: "mailto:[e-mail]" and click OK
4. Type "test" in the subject field
5. Click Send

We then crash...

Comment 17

[Mark Banner (:standard8)]

(In reply to comment #15)
> (In reply to comment #14)
> > sorry for not including the mailnews/db changes in the earlier patch. 
> 
> np. The patch is now better and appears to work, but I'm still getting various
> errors on the console that I wouldn't expect and may be related (see below for
> a variety). I haven't got time to rebuild without the patch tonight, so I don't
> know if they are existing problems or not, but we probably still need to fix
> them anyway.
> 

I've just retested this, firstly without the patch and then with the patch. It appears that the earlier version I had tried had screwed up the msf files, and lead to the assertions I mentioned in comment 15. So I 'reset' my mail folders to a good state without your patch, then applied it again and it all worked fine.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 273001 [details] [diff] [review]
include my database changes this time...

A couple of nits:
+void nsMsgFolderCache::GetMDBFactory(nsIMdbFactory ** aMdbFactory)
...
+  NS_IF_ADDREF(*aMdbFactory = mMdbFactory);
+  return;
 }

You no longer need the return there (its like that in a couple of places)

r=me as long as you do a patch for xpfe/components/history as well.

Comment 19

[Scott MacGregor]

Comment on attachment 272865 [details] [diff] [review]
toolkit changes

asking Seth to review the toolkit changes.

Seth, we're making nsIMdbFactoryFactory an actual service in order to fix a small memory leak.

Comment 20

[(not reading, please use [e-mail] instead)]

Comment on attachment 272865 [details] [diff] [review]
toolkit changes

r=sspitzer for the toolkit changes

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using version 3.0a1pre (2007073104) of Thunderbird on Windows Vista; I was running into this bug all the time, and now have no problems.