534858 - Crash on repeated collapse/expand of threads with subthreads killed by filter [@ memmove | nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int)] - nsMsgDBView::RemoveRows

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[[e-mail]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.23) Gecko/20090812

may be #524266 is relevant, but marked as resolved.
Crash, if a thread is collapsed/expanded multiple times, that contains posts that are killed by filter (kill sub-thread).


Reproducible: Always

Steps to Reproduce:
1. Make filter: If sender is in adressbook PLONK->mark as read + kill subthread
2. Create Addressbook PLONK and add entry (e.g.) Harald Hengel/[e-mail]
2. Subscribe to newsgroup de.comp.security.virus
3. Select thread  "Welche Antivirus Programme nutzt ihr?" 28 Oct 2009 13:35:46 +0100
4. Multiple times collapse/expand the thread
5. Later posts start vanishing
6. crash
Actual Results:  
crash

Expected Results:  
not crash

Tested with new profile.
No extensions installed.

Crash report:
Add-ons: {972ce4c6-7e08-4474-a285-3208198ce6fd}:3.0
BuildID: 20091204171430
CrashTime: 1260882592
Email: [e-mail]
InstallTime: 1260360135
ProductName: Thunderbird
SecondsSinceLastCrash: 129
StartupTime: 1260882568
Theme: classic/1.0
Throttleable: 1
URL: 
Vendor: 
Version: 3.0

Comment 1

[Ludovic Hirlimann [:Usul]]

Did you submit the crashes ? can you provide us with a crash id (see http://kb.mozillazine.org/Breakpad) ?

Comment 2

[[e-mail]]

Attachment: Still pending crashdump

Comment 3

[[e-mail]]

Attachment: Associated .extra

Comment 4

[[e-mail]]

Submitted crashes:
bp-152833f6-faf1-4ad0-8e4b-453272091215
bp-5a206f20-f3ca-463f-b937-3a1132091215
bp-5f3f40ca-4599-47e7-9c45-a49062091215
bp-80c0ad41-73a2-4edc-bc0e-69fe12091215
bp-a1019ee0-0d8c-4caa-84ab-fdaef2091215

Comment 5

[timeless]

Signature	memmove | nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int)
UUID	152833f6-faf1-4ad0-8e4b-453272091215
Time 	2009-12-15 02:14:46.269632
Uptime	7640
Last Crash	7833 seconds before submission
Product	Thunderbird
Version	3.0
Build ID	20091204171430
Branch	1.9.1
OS	Windows NT
OS Version	5.1.2600 Service Pack 2
CPU	x86
CPU Info	GenuineIntel family 15 model 2 stepping 7
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x4c00000
User Comments	Crash after collapsing thread in news
Processor Notes 	
Related Bugs

Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	mozcrt19.dll 	memmove 	MEMCPY.ASM:188
1 	xpcom_core.dll 	nsTArray_base::ShiftData 	objdir-tb/mozilla/xpcom/build/nsTArray.cpp:173
2 	thunderbird.exe 	nsMsgDBView::RemoveRows 	mailnews/base/src/nsMsgDBView.cpp:5203
3 	thunderbird.exe 	nsMsgDBView::CollapseByIndex 	mailnews/base/src/nsMsgDBView.cpp:4778
4 	thunderbird.exe 	nsMsgDBView::ToggleExpansion 	mailnews/base/src/nsMsgDBView.cpp:4605
5 	thunderbird.exe 	nsMsgDBView::ToggleOpenState 	mailnews/base/src/nsMsgDBView.cpp:1935
6 	xpcom_core.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
7 	thunderbird.exe 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2456
8 	thunderbird.exe 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1590

Comment 6

[Wayne Mery (:wsmwk)]

#10 crash => topcrash

xref Bug 524064 - crash [@memmove | nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int) ] - whcih was also about expanding and collapsing threads

Comment 7

[Wayne Mery (:wsmwk)]

#7 crash for 3.0.1, so nominating blocking 3.1 to see if we can get this for 3.0.

bienvenu, do we need >1 bug?  some stacks are like reporter's. but others, like mine look imap related. and do you need steps?

view examples (with email addresses):
- bp-a6c2d030-f008-42e8-8919-5c9aa2100212
- bp-55e4826d-6def-4a13-9aac-722cd2100212
- bp-a3b657c9-3f58-4458-a979-a791d2100213 (no address)

imap examples:
- bp-fa0cad52-efd5-4dca-8b40-ed87e2100213
- bp-81eaaf3a-c149-4467-9d58-022cd2100216 I just hit this using 3.1 and wasn't doing anything in that instance for probably 12+ hours. 
0  	mozcrt19.dll  	memmove  	 MEMCPY.ASM:188
1 	xpcom_core.dll 	nsTArray_base::ShiftData 	objdir-tb/mozilla/xpcom/build/nsTArray.cpp:173
2 	thunderbird.exe 	nsImapFlagAndUidState::ExpungeByIndex 	mailnews/imap/src/nsImapFlagAndUidState.cpp:199
3 	thunderbird.exe 	nsImapServerResponseParser::numeric_mailbox_data 	mailnews/imap/src/nsImapServerResponseParser.cpp:1039
4 	thunderbird.exe 	nsImapServerResponseParser::response_data 	mailnews/imap/src/nsImapServerResponseParser.cpp:756
5 	thunderbird.exe 	nsImapServerResponseParser::ParseIMAPServerResponse 	mailnews/imap/src/nsImapServerResponseParser.cpp:243
6 	thunderbird.exe 	nsImapProtocol::ParseIMAPandCheckForNewMail 	mailnews/imap/src/nsImapProtocol.cpp:1861
7 	thunderbird.exe 	nsImapProtocol::Expunge 	mailnews/imap/src/nsImapProtocol.cpp:5197
8 	thunderbird.exe 	nsImapProtocol::ProcessMailboxUpdate 	mailnews/imap/src/nsImapProtocol.cpp:3949
9 	thunderbird.exe 	nsImapProtocol::ProcessSelectedStateURL 	mailnews/imap/src/nsImapProtocol.cpp:2824
10 	thunderbird.exe 	nsImapProtocol::ProcessCurrentURL 	mailnews/imap/src/nsImapProtocol.cpp:1715
11 	thunderbird.exe 	nsImapProtocol::ImapThreadMainLoop 	mailnews/imap/src/nsImapProtocol.cpp:1363
12 	thunderbird.exe 	nsImapProtocol::Run 	mailnews/imap/src/nsImapProtocol.cpp:1062

Comment 8

[David :Bienvenu]

The imap related crash in 3.1 will eventually be fixed when the patch in bug 537551 gets sr'd and landed.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

After discussion with bienvenu and jcranmer, what's left here appears to be NNTP-only, which is low-priority for us these days, so we wouldn't block based on that alone.  So marking blocking-.

However, bienvenu thinks that _if_ he can get a way to reproduce the bug, it should be an easy fix, jcranmer thinks he's got a way to reproduce, and fixing crashers is always a good thing, so marking wanted+.

Comment 10

[David :Bienvenu]

Attachment: fix for crash

this fixes the crash by making collapse always count in the view how many messages are in the expanded thread, instead of just looking at the thread count from the db. This is a bit slower, but much more accurate.

I rearranged the code a bit to make more sense (e.g., toggle the flag after figuring out the expansion delta, bail out w/o changing the flag in case of errors, etc).

Now I need to write a unit test for this...

Comment 11

[David :Bienvenu]

Attachment: propsed fix with unit test

as described earlier, the fix for collapse is to actually count in the view how many messages are in the expanded thread.

Comment 12

[David :Bienvenu]

I'd like to get this into b1, since it appears to be a top crash.

Comment 13

[[e-mail]]

Comment on attachment 427625 [details] [diff] [review]
propsed fix with unit test

>-  if (!(m_viewFlags & nsMsgViewFlagsType::kUnreadOnly))
[How did this ever work for collapsed unread only threads?]

>+  if (flags & nsMsgMessageFlags::Elided)
Can we merge this with the (now identical condition) following if block?
(I wonder whether everyone using ExpansionDelta for an expanded thread should actually be using CountExpandedThread instead.)

>-    PRInt32 numRemoved = threadCount; // don't count first header in thread
>+  PRInt32 numRemoved = -rowDelta; // don't count first header in thread
This change was confusing, but we're now calling ExpansionDelta before marking the thread collapsed, thus negating the return value, right?

Comment 14

[David :Bienvenu]

(In reply to comment #13)
> (From update of attachment 427625 [details] [diff] [review])
> >-  if (!(m_viewFlags & nsMsgViewFlagsType::kUnreadOnly))
> [How did this ever work for collapsed unread only threads?]

Basic unread only view is basically not used because we create a quick search view with unread as a criteria in the UI,and nsMsgQuickSearchDBView overrides ExpansionDelta.

> 
> >+  if (flags & nsMsgMessageFlags::Elided)
> Can we merge this with the (now identical condition) following if block?

yes, for some reason, I thought that would look worse, but it's fine.

> (I wonder whether everyone using ExpansionDelta for an expanded thread should
> actually be using CountExpandedThread instead.)

I suppose, but ExpansionDelta is convenient because it does the negation of the count for you, and I like the idea of not having to check whether the thread is expanded or not. It also does some extra checks, like whether you're threaded or not, and index validation...

> 
> >-    PRInt32 numRemoved = threadCount; // don't count first header in thread
> >+  PRInt32 numRemoved = -rowDelta; // don't count first header in thread
> This change was confusing, but we're now calling ExpansionDelta before marking
> the thread collapsed, thus negating the return value, right?
Right, the change is confusing, I think because the code was confusing, and I hope the resulting code is less confusing now.

Comment 15

[David :Bienvenu]

Attachment: fix addressing one of Neil's comment

Comment 16

[[e-mail]]

[Note that these comments are not a review prerequisite.]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 427625 [details] [diff] [review])
> > >-  if (!(m_viewFlags & nsMsgViewFlagsType::kUnreadOnly))
> > [How did this ever work for collapsed unread only threads?]
> Basic unread only view is basically not used because we create a quick search
> view with unread as a criteria in the UI
For my bugmail, which I admittedly read using an old XPFE SeaMonkey 1.5a, I choose View - Threads - Unread, which is a completely different kettle of fish to View - Messages - Unread, which I agree is a quick search view.
However I don't actually turn threading on for this view, since that requires me to receive the original bugmail for each thread to get created correctly.

> > (I wonder whether everyone using ExpansionDelta for an expanded thread should
> > actually be using CountExpandedThread instead.)
> I suppose, but ExpansionDelta is convenient because it does the negation of the
> count for you, and I like the idea of not having to check whether the thread is
> expanded or not. It also does some extra checks, like whether you're threaded
> or not, and index validation...
Sure, but there are only 4 callers to ExpansionDelta, and all 4 know whether or not the thread is expanded or not, and therefore that the index is valid, and the three that operate on collapsed threads need to negate the count anyway...

Comment 17

[David :Bienvenu]

(In reply to comment #16)

> For my bugmail, which I admittedly read using an old XPFE SeaMonkey 1.5a, I
> choose View - Threads - Unread, which is a completely different kettle of fish
> to View - Messages - Unread, which I agree is a quick search view.
> However I don't actually turn threading on for this view, since that requires
> me to receive the original bugmail for each thread to get created correctly.

I think it was TB 1.5 that switched over to making view | unread use a quick search (much to my surprise and annoyance, since at the time that meant you couldn't thread). But that was done in the TB UI, so I'm not sure what SM does or did. But I bet the backend was pretty broken. There were also a lot of changes to the view code to support threading in cross-folder search results...

> Sure, but there are only 4 callers to ExpansionDelta, and all 4 know whether or
> not the thread is expanded or not, and therefore that the index is valid, and
> the three that operate on collapsed threads need to negate the count anyway...

Good points, all.

Comment 18

[David :Bienvenu]

fix checked in - I think we should get this for 3.03 as well.

Comment 19

[David :Bienvenu]

thx again to jcranmer for the test case.

Comment 20

[Wayne Mery (:wsmwk)]

apparently also shows up as fastcopy_and prob other random sigs.
I just saw this bp-1e8cded2-0c75-41a0-8655-438b32100220  3.1b1pre
0  	mozcrt19.dll  	fastcopy_I  	
1 	mozcrt19.dll 	_VEC_memcpy 	
2 	mozcrt19.dll 	_VEC_memcpy 	
3 	xpcom_core.dll 	nsTArray_base::ShiftData 	objdir-tb/mozilla/xpcom/build/nsTArray.cpp:173

someone else's 
bp-d8951d4f-e463-46c8-826a-924b22100205
0  	mozcrt19.dll  	fastcopy_I  	
1 	mozcrt19.dll 	_VEC_memcpy 	
2 	mozcrt19.dll 	_VEC_memcpy 	
3 	thunderbird.exe 	nsMsgGroupThread::GetChildAt 	mailnews/base/src/nsMsgGroupThread.cpp:248
4 	xpcom_core.dll 	nsTArray_base::ShiftData 	objdir-tb/mozilla/xpcom/build/nsTArray.cpp:173

Comment 21

[[e-mail]]

(In reply to comment #17)
> I think it was TB 1.5 that switched over to making view | unread use a quick
> search (much to my surprise and annoyance, since at the time that meant you
> couldn't thread). But that was done in the TB UI, so I'm not sure what SM does
> or did. But I bet the backend was pretty broken.
I checked MXR and both TB and SM support View - Threads - Unread using the kUnreadOnly flag, as distinct from View - Messages - Unread which does not.

Comment 23

[David :Bienvenu]

fixed for 3.0.4

Comment 24

[Wayne Mery (:wsmwk)]

post 3.0.4, we should check if v3.0.3 Mac crash for [@ nsMsgDBView::RemoveRows(unsigned int, int)]  goes away
http://crash-stats.mozilla.com/query/query?product=Thunderbird&version=ALL%3AALL&date=&range_value=3&range_unit=weeks&query_search=signature&query_type=contains&query=nsMsgDBView%3A%3ARemoveRows&build_id=&process_type=all&do_query=1 
(which may be bug 505960, but not reopening unless someone has a strong argument for doing so)

Comment 25

[Wayne Mery (:wsmwk)]

Not marking this verified as yet. To properly verify without a lot of labor, we need Bug 555599.  (*top* of stack is #4 for 3.0.6 and #67 for 3.1.2)

A (non-scientific) sampling of 12 v3.0.6 crashes sindicate this is mostly gone. The sample has one crash for this bug - bp-f1eb8a6a-2e98-4ca8-8a4d-2dcfb2100809. 

Can someone please look at that crash report?