482432 - crash [@ nsImapFolderCopyState::OnStopRunningUrl(nsIURI*, unsigned int)]

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Ludovic Hirlimann [:Usul]]

0  	thunderbird.exe  	nsImapFolderCopyState::OnStopRunningUrl  	nsImapMailFolder.cpp:6938
1 	thunderbird.exe 	nsMsgMailNewsUrl::SetUrlState 	nsMsgMailNewsUrl.cpp:135
2 	thunderbird.exe 	nsImapMailFolder::SetUrlState 	nsImapMailFolder.cpp:6078
3 	xpcom_core.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
4 	nspr4.dll 	PR_Unlock 	nsprpub/pr/src/threads/combined/prulock.c:355
5 	xpcom_core.dll 	nsProxyObjectCallInfo::Run 	xpcom/proxy/src/nsProxyEvent.cpp:181
6 	xpcom_core.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510

Comment 1

[Wayne Mery (:wsmwk)]

"moving folders local to imap" is the comment for bp-dc61121d-2024-4106-afb0-0b0482090305

doesn't appear in TB2/talkback, so assuming this is a regression. earliest crash found is build 20081205075757 bp-3ab59962-462c-4c39-9d3c-d9f062081208 (but it's pretty hard to say that's the first crash because hunting old crashes on crash-stats is dang hard)

Comment 2

[Magnus Melin [:mkmelin]]

Crashes at http://hg.mozilla.org/comm-central/file/6fcaf32e4036/mailnews/imap/src/nsImapMailFolder.cpp#l6938

The one rv is unchecked, though I don't know if it's needed for these iterator methods.

Comment 3

[Wayne Mery (:wsmwk)]

3.0b3 #8 topcrash, so requesting blocking

Comment 4

[Wayne Mery (:wsmwk)]

dropped to #39 in 3.0b3 ranking and doesn't appear in top 100 of 3.0b4pre. so
topcrash- and removing blocking-thunderbird3?

Comment 5

[David :Bienvenu]

yes, that sounds right. If it regains its previous popularity, we can always move it back on.

Comment 6

[Wayne Mery (:wsmwk)]

bienvenu, magnus, do you think we need more information to attempt a fix?

This looks like a pain point for importers of outlook data. see crash comments at https://crash-stats.mozilla.com/query/query?product=Thunderbird&version=ALL%3AALL&date=03%2F16%2F2010&range_value=4&range_unit=weeks&query_search=signature&query_type=exact&query=nsImapFolderCopyState%3A%3AOnStopRunningUrl%28nsIURI*%2C+unsigned+int%29&build_id=&process_type=all&do_query=1

I don't know precisely if it's certain types of folders or data, or whether it's only moving from local to imap account. Outlook imports to local folders, and it's common for user to then move the imported data to a pop or imap account.

The person in #thunderbird yesterday, viaSanctus, experienced this crash. unfortunately no testcase or log from him. Partial IRC log
<wsmwk_away>	i wonder if we are seeing bug 199079
<firebot>	wsmwk_away: Bug https://bugzilla.mozilla.org/show_bug.cgi?id=199079 maj, --, ---, nobody, NEW, Dragging folders either works, does nothing, or copies only the folder (not the mail inside), and of
<viaSanctus>	whilst office 2K7 hangs
<viaSanctus>	another issue i'm having is that the imap store suddenly asks for the password in the middle of the move process
<wsmwk_away>	which unfortunately is not really in good shape/recently confirmed
<bienvenu>	in the middle? has it actually started talking to the imap server?
<bienvenu>	I'm getting the impression the server is dropping the connection on you
<viaSanctus>	perhaps
<viaSanctus>	does office retry the connection?
<viaSanctus>	because I've moved thens of thousands of mails in outlook without problems
<viaSanctus>	tens*
<bienvenu>	no idea...
<bienvenu>	oh, talking to an exchange server?
<bienvenu>	or a real imap server?
<viaSanctus>	mail enable imap
<viaSanctus>	version 4.0 professional
<viaSanctus>	latest release
<viaSanctus>	includes smtp, web and pop
<wsmwk_away>	or rather http://www.mailenable.com/

Comment 7

[Wayne Mery (:wsmwk)]

#19 for v3.0.3, so worthy of "wanted" for 3.1 and changing back to topcrash

Comment 8

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Potentially fixes it.

Comment 9

[David :Bienvenu]

Comment on attachment 433617 [details] [diff] [review]
proposed fix

I think it would be better not to abort the whole move/copy because we had an error iterating over the db. Instead, I think we should continue to the next message/folder, perhaps with an assert, if possible. Also, we would really like to have a reproducible case for this bug so we could know what exactly is going wrong...

Comment 10

[Magnus Melin [:mkmelin]]

But but, the continuing to next is what's failing (or that's the theory)... How do i go on to next from that?

Comment 11

[Magnus Melin [:mkmelin]]

bienvenu: ^^^

Comment 12

[David :Bienvenu]

if you copy multiple folders, we copy the messages within each folder, chaining them, and create the target folders. If you bail out like the patch proposes, it breaks the chain, and we won't go to the next folder. It's possible that we did find some messages to copy. Given that we don't know what causes this crash, it's hard to know what the right thing to do is...

Comment 13

[Magnus Melin [:mkmelin]]

Ok, i don't really see what else to do here, so back to defaults.

Comment 14

[Makoto Kato [:m_kato]]

hasMoreElements should be initialized at first.

As long as I disassemble code around crash, I believe that messages seems be NULL.

Comment 15

[David :Bienvenu]

Attachment: proposed fix

thx, we should definitely be initializing hasMore to false...

Comment 16

[David :Bienvenu]

fixed on trunk

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 453376 [details] [diff] [review]
proposed fix

a=Standard8 please land this for comm-1.9.1 as well as comm-1.9.2 as the crash happens in both places.

Comment 18

[David :Bienvenu]

fixed for 3.0.x and 3.1.x

Comment 19

[Wayne Mery (:wsmwk)]

Unless we have a good testcase, we wait til 3.1.1 or 3.0.6 ships to verify this is gone because there are no crashes on trunk.

Bienvenu, could checkin http://hg.mozilla.org/comm-central/rev/6521a88c1fc7 have increased the probability of crashing in this area?  Bug 448550 - can't move more than 2 folders to an imap folder.  

And what is the connection to import crashes, which imports to local folders, not imap?  Does import exercise the imap code?

(In reply to comment #17)
> (From update of attachment 453376 [details] [diff] [review])
> a=Standard8 please land this for comm-1.9.1 as well as comm-1.9.2 as the crash
> happens in both places.
>...
> fixed for 3.0.x and 3.1.x

Thanks for that - this is emerging as #1-3 crash early in the 3.1 release. why the increase from not even being top 40 I don't know.  Speculation (based on crash comments) ... more people trying to import.

Lastly, doesn't much matter at this point but crashes go back to at least 20080708 3.0a1 so we're not going to get a regression range. And bug may not even be a regression.

Comment 20

[Wayne Mery (:wsmwk)]

(In reply to comment #19)
> Lastly, doesn't much matter at this point but crashes go back to at least
> 20080708 3.0a1 

correction - 2008050715

Comment 21

[David :Bienvenu]

(In reply to comment #19)

> 
> Bienvenu, could checkin http://hg.mozilla.org/comm-central/rev/6521a88c1fc7
> have increased the probability of crashing in this area?  Bug 448550 - can't
> move more than 2 folders to an imap folder.  
Yes, I think it very well could have increased the probability. 
> 
> And what is the connection to import crashes, which imports to local folders,
> not imap?  Does import exercise the imap code?

No, it doesn't. But perhaps people try to move folders around after import.

Comment 22

[Wayne Mery (:wsmwk)]

replaced url http://crash-stats.mozilla.com/report/index/dc61121d-2024-4106-afb0-0b0482090305 with gsfn http://gsfn.us/t/189xb

I did not search the rest of gsfn for more reports

Comment 23

[Wayne Mery (:wsmwk)]

Does not appear in crash-stats for v3.1.1, v3.1.2 and v3.0.6, so v.fixed