Closed
Bug 492963
Opened 15 years ago
Closed 15 years ago
crash [@ morkTableRowCursor::NextRow(morkEnv*, mdbOid*, int*)]
Categories
(MailNews Core :: Database, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b3
People
(Reporter: wsmwk, Assigned: Bienvenu)
References
Details
(Keywords: crash, topcrash)
Crash Data
Attachments
(1 file, 3 obsolete files)
12.60 KB,
patch
|
standard8
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
crash [@ morkTableRowCursor::NextRow] #14 crasher for 3.0b2 and also seen in 3.0b3pre typical is bp-a3d7a092-a878-4e0f-9068-c7c4c2090513 Frame Module Signature [Expand] Source 0 thunderbird.exe morkTableRowCursor::NextRow db/mork/src/morkTableRowCursor.cpp:508 1 thunderbird.exe morkTableRowCursor::NextRow db/mork/src/morkTableRowCursor.cpp:289 2 thunderbird.exe nsMsgDBEnumerator::PrefetchNext mailnews/db/msgdb/src/nsMsgDatabase.cpp:2588 3 thunderbird.exe nsMsgDBEnumerator::HasMoreElements mailnews/db/msgdb/src/nsMsgDatabase.cpp:2634 4 xpcom_core.dll NS_InvokeByIndex_P xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101 5 xpcom_core.dll xptiInterfaceInfo::GetMethodInfo xpcom/reflect/xptinfo/src/xptiprivate.h:706 6 js3250.dll js_Interpret js/src/jsinterp.cpp:4411 another example bp-ad7c0136-a199-4a50-a56d-5826d2090510 0 eudora.exe morkTableRowCursor::NextRow db/mork/src/morkTableRowCursor.cpp:508 1 eudora.exe morkTableRowCursor::NextRow db/mork/src/morkTableRowCursor.cpp:289 2 eudora.exe nsMsgDBEnumerator::PrefetchNext nsMsgDatabase.cpp:2603 3 eudora.exe nsMsgDBEnumerator::HasMoreElements nsMsgDatabase.cpp:2649 4 xpcom_core.dll NS_InvokeByIndex_P xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101 5 xpcom_core.dll xptiInterfaceInfo::GetMethodInfo xpcom/reflect/xptinfo/src/xptiprivate.h:706 6 js3250.dll js_obj_defineSetter js/src/jsobj.cpp:1752 7 js3250.dll js_Interpret js/src/jsinterp.cpp:4983 8 @0x24f18f
Flags: blocking-thunderbird3?
Comment 1•15 years ago
|
||
bp-dd5e9671-9c2d-4ddd-9d62-4dd1c2090511 has a better (full) backtrace It looks like gloda, bug 483329 in particular. Although gloda is in the wrong, I'd suggest we keep this bug open and try and make it so that people holding on to enumerators can't crash us.
Reporter | ||
Comment 2•15 years ago
|
||
This needs to block +. In fact I would advocate this needs to block 3.0b3. It will effectively become the #1 or 2 crasher for 3.0b3 after bug 480090 gets fixed (perhaps even if it doesn't). 7 crashes per day for 3.0b2 - but of course not everyone has gloda on - so the numbers will get far, far worse with gloda prefed on. (sorry for this sneaking up on us - I haven't kept tight tabs on crash stats since 3.0b3 dates slipped) Based on crash comments this can't all be blamed on shutdown, but setting bug 483329 as blocker. (comment 1) 3.0b2 crash comments: * ジャンクメールを削除したのちコンパクト化をおこなっていたところ突然クラッシュ! [1] * * Upon deletion of account thunderbird crashes * Trying to empty trash * Tried to compact my inbox. [3] * POP3 Initial download * It was basically idle -- apparently it went to retrieve email!??? don't really know -- it was minimized and I wasn't currently using it. * I was using the Archive feature on a large number of emails at once. I also had the Activity Monitor open and as soon as I closed it, Thunderbird crashed. * Had recently installed Lightning * Delete IMAP account, reproducible. [2] [1] bp-3b6ee656-9242-4b3a-b5d4-f79d72090520 [2] bp-23e8f63d-e47b-4db6-8df4-de5fd2090515 [3] bp-5d16357d-752d-4625-b9fb-1c6952090520 (example of longer stack) 3.0b3pre crash comments: * Opened IMAP folder. Found a single message that had all of the others one after another. Asked the folder to rebuild index and crashed. bp-f60a7288-2388-4833-9abd-387012090507 * I tried to close Shredder. FAIL. bp-cb33aa71-be30-4c45-bcd9-1eb842090519 * I deleted a mail folder while shredder indexed it. bp-6c2e1ca1-2b32-49de-8fd9-2ed062090526 * Deleting a message bp-b6e2e2a1-4559-4944-9cc9-4a9c02090518 (I haven't looked at all the stacks of the crashes cited to see if they all match up.)
Depends on: 483329
Comment 3•15 years ago
|
||
(In reply to comment #2) > Based on crash comments this can't all be blamed on shutdown, but setting bug > 483329 as blocker. (comment 1) I vaguely recall hitting this crash a while back - and I'm sure it wasn't shutdown-related.
Reporter | ||
Comment 4•15 years ago
|
||
i'll leave to others whether this should block bug 464354 - but I suspect it should
Assignee | ||
Comment 5•15 years ago
|
||
OK, I can investigate this. I've never seen this particular crash myself, however. The enumerator holds a reference to the db, so in theory the only common ways for the db to get closed out from under the enumerator are folder compact, rebuild index, shutdown, or folder deletion. Hmm, that's rather a lot. So I can make the db keep track of the enumerators it has handed out, and if the db is closed, I can disable the enumerator...
Assignee: nobody → bienvenu
Assignee | ||
Updated•15 years ago
|
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Assignee | ||
Updated•15 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•15 years ago
|
||
This makes the nsMsgDatabase keep track of the enumerators it has handed out (which should be very few), and clean them up if the db goes away before the enumerator.
Assignee | ||
Comment 7•15 years ago
|
||
Standard8, I'll come up with a separate test case for this, but it seems to me that this should fix bug 496557, and it would be interesting to know if it does.
Assignee | ||
Comment 8•15 years ago
|
||
one of the gloda tests seems unhappy with this patch, natch. I'll investigate.
Assignee | ||
Comment 9•15 years ago
|
||
I think there are cases where we're calling ClearCachedObjects() prior to the nsMsgDatabase destructor, and then clearing the enumerator releases the last ref to the db, so we re-entrently call ClearCachedObjects. So one way to fix that is to copy the array of enumerators to a local and clear the member variable...
Assignee | ||
Comment 10•15 years ago
|
||
This makes the db keep track of the enumerators it has handed out, and cleans them up when the db goes away. It also adds a unit test that fails w/o the patch, and succeeds with it...
Attachment #381881 -
Attachment is obsolete: true
Attachment #381984 -
Flags: superreview?(neil)
Attachment #381984 -
Flags: review?(bugzilla)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [has patch for review, standard8, neil]
Target Milestone: --- → Thunderbird 3.0b3
Comment 11•15 years ago
|
||
Comment on attachment 381984 [details] [diff] [review] proposed fix with unit test >+ nsresult GetRowCursor(); >+ nsresult PrefetchNext(); >+ nsRefPtr<nsMsgDatabase> mDB; >+ nsCOMPtr<nsIMdbTableRowCursor> mRowCursor; >+ nsCOMPtr<nsIMsgDBHdr> mResultHdr; >+ PRBool mDone; >+ PRBool mNextPrefetched; >+ nsMsgDBEnumeratorFilter mFilter; >+ nsCOMPtr <nsIMdbTable> mTable; >+ void* mClosure; Nit: ugly "alignment" >+ // clean out existing enumerators >+ nsTArray<nsMsgDBEnumerator *> copyEnumerators(m_enumerators); >+ m_enumerators.Clear(); Nit: use SwapElements instead. >+ PRInt32 numEnums = copyEnumerators.Length(); >+ for (PRInt32 i = 0; i < numEnums; i++) >+ copyEnumerators[i]->Clear(); Nit: Length() is a PRUint32 >+ if (!mNextPrefetched && (NS_FAILED(PrefetchNext()))) >+ mDone = PR_TRUE; Worth checking !mDone too?
Attachment #381984 -
Flags: superreview?(neil) → superreview+
Comment 12•15 years ago
|
||
Comment on attachment 381984 [details] [diff] [review] proposed fix with unit test Can you refresh this before I take a look at it? (due to the enumerator changes).
Attachment #381984 -
Flags: review?(bugzilla)
Assignee | ||
Comment 13•15 years ago
|
||
Attachment #381984 -
Attachment is obsolete: true
Attachment #382132 -
Flags: review?(bugzilla)
Comment 14•15 years ago
|
||
(In reply to comment #13) > Created an attachment (id=382132) [details] > un-bitrotted patch With this patch applied, I've seen test_filter.js fail and/or crash repeatedly (with today's latest trunk, this is including the fix for bug 496557).
Assignee | ||
Comment 15•15 years ago
|
||
Comment on attachment 382132 [details] [diff] [review] un-bitrotted patch clearing review request while I figure out the test crash.
Attachment #382132 -
Flags: review?(bugzilla)
Assignee | ||
Comment 16•15 years ago
|
||
The fix for that test failure is to make sure that the thread object releases its metarow before releasing the db pointer, since releasing the db can make all of mork go away. I also noticed that we were leaking a threadRow in some relatively uncommon situations (though one encountered by the test case). All the tests pass for me with this patch.
Attachment #382132 -
Attachment is obsolete: true
Attachment #382201 -
Flags: review?(bugzilla)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [has patch for review, standard8, neil] → [needs review standard8]
Comment 17•15 years ago
|
||
Comment on attachment 382201 [details] [diff] [review] fix problems with test_filter.js >diff --git a/mailnews/db/msgdb/test/unit/test_enumerator_cleanup.js b/mailnews/db/msgdb/test/unit/test_enumerator_cleanup.js >+function test_enumerator_cleanup() { >+ let db = gLocalInboxFolder.msgDatabase; >+ let enumerator = db.EnumerateMessages(); >+ db.forceFolderDBClosed(gLocalInboxFolder); >+ gLocalInboxFolder.msgDatabase = null; >+ db = null; >+ hdr = null; nit: hdr isn't used or defined. >+var messageHeaderGetterListener = { >+ msgKey: null, >+ >+ OnStartCopy: function() {}, >+ OnProgress: function(aProgress, aProgressMax) {}, >+ GetMessageId: function (aMessageId) {}, >+ SetMessageKey: function(aKey) { >+ this.msgKey = aKey; >+ }, nit: msgKey isn't required either. r=Standard8 with those fixed.
Attachment #382201 -
Flags: review?(bugzilla) → review+
Assignee | ||
Comment 18•15 years ago
|
||
fix checked in, with nits addressed...
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review standard8]
Assignee | ||
Comment 19•15 years ago
|
||
Comment on attachment 382201 [details] [diff] [review] fix problems with test_filter.js carrying forward sr=neil...
Attachment #382201 -
Flags: superreview+
Reporter | ||
Comment 20•15 years ago
|
||
fix signature for crash-stats
Summary: crash [@ morkTableRowCursor::NextRow] → crash [@ morkTableRowCursor::NextRow(morkEnv*, mdbOid*, int*)]
Updated•13 years ago
|
Crash Signature: [@ morkTableRowCursor::NextRow(morkEnv*, mdbOid*, int*)]
Comment 21•4 years ago
|
||
Just hit what appears to be this bug or possibly a closely related bug: https://crash-stats.mozilla.com/report/index/7ec4e566-6ef1-4967-bdc0-2aa290191201
You need to log in
before you can comment on or make changes to this bug.
Description
•