Closed Bug 492963 Opened 15 years ago Closed 15 years ago

crash [@ morkTableRowCursor::NextRow(morkEnv*, mdbOid*, int*)]

Categories

(MailNews Core :: Database, defect)

x86
All
defect
Not set
critical

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)

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?
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.
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
(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.
i'll leave to others whether this should block bug 464354 - but I suspect it should
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
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Status: NEW → ASSIGNED
Attached patch proposed fix (obsolete) — Splinter Review
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.
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.
one of the gloda tests seems unhappy with this patch, natch. I'll investigate.
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...
Attached patch proposed fix with unit test (obsolete) — Splinter Review
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)
Whiteboard: [has patch for review, standard8, neil]
Target Milestone: --- → Thunderbird 3.0b3
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 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)
Attached patch un-bitrotted patch (obsolete) — Splinter Review
Attachment #381984 - Attachment is obsolete: true
Attachment #382132 - Flags: review?(bugzilla)
(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).
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)
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)
Whiteboard: [has patch for review, standard8, neil] → [needs review standard8]
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+
fix checked in, with nits addressed...
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review standard8]
Comment on attachment 382201 [details] [diff] [review]
fix problems with test_filter.js

carrying forward sr=neil...
Attachment #382201 - Flags: superreview+
fix signature for crash-stats
Summary: crash [@ morkTableRowCursor::NextRow] → crash [@ morkTableRowCursor::NextRow(morkEnv*, mdbOid*, int*)]
Crash Signature: [@ morkTableRowCursor::NextRow(morkEnv*, mdbOid*, int*)]

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.