Last Comment Bug 482432 - crash [@ nsImapFolderCopyState::OnStopRunningUrl(nsIURI*, unsigned int)]
: crash [@ nsImapFolderCopyState::OnStopRunningUrl(nsIURI*, unsigned int)]
Status: VERIFIED FIXED
[gs][gssolved]
: crash, regression, topcrash
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: 1.9.1 Branch
: x86 All
: -- critical (vote)
: Future
Assigned To: David :Bienvenu
:
:
Mentors:
http://gsfn.us/t/189xb
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-10 03:27 PDT by Ludovic Hirlimann [:Usul]
Modified: 2015-10-07 18:39 PDT (History)
8 users (show)
dmose: wanted‑thunderbird+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
.1-fixed
.6-fixed


Attachments
proposed fix (1022 bytes, patch)
2010-03-19 13:00 PDT, Magnus Melin
mozilla: review-
mozilla: superreview-
Details | Diff | Splinter Review
proposed fix (854 bytes, patch)
2010-06-23 07:48 PDT, David :Bienvenu
neil: review+
neil: superreview+
standard8: approval‑thunderbird3.0.6+
standard8: approval‑thunderbird3.1.1+
Details | Diff | Splinter Review

Description Ludovic Hirlimann [:Usul] 2009-03-10 03:27:57 PDT
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, NI for questions) 2009-03-10 04:20:39 PDT
"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 2009-04-18 02:53:42 PDT
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, NI for questions) 2009-07-27 12:09:22 PDT
3.0b3 #8 topcrash, so requesting blocking
Comment 4 Wayne Mery (:wsmwk, NI for questions) 2009-08-10 11:45:45 PDT
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 2009-08-10 11:51:13 PDT
yes, that sounds right. If it regains its previous popularity, we can always move it back on.
Comment 6 Wayne Mery (:wsmwk, NI for questions) 2010-03-16 05:17:55 PDT
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, NI for questions) 2010-03-17 10:56:11 PDT
#19 for v3.0.3, so worthy of "wanted" for 3.1 and changing back to topcrash
Comment 8 Magnus Melin 2010-03-19 13:00:32 PDT
Created attachment 433617 [details] [diff] [review]
proposed fix

Potentially fixes it.
Comment 9 David :Bienvenu 2010-03-21 09:02:44 PDT
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 2010-03-22 13:03:09 PDT
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 2010-04-02 11:44:20 PDT
bienvenu: ^^^
Comment 12 David :Bienvenu 2010-04-02 11:52:36 PDT
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 2010-04-02 12:11:55 PDT
Ok, i don't really see what else to do here, so back to defaults.
Comment 14 Makoto Kato [:m_kato] 2010-06-23 01:22:42 PDT
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 2010-06-23 07:48:50 PDT
Created attachment 453376 [details] [diff] [review]
proposed fix

thx, we should definitely be initializing hasMore to false...
Comment 16 David :Bienvenu 2010-06-23 08:14:48 PDT
fixed on trunk
Comment 17 Mark Banner (:standard8) 2010-06-23 08:45:08 PDT
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 2010-06-23 08:58:06 PDT
fixed for 3.0.x and 3.1.x
Comment 19 Wayne Mery (:wsmwk, NI for questions) 2010-06-26 07:14:41 PDT
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, NI for questions) 2010-06-26 07:20:19 PDT
(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 2010-06-26 09:05:12 PDT
(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, NI for questions) 2010-07-20 08:32:18 PDT
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, NI for questions) 2010-08-17 10:25:54 PDT
Does not appear in crash-stats for v3.1.1, v3.1.2 and v3.0.6, so v.fixed

Note You need to log in before you can comment on or make changes to this bug.