632011 - crash [@ nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, int, int)], [@ nsIMAPBodypartMultipart::Generate], [@ @0x0 | nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, int, int)]

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, int, int)]
trunk top crash, but very few on branches

I thought this was gone because I didn't see any trunk crashes after build 20110114140250. But that must have been a blip. 

bp-bd0d1f91-dcf9-454b-a0be-fb44b2110207 v3.3a3pre 20110206030007
EXCEPTION_ACCESS_VIOLATION_READ
0x0
0	xul.dll	nsIMAPBodypartMultipart::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:1044
1	xul.dll	nsIMAPBodypartMessage::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:898
2	xul.dll	nsIMAPBodyShell::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:296
3	xul.dll	nsImapProtocol::ProcessSelectedStateURL	mailnews/imap/src/nsImapProtocol.cpp:2621
4	xul.dll	nsImapProtocol::ProcessCurrentURL	mailnews/imap/src/nsImapProtocol.cpp:1762
5	xul.dll	nsImapProtocol::ImapThreadMainLoop	mailnews/imap/src/nsImapProtocol.cpp:1401
6	xul.dll	nsImapProtocol::Run	mailnews/imap/src/nsImapProtocol.cpp:1097 

bp-58d319ae-02d7-4195-a606-0835b2110131 v3.3a2
bp-1640deef-81b8-4ac5-9508-910402110112 v3.1.7

Comment 1

[Wayne Mery (:wsmwk)]

more a=... all trunk crashes ... with a different line number for nsIMAPBodyShell.cpp are

A. Mac crashes nsIMAPBodypartMultipart::Generate
bp-d7c8d784-6540-4134-a16c-2b45b2110204
0	XUL	nsIMAPBodypartMultipart::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:1047
1	XUL	nsIMAPBodypartMessage::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:898
2	XUL	nsIMAPBodyShell::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:296
3	XUL	nsImapProtocol::ProcessSelectedStateURL	mailnews/imap/src/nsImapProtocol.cpp:2621
4	XUL	nsImapProtocol::ProcessCurrentURL	mailnews/imap/src/nsImapProtocol.cpp:1762 

bp-743925b3-6771-4c1f-bb07-580382110202


B. @0x0 | nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, int, int)
bp-7252d68b-27ae-4f0a-950f-dabb22110204
EXCEPTION_ACCESS_VIOLATION_EXEC
0x0
0		@0x0	
1	xul.dll	nsIMAPBodypartMultipart::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:1049
2	xul.dll	nsIMAPBodypartMessage::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:898
3	xul.dll	nsIMAPBodyShell::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:296
4	xul.dll	nsImapProtocol::ProcessSelectedStateURL	mailnews/imap/src/nsImapProtocol.cpp:2621
5	xul.dll	nsImapProtocol::ProcessCurrentURL	mailnews/imap/src/nsImapProtocol.cpp:1762

Comment 2

[Wayne Mery (:wsmwk)]

pretty frequent on trunk, as far as trunk crashes go

bp-e9e890a9-cc8a-4a6e-a4fd-f31fe2110303
EXC_BAD_ACCESS / KERN_INVALID_ADDRESS Mac
0x6b
0	XUL	nsIMAPBodypartMultipart::Generate	nsIMAPBodyShell.cpp:1047
1	XUL	nsIMAPBodypartMessage::Generate	nsIMAPBodyShell.cpp:898
2	XUL	nsIMAPBodyShell::Generate	nsIMAPBodyShell.cpp:296
3	XUL	nsImapProtocol::ProcessSelectedStateURL	nsImapProtocol.cpp:2621
4	XUL	nsImapProtocol::ProcessCurrentURL	nsImapProtocol.cpp:1762

Comment 3

[Ludovic Hirlimann [:Usul]]

Wayne how would you feel about contacting the senders of these crashes so we can try to figure out a usage pattern that triggers it ?

Comment 4

[David :Bienvenu]

I had this happen to me once, and there was nothing really interesting about what I was doing - just reading messages. I can try to add a bullet-proofing fix.

If you have autosync turned on, you're not likely to run into this crash, fwiw.

Comment 5

[Wayne Mery (:wsmwk)]

(In reply to comment #4)
> I can try to add a bullet-proofing fix.

That might be good, because there's almost no users reporting email addresses.

Mitra however very recently had such a crash bp-9bae1c2c-ed04-496a-a2fc-cdfb92110521.  I find only two others in the past 4 months
bp-58de3f16-7d82-41ce-8259-6cca52110303 (nightxs)
bp-02ca13bc-5e0c-4401-90a9-6bfee2110223 (kranthi)

Comment 6

[David :Bienvenu]

OK, I have an idea about what's going on here, and a semi-reliable way of reproducing it, involving having an autosync run while we're generating a part. The basic issue is that the shells aren't ref-counted, and there's some crufty code that tries to tell if we've changed the pref for displaying plain text:

http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#820

but unfortunately, m_preferPlainText starts off life uninitialized, which means we call ClearShellCacheForHost fairly reliably. Since the shells aren't ref-counted, and ClearShellCacheForHost deletes the shells, that's a problem.  The multiple connection protection code doesn't help us because autosync is merely doing a status on the folder, which doesn't require a selected state connection. 

I need to fix the prefer plain text code, but I think shells need to be ref-counted as well.

Comment 7

[David :Bienvenu]

Attachment: wip

this makes us ref-count body shells, which should fix the crash. But I still want to fix the initialization of the m_preferPlainText var so we don't go down the bad road at all.

Comment 8

[David :Bienvenu]

Attachment: proposed fix

this switches to holding references to body shells, and cleans up a lot of tabs and long lines, etc.

Comment 9

[David :Bienvenu]

Attachment: fix for TB 5/6

this is a much reduced fix suitable for TB 6 (and possibly TB 5). If we initialize m_preferPlainText correctly, we won't blow away the host's shell cache out from under ourselves.

We can't do an xpcshell test for this until we have support for multiple connections (and even then it will be tricky)

Comment 10

[[e-mail]]

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

There's lots of cleanup in this patch which makes it very difficult to review :-(

Comment 11

[David :Bienvenu]

(In reply to comment #10)
> Comment on attachment 535377 [details] [diff] [review] [review]
> proposed fix
> 
> There's lots of cleanup in this patch which makes it very difficult to
> review :-(

sorry, that code was so badly in need of cleanup (e.g., tabs all over the place). But the real functional change is reference counting via xpcom, and very little else. I removed a function that wasn't used, as well.

Comment 12

[David :Bienvenu]

Attachment: patch w/o cleanup

I haven't tested this extensively, but I believe this has all the non-cleanup stuff.

Comment 13

[[e-mail]]

Comment on attachment 535650 [details] [diff] [review]
patch w/o cleanup

>+NS_IMPL_THREADSAFE_ISUPPORTS0(nsIMAPBodyShell)
...
>-	nsClassHashtable <nsCStringHashKey, nsIMAPBodyShell> m_shellHash;	// For quick lookup based on UID
>-
>+  // For quick lookup based on UID
>+  nsInterfaceHashtable <nsCStringHashKey, nsISupports> m_shellHash;
The hashtable isn't threadsafe so I'm not sure why your refcount is. If it doesn't need to be threadsafe you could use NS_INLINE_DECL_REFCOUNTING.

Also you're better off using an nsRefPtrHashtable to avoid casting. It would also avoid having to inherit from nsISupports, with the added bonus that the refcounting doesn't need to be virtual.

Comment 14

[David :Bienvenu]

Attachment: patch w/o cleanup addressing comments

the shell cache and shells need to be thread-safe because the code that clears out the cache when the pref changes is on the UI thread, and the code that uses the cache and shells is on the imap thread. So I switched to nsRefPtrHashTableMT (much nicer w/o those casts, thx!) but I kept the nsISupports because I didn't see a thread-safe version of NS_INLINE_DECL_REFCOUNTING. Perf-wise, it's not important since this code is not a performance hot-spot by any stretch of the imagination.

Comment 15

[David :Bienvenu]

Attachment: patch with cleanup and additional fix - checked in for miramar

this is what I plan to land. I had to tweak nsIMAPHostSessionList::FindShellInCacheForHost to addref the out body shell.

Comment 17

[David :Bienvenu]

fixed on trunk http://hg.mozilla.org/comm-central/rev/d3359524a8d4

Comment 18

[David :Bienvenu]

this regressed when we invalidated the cache when the user changed the view inline pref.

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 535380 [details] [diff] [review]
fix for TB 5/6

r=me. Please land on miramar, and we'll sort out aurora next week.

Comment 20

[David :Bienvenu]

Comment on attachment 535845 [details] [diff] [review]
patch with cleanup and additional fix - checked in for miramar

one line fix checked into miramar - http://hg.mozilla.org/releases/comm-beta/rev/e8ce8bfea499

Comment 21

[Joe Sabash [:JoeS1]]

Maybe I still have these repositories confused, but shouldn't this be pushed to:
http://hg.mozilla.org/releases/comm-miramar/ as well.

Comment 22

[David :Bienvenu]

I'm not sure - Standard8?

Comment 23

[Mark Banner (:standard8)]

Joe's right, comm-beta isn't in action yet.

Comment 24

[David :Bienvenu]

landed on comm-miramar - http://hg.mozilla.org/releases/comm-miramar/rev/751ac465b6b2

Comment 25

[Justin Wood (:Callek)]

(In reply to comment #23)
> Joe's right, comm-beta isn't in action yet.

Well it is for SeaMonkey, so we thank you for the landing ;-)

Comment 26

[Mark Banner (:standard8)]

Checked into aurora: http://hg.mozilla.org/releases/comm-aurora/rev/f2d5fc07af48