All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Thunderbird 3.0b3

Status

--
critical
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: wsmwk, Assigned: Bienvenu)

Tracking

({crash, topcrash})

Trunk
Thunderbird 3.0b3
x86
All
crash, topcrash
Bug Flags:
blocking-thunderbird3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 2

10 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
(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

10 years ago
i'll leave to others whether this should block bug 464354 - but I suspect it should
(Assignee)

Comment 5

10 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

10 years ago
Flags: blocking-thunderbird3? → blocking-thunderbird3+
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 6

10 years ago
Created attachment 381881 [details] [diff] [review]
proposed fix

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

10 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

10 years ago
one of the gloda tests seems unhappy with this patch, natch. I'll investigate.
(Assignee)

Comment 9

10 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

10 years ago
Created attachment 381984 [details] [diff] [review]
proposed fix with unit test

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

10 years ago
Whiteboard: [has patch for review, standard8, neil]
Target Milestone: --- → Thunderbird 3.0b3

Comment 11

10 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 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

10 years ago
Created attachment 382132 [details] [diff] [review]
un-bitrotted patch
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).
(Assignee)

Comment 15

10 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

10 years ago
Created attachment 382201 [details] [diff] [review]
fix problems with test_filter.js

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

10 years ago
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+
(Assignee)

Comment 18

10 years ago
fix checked in, with nits addressed...
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review standard8]
(Assignee)

Comment 19

10 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

9 years ago
fix signature for crash-stats
Summary: crash [@ morkTableRowCursor::NextRow] → crash [@ morkTableRowCursor::NextRow(morkEnv*, mdbOid*, int*)]
Crash Signature: [@ morkTableRowCursor::NextRow(morkEnv*, mdbOid*, int*)]
You need to log in before you can comment on or make changes to this bug.