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

VERIFIED FIXED

Status

defect
--
critical
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: ajschult784, Assigned: mscott)

Tracking

({crash, regression})

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
Posted file 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

12 years ago
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?
(Reporter)

Comment 2

12 years ago
> 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?
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.
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.
(Assignee)

Comment 5

12 years ago
Hmm, I'll take a look at this.
(Assignee)

Comment 6

12 years ago
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.
(Assignee)

Comment 7

12 years ago
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.
Assignee: bienvenu → mscott
Status: NEW → ASSIGNED

Comment 8

12 years ago
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.
(Assignee)

Comment 9

12 years ago
Posted patch camino changeSplinter Review
(Assignee)

Comment 10

12 years ago
(Assignee)

Comment 11

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

It's not shown here, but I also changed the interface ID.
Attachment #272847 - Flags: superreview?(bienvenu)

Updated

12 years ago
Attachment #272847 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 12

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

Mark, what do you think of this approach?
Attachment #272847 - Flags: review?(bugzilla)
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.
Attachment #272847 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 14

12 years ago
sorry for not including the mailnews/db changes in the earlier patch.
Attachment #272847 - Attachment is obsolete: true
Attachment #273001 - Flags: review?(bugzilla)
(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
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:stephend@mozilla.com" and click OK
4. Type "test" in the subject field
5. Click Send

We then crash...
OS: Linux → All
(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 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.
Attachment #273001 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 19

12 years ago
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.
Attachment #272865 - Flags: superreview?(sspitzer)
Comment on attachment 272865 [details] [diff] [review]
toolkit changes

r=sspitzer for the toolkit changes
Attachment #272865 - Flags: superreview?(sspitzer) → review+
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Product: Core → MailNews Core
Crash Signature: [@ nsMorkFactoryFactory::GetMdbFactory] [@ ns_if_addref<nsIMdbFactory*>]
You need to log in before you can comment on or make changes to this bug.