876548 - Segmentation fault (crash) when writing email with no account created nsMsgDBService::GetOpenDBs

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Wayne Mery (:wsmwk)]

+++ This bug was initially created as a clone of Bug #750080 +++

still are crashes, but rare 1-2 crashes per month.  unknown if steps of bug 750080 comment 0 cause this.

 bp-e6c7ffac-0606-4d6f-a25a-072592130325 (thiago)
0	xul.dll	nsMsgDBService::GetOpenDBs	mailnews/db/msgdb/src/nsMsgDatabase.cpp:431
1	xul.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70
2	xul.dll	XPCWrappedNative::CallMethod	js/xpconnect/src/XPCWrappedNative.cpp:2367
3	xul.dll	XPC_WN_GetterSetter	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1526
4	mozjs.dll	js::InvokeKernel	js/src/jsinterp.cpp:352
5	mozjs.dll	js::Invoke	js/src/jsinterp.cpp:396
6	mozjs.dll	js::InvokeGetterOrSetter	js/src/jsinterp.cpp:469
7	mozjs.dll	js::Shape::get	js/src/jsscopeinlines.h:296
8	mozjs.dll	js_NativeGet	js/src/jsobj.cpp:4217
9	mozjs.dll	js::NativeGet	js/src/jsinterpinlines.h:175
10	mozjs.dll	js::GetPropertyOperation	js/src/jsinterpinlines.h:259

Comment 1

[:aceman]

While I do not yet see a reason for the crash, does m_dbCache need to be a pointer allocated with 'new' and we must not forget to initialize it (as in bug 750080)? Can't it be nsAutoTArray<nsMsgDatabase*, kInitialMsgDBCacheSize> from the start on?

Comment 2

[David :Bienvenu]

It wouldn't be nsAutoTArray; that's for stack based arrays. The reason it's not a static global nsTArray is that it looks like a memory leak, and iirc, in general, we try not to have static globals of things like nsTArrays or nsStrings.

Comment 3

[:aceman]

Thanks, so then at http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/public/nsMsgDatabase.h#243 this only creates a static pointer to a nsTArray... So that there is only one instance of the cache. Would it be worth to split the cache management functions and into a separate DBCache class which will only have one instance (e.g. a global variable in mailnews/db/msgdb/public/nsMsgDatabase.cpp) and it will have the nsTArray as a normal (non-pointer) member? Is that harder than I imagine?

Comment 4

[David :Bienvenu]

(In reply to :aceman from comment #3)
That would still result in a global nsTArray, just wrapped in a container class.

You could add a custom Init method to the nsMsgDBService that intializes the db cache. You would need to change http://mxr.mozilla.org/comm-central/source/mailnews/build/nsMailModule.cpp to make nsMsgDBService have an Init() method and then add that method to nsMsgDBService (see http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgAccountManager.cpp#153 for an example).

Comment 5

[:aceman]

Attachment: WIP patch

Thanks. Is this what you meant? This is not finished yet, as for some reason while running tests the nsMsgDatabase::CleanupCache is hit with a null m_dbCache.

Comment 6

[Wayne Mery (:wsmwk)]

bp-ddfe7165-3fbd-4205-9396-a65d02131117 24.1.0

Comment 7

[[e-mail]]

Attachment: Possible patch

Last time I looked at this array I thought it sucked, so I wrote this patch, but I never got around to attaching it to a bug, but as it happens, it looks like it would fix this bug, so I might as well attach it before it gets lost.

Comment 8

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8374712 [details] [diff] [review]
Possible patch

Review of attachment 8374712 [details] [diff] [review]:
-----------------------------------------------------------------

Over all I prefer the approach in this patch of moving the cache from a global to an element in nsMsgDBService, particularly if we can move a few more functions over and get rid of the global pointer.

That said, is this going to cause too many API changes to take as a 24.x patch? Is this crash important enough that we want to promote a fix?

::: mailnews/db/msgdb/public/nsMsgDatabase.h
@@ +37,5 @@
>  
> +// Hopefully we're not opening up lots of databases at the same time, however
> +// this will give us a buffer before we need to start reallocating the cache
> +// array.
> +const uint32_t kInitialMsgDBCacheSize = 20;

I don't think reallocs of this particular array will have a noticeable effect on performance.

@@ +55,4 @@
>  
>    nsCOMArray <nsIMsgFolder> m_foldersPendingListeners;
>    nsCOMArray <nsIDBChangeListener> m_pendingListeners;
> +  nsAutoTArray<nsMsgDatabase*, kInitialMsgDBCacheSize> m_dbCache;

Do we want nsCOMArray<nsMsgDatabase> here to reference count the database objects?

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +69,5 @@
>  static const nsMsgKey kIdStartOfFake = 0xffffff80;
>  
>  static PRLogModuleInfo* DBLog;
>  
> +nsTArray<nsMsgDatabase*>* g_dbCache = nullptr;

Can we expose an accessor on nsMsgDBService for the cache, rather than defining a global?

@@ +165,5 @@
>    NS_ENSURE_ARG(aFolder);
> +
> +  nsCOMPtr<nsIMsgPluggableStore> msgStore;
> +  nsresult rv = aFolder->GetMsgStore(getter_AddRefs(msgStore));
> +  NS_ENSURE_SUCCESS(rv, rv);

NS_ENSURE_SUCCESS is deprecated

@@ +937,2 @@
>  void
>  nsMsgDatabase::CleanupCache()

If you move this into nsMsgDBService, you don't need g_dbCache

@@ +960,5 @@
>    dbName->GetNativePath(dbPath);
>    return dbPath.Equals(m_dbName);
>  }
>  
> +void nsMsgDatabase::AddToCache(nsMsgDatabase* pMessageDB)

Move to nsMsgDBService?

@@ -976,5 @@
>  
> -//----------------------------------------------------------------------
> -// RemoveFromCache
> -//----------------------------------------------------------------------
> -void nsMsgDatabase::RemoveFromCache(nsMsgDatabase* pMessageDB)

move to nsMsgDBService?

@@ +981,5 @@
>  
>  /**
>   * Log the open db's, and how many headers are in memory.
>   */
>  void nsMsgDatabase::DumpCache()

Move to nsMsgDBService?

Comment 9

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 772181 [details] [diff] [review]
WIP patch

Review of attachment 772181 [details] [diff] [review]:
-----------------------------------------------------------------

I'd rather go with Neil's approach unless we need to minimize API changes for a 24.x patch.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +904,5 @@
> +nsresult nsMsgDatabase::InitDBCache()
> +{
> +  if (!m_dbCache)
> +  {
> +    m_dbCache = new nsAutoTArray<nsMsgDatabase*, kInitialMsgDBCacheSize>;

These are XPCOM objects, so I think we want nsCOMArray<nsMsgDatabase>  rather than nsAutoTArray<...> here; the Auto version is meant for use on the stack, and doesn't manage pointers.

Comment 10

[:aceman]

Comment on attachment 772181 [details] [diff] [review]
WIP patch

No problem with that. We probably do not need a 24.x variant as the bug is marked [rare].

Comment 11

[[e-mail]]

(In reply to Irving Reid from comment #8)
> Over all I prefer the approach in this patch of moving the cache from a
> global to an element in nsMsgDBService, particularly if we can move a few
> more functions over and get rid of the global pointer.

Yeah, I wasn't sure how far to go with this for a first pass.

> > +// Hopefully we're not opening up lots of databases at the same time, however
> > +// this will give us a buffer before we need to start reallocating the cache
> > +// array.
> > +const uint32_t kInitialMsgDBCacheSize = 20;
> 
> I don't think reallocs of this particular array will have a noticeable
> effect on performance.

This line was moved from the .cpp to the .h file, so you'll need to specify any change you want.

> > +  nsAutoTArray<nsMsgDatabase*, kInitialMsgDBCacheSize> m_dbCache;
> 
> Do we want nsCOMArray<nsMsgDatabase> here to reference count the database
> objects?

We don't remove databases from the cache until they have no more references. This has two side effects:
1. Someone else is always referencing the databases in the cache
2. If the cache had its own reference it would prevent the removal

> > +nsTArray<nsMsgDatabase*>* g_dbCache = nullptr;
> 
> Can we expose an accessor on nsMsgDBService for the cache, rather than
> defining a global?

That would mean a global nsMsgDBService pointer, or ugly static casts. Moving more functions over might be OK though.

> > +  NS_ENSURE_SUCCESS(rv, rv);
> 
> NS_ENSURE_SUCCESS is deprecated

Again, this is just code motion.

> >  void
> >  nsMsgDatabase::CleanupCache()
> 
> If you move this into nsMsgDBService, you don't need g_dbCache

Sure, but I wasn't ready to touch code outside of mailnews/db/msgdb yet.

> > +void nsMsgDatabase::AddToCache(nsMsgDatabase* pMessageDB)
> > -void nsMsgDatabase::RemoveFromCache(nsMsgDatabase* pMessageDB)
> >  void nsMsgDatabase::DumpCache()
> 
> Move to nsMsgDBService?

Awkward but doable.

Comment 12

[[e-mail]]

(In reply to Irving Reid from comment #8)
> >  void
> >  nsMsgDatabase::CleanupCache()
> 
> If you move this into nsMsgDBService, you don't need g_dbCache

Hmm, we currently call this statically from the module destructor... I guess that doing it in the service's destructor would work, assuming the service isn't leaked...

Comment 13

[[e-mail]]

Attachment: Proposed patch

OK, this is about the best I can do without using globals.

Comment 14

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8376421 [details] [diff] [review]
Proposed patch

Review of attachment 8376421 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/db/msgdb/public/nsMsgDatabase.h
@@ +152,5 @@
>     *                        The database is present (and was opened), but the
>     *                        summary file is missing.
>     */
> +  virtual nsresult Open(nsMsgDBService *aDBService, nsIFile *aFolderName,
> +                        bool aCreate, bool aLeaveInvalidDB);

+1 to passing a reference instead of using a global (or GetDBService()) when it's easy to do.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +1142,5 @@
>    PR_LOG(DBLog, PR_LOG_ALWAYS, ("closing database    %s\n",
>      (const char*)m_dbName.get()));
>  
> +  nsCOMPtr<nsIMsgDBService> serv(do_GetService(NS_MSGDB_SERVICE_CONTRACTID));
> +  static_cast<nsMsgDBService*>(serv.get())->RemoveFromCache(this);

Here we can use services::GetDBService()

@@ +4071,5 @@
>    //  not have been added to the cache. Add it now if missing.
>    if (valid)
>    {
> +    nsCOMPtr<nsIMsgDBService> serv(do_GetService(NS_MSGDB_SERVICE_CONTRACTID));
> +    static_cast<nsMsgDBService*>(serv.get())->EnsureCached(this);

services::GetDBService()

::: mailnews/db/msgdb/test/unit/test_enumerator_cleanup.js
@@ +13,5 @@
>    let db = localAccountUtils.inboxFolder.msgDatabase;
>    let enumerator = db.EnumerateMessages();
> +  Cc["@mozilla.org/msgDatabase/msgDBService;1"]
> +    .getService(Ci.nsIMsgDBService)
> +    .forceFolderDBClosed(localAccountUtils.inboxFolder);

Should probably add msgDBService to mailServices.js, but that doesn't have to happen in this patch.

Comment 15

[[e-mail]]

Pushed comm-central changeset 9cd7b9a9a86f.

Comment 16

[Mark Banner (:standard8)]

Looks like this broke xpcshell-tests on trunk:

TEST-INFO | (xpcshell/head.js) | 0 check(s) todo
<<<<<<<
mozcrash INFO | Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tinderbox-builds/comm-central-win32/1392944667/thunderbird-30.0a1.en-US.win32.crashreporter-symbols.zip
PROCESS-CRASH | C:\slave\test\build\xpcshell\tests\mail\base\test\unit\test_viewWrapper_logic.js | application crashed [@ mozilla::services::`anonymous namespace'::ShutdownObserver::EnsureInitialized()]
Crash dump filename: c:\users\cltbld\appdata\local\temp\tmpunlmec\d7a168b1-36a6-41e8-9883-9ec6665ed5b4.dmp
Operating system: Windows NT
                  6.1.7601 Service Pack 1
CPU: x86
     GenuineIntel family 6 model 30 stepping 5
     8 CPUs

Crash reason:  EXCEPTION_ACCESS_VIOLATION_READ
Crash address: 0x0

Thread 0 (crashed)
 0  xul.dll!mozilla::services::`anonymous namespace'::ShutdownObserver::EnsureInitialized() [Services.cpp:01a9de9baf7a : 88 + 0x0]
    eip = 0x6586ada4   esp = 0x001ff83c   ebp = 0x001ff848   ebx = 0x00000009
    esi = 0x00000000   edi = 0x748511dc   eax = 0x001ff844   ecx = 0x00000000
    edx = 0x65a1c698   efl = 0x00010246
    Found by: given as instruction pointer in context
 1  xul.dll!mozilla::services::GetDBService() [ServiceList.h:01a9de9baf7a : 16 + 0x8]
    eip = 0x6586adca   esp = 0x001ff850   ebp = 0x001ff854
    Found by: call frame info
 2  xul.dll!nsMsgDatabase::~nsMsgDatabase() [nsMsgDatabase.cpp:01a9de9baf7a : 1146 + 0x8]
    eip = 0x658bbdc9   esp = 0x001ff85c   ebp = 0x001ff874
    Found by: call frame info