Closed Bug 632011 Opened 9 years ago Closed 8 years ago

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

Categories

(MailNews Core :: Networking: IMAP, defect, critical)

x86
Windows Vista
defect
Not set
critical

Tracking

(blocking-thunderbird5.0 needed, thunderbird5.0 beta2-fixed, thunderbird6 fixed)

RESOLVED FIXED
Thunderbird 7.0
Tracking Status
blocking-thunderbird5.0 --- needed
thunderbird5.0 --- beta2-fixed
thunderbird6 --- fixed

People

(Reporter: wsmwk, Assigned: Bienvenu)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files, 3 obsolete files)

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
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
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
Summary: crash [@ nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, int, int)] → crash [@ nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, int, int)], [@ nsIMAPBodypartMultipart::Generate], [@ @0x0 | nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, int, int)]
Component: MIME → Networking: IMAP
QA Contact: mime → networking.imap
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 ?
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.
(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)
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.
Assignee: nobody → dbienvenu
Attached patch wip (obsolete) — Splinter Review
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.
Attached patch proposed fix (obsolete) — Splinter Review
this switches to holding references to body shells, and cleans up a lot of tabs and long lines, etc.
Attachment #535263 - Attachment is obsolete: true
Attachment #535377 - Flags: review?(neil)
Attached patch fix for TB 5/6Splinter Review
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)
Attachment #535380 - Flags: review?(mbanner)
Comment on attachment 535377 [details] [diff] [review]
proposed fix

There's lots of cleanup in this patch which makes it very difficult to review :-(
(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.
Attached patch patch w/o cleanup (obsolete) — Splinter Review
I haven't tested this extensively, but I believe this has all the non-cleanup stuff.
Attachment #535650 - Flags: review?(neil)
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.
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.
Attachment #535650 - Attachment is obsolete: true
Attachment #535758 - Flags: review?(neil)
Attachment #535650 - Flags: review?(neil)
Attachment #535758 - Flags: review?(neil) → review+
this is what I plan to land. I had to tweak nsIMAPHostSessionList::FindShellInCacheForHost to addref the out body shell.
Attachment #535377 - Attachment is obsolete: true
Attachment #535377 - Flags: review?(neil)
Duplicate of this bug: 645383
fixed on trunk http://hg.mozilla.org/comm-central/rev/d3359524a8d4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 7.0
this regressed when we invalidated the cache when the user changed the view inline pref.
Keywords: regression
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.
Attachment #535380 - Flags: review?(mbanner)
Attachment #535380 - Flags: review+
Attachment #535380 - Flags: approval-thunderbird3.3+
Attachment #535380 - Flags: approval-comm-aurora?
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
Attachment #535845 - Attachment description: patch with cleanup and additional fix → patch with cleanup and additional fix - checked in for miramar
Maybe I still have these repositories confused, but shouldn't this be pushed to:
http://hg.mozilla.org/releases/comm-miramar/ as well.
I'm not sure - Standard8?
Joe's right, comm-beta isn't in action yet.
blocking-thunderbird5.0: --- → ?
(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 ;-)
blocking-thunderbird5.0: ? → needed
Crash Signature: [@ nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, int, int)] [@ nsIMAPBodypartMultipart::Generate] [@ @0x0 | nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, int, int)]
Attachment #535380 - Flags: approval-comm-aurora? → approval-comm-aurora+
Checked into aurora: http://hg.mozilla.org/releases/comm-aurora/rev/f2d5fc07af48
Crash Signature: [@ nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, int, int)] [@ nsIMAPBodypartMultipart::Generate] [@ @0x0 | nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, int, int)] → [@ nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, int, int)] [@ nsIMAPBodypartMultipart::Generate] [@ @0x0 | nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, int, int)]
Duplicate of this bug: 662229
You need to log in before you can comment on or make changes to this bug.