Closed
Bug 876548
Opened 11 years ago
Closed 10 years ago
Segmentation fault (crash) when writing email with no account created nsMsgDBService::GetOpenDBs
Categories
(MailNews Core :: Database, defect)
MailNews Core
Database
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 30.0
People
(Reporter: wsmwk, Assigned: neil)
References
Details
(Keywords: crash, Whiteboard: [rare])
Crash Data
Attachments
(1 file, 2 obsolete files)
36.07 KB,
patch
|
Irving
:
review+
|
Details | Diff | Splinter Review |
+++ 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
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?
Flags: needinfo?(mozilla)
Comment 2•11 years ago
|
||
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.
Flags: needinfo?(mozilla)
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?
Flags: needinfo?(mozilla)
Comment 4•11 years ago
|
||
(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).
Flags: needinfo?(mozilla)
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.
Attachment #772181 -
Flags: feedback?(mozilla)
Updated•10 years ago
|
Attachment #772181 -
Flags: feedback?(irving)
Assignee | ||
Comment 7•10 years ago
|
||
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.
Attachment #8374712 -
Flags: feedback?(irving)
Comment 8•10 years ago
|
||
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?
Attachment #8374712 -
Flags: feedback?(irving) → feedback+
Comment 9•10 years ago
|
||
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.
Attachment #772181 -
Flags: feedback?(irving) → feedback-
Comment 10•10 years ago
|
||
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].
Attachment #772181 -
Attachment is obsolete: true
Attachment #772181 -
Flags: feedback?(mozilla)
Assignee: nobody → neil
Status: NEW → ASSIGNED
Component: Account Manager → Database
Product: Thunderbird → MailNews Core
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Assignee | ||
Comment 12•10 years ago
|
||
(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...
Assignee | ||
Comment 13•10 years ago
|
||
OK, this is about the best I can do without using globals.
Attachment #8374712 -
Attachment is obsolete: true
Attachment #8376421 -
Flags: review?(irving)
Comment 14•10 years ago
|
||
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.
Attachment #8376421 -
Flags: review?(irving) → review+
Assignee | ||
Comment 15•10 years ago
|
||
Pushed comm-central changeset 9cd7b9a9a86f.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → Thunderbird 30.0
Comment 16•10 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•