842963 - Crash loop/repeats in libxul.so nsMsgGroupThread::RemoveChildAt

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mihai Sucan [:msucan]]

This bug was filed from the Socorro interface and is 
report bp-423c4afa-e83b-4509-99de-947bf2130220 .
============================================================= 

Thunderbird 19 keeps crashing quite often for me. I noticed I can crash it by changing filters in Gmail while Thunderbird is running or if I change labels / folders. I can do this with Zimbra as well and Thunderbird crashes with those accounts.

If I am not mistaken you need to have a virtual folder associated with an IMAP account where you make the folder changes via a web mail UI, while Thunderbird is started. You also need to make sure you have the virtual folder selected, so you are viewing it.

This is a crash loop - if I choose to restart Thunderbird, it crashes again when it starts to load emails from the server. I have to be quick and switch to a regular IMAP folder, to avoid the crash.

I had many crashes today, and one came in just now, while I wasn't playing with filters. This is the list of crash reports I have submitted until now:

  bp-423c4afa-e83b-4509-99de-947bf2130220
  bp-4fe43423-f39e-4951-bc72-5f1a12130219
  bp-2383e4bf-278c-49d6-877e-a7aac2130219
  bp-374ba99b-9df1-497f-82bc-5f21b2130219
  bp-ef3782fb-5170-47d3-a90f-d447f2130219
  bp-750f6157-9845-413e-88cc-0decf2130219
  bp-3e3b08ba-b8de-4a79-bad7-c7d4a2130219

Please let me know if I can do anything to help fixing this crasher. Thank you!

(I'm not sure I picked the correct component, please move this report as needed.)

Comment 1

[Ludovic Hirlimann [:Usul]]

0 	libc-2.15.so 	libc-2.15.so@0x14f5c0 	
1 	libxul.so 	nsMsgGroupThread::RemoveChildAt 	nsTArray.h:945
2 	libxul.so 	nsMsgXFGroupThread::RemoveChildAt 	nsMsgGroupThread.cpp:806
3 	libxul.so 	nsMsgGroupThread::RemoveChildHdr 	nsMsgGroupThread.cpp:258
4 	libxul.so 	nsMsgGroupView::OnHdrDeleted 	nsMsgGroupView.cpp:651
5 	libxul.so 	nsMsgSearchDBView::OnHdrDeleted 	nsMsgSearchDBView.cpp:220
6 	libxul.so 	nsMsgDatabase::NotifyHdrDeletedAll 	nsMsgDatabase.cpp:858
7 	libxul.so 	nsMsgDatabase::DeleteHeader 	nsMsgDatabase.cpp:1942
8 	libxul.so 	nsMsgDatabase::DeleteMessages 	nsMsgDatabase.cpp:1886
9 	libxul.so 	nsImapMailFolder::OnStopRunningUrl 	nsImapMailFolder.cpp:5285
10 	libxul.so 	nsMsgMailNewsUrl::SetUrlState 	nsMsgMailNewsUrl.cpp:104
11 	libxul.so 	nsImapMailFolder::SetUrlState 	nsImapMailFolder.cpp:6837
12 	libxul.so 	SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, 	nsSyncRunnableHelpers.cpp:250
13 	libxul.so 	nsThread::ProcessNextEvent 	nsThread.cpp:627
14 	libxul.so 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:238
15 	libxul.so 	mozilla::ipc::MessagePump::Run 	MessagePump.cpp:117
16 	libxul.so 	MessageLoop::Run 	message_loop.cc:208
17 	libxul.so 	nsBaseAppShell::Run 	nsBaseAppShell.cpp:163
18 	libxul.so 	nsAppStartup::Run 	nsAppStartup.cpp:290
19 	libxul.so 	XREMain::XRE_mainRun 	nsAppRunner.cpp:3823
20 	libxul.so 	XREMain::XRE_main 	nsAppRunner.cpp:3890
21 	libxul.so 	XRE_main 	nsAppRunner.cpp:4084
22 	thunderbird 	main 	nsMailApp.cpp:111
23 	libc-2.15.so 	libc-2.15.so@0x2176d 	
24 	thunderbird 	thunderbird@0x2120 	
25 	thunderbird 	main 	nsMailApp.cpp:205
26 		@0x7fff55351210

Comment 2

[Wayne Mery (:wsmwk)]

msucan, no crashes when using TB18 beta?

ludo, does it look to you like one or both of bug 616229 and bug 646810  are the same?

Comment 3

[Mihai Sucan [:msucan]]

(In reply to Wayne Mery (:wsmwk) from comment #2)
> msucan, no crashes when using TB18 beta?

I don't have tb18 beta. I just started using TB last week. This is tb19 from the ubuntu ppa for tb-beta channel.

Is it possible this bug is related to bug 662091? I've been reading through that and I notice two things in common: RemoveChildAt() and grouping by date. I use views with grouping by date.

Comment 4

[Mihai Sucan [:msucan]]

Another crash happened right now - just checking for new emails:

https://crash-stats.mozilla.com/report/index/bp-3f9e32e6-8c22-4b2d-92a0-720db2130220

I was in a virtual folder, with grouping by date.

Comment 5

[Mihai Sucan [:msucan]]

Another crash:

https://crash-stats.mozilla.com/report/index/bp-a6db4cba-10e6-47d9-b989-369a12130221

Just checking email, virtual folder, grouping by date.

What can I do to help fixing this bug?

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to Mihai Sucan [:msucan] from comment #5)
> What can I do to help fixing this bug?

I think wait on fixing of bug 616229, and then test the resulting build

Comment 7

[:aceman]

Wayne, why are there no links to source files in these crash reports?

Comment 8

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #7)
> Wayne, why are there no links to source files in these crash reports?

Comment 9

[Ludovic Hirlimann [:Usul]]

Could be that the symbols didn't get uploaded properly, Mihai where did you get the build that fails , from your distro's repo or from ftp.mozilla.org ?

Comment 10

[(not currently active) Ted Mielczarek]

Doesn't look like the build had any source info to use:
0|2|libxul.so|nsMsgXFGroupThread::RemoveChildAt(int)|/build/buildd/thunderbird-19.0~b1+build1/mailnews/base/src/nsMsgGroupThread.cpp|806|0x5

That sounds like an Ubuntu build to me.

Comment 11

[Mihai Sucan [:msucan]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> Doesn't look like the build had any source info to use:
> 0|2|libxul.so|nsMsgXFGroupThread::RemoveChildAt(int)|/build/buildd/
> thunderbird-19.0~b1+build1/mailnews/base/src/nsMsgGroupThread.cpp|806|0x5
> 
> That sounds like an Ubuntu build to me.

Yes. I'm using Thunderbird from the Mozilla PPA: 

https://launchpad.net/~mozillateam/+archive/thunderbird-next

Comment 12

[Leho Kraav (:macmaN @lkraav)]

thunderbird-bin-17.0.4, gentoo

I am now in crash city with this. 8 crashes in 3 days with the topic signature, while previous rate has been ~0.5 crashes/month. I think this streak may have started when I added a new search folder to an IMAP subfolder.

bp-5da6cb26-70a1-4663-8ed9-9938e2130403
bp-44d672a7-0489-4373-9bbf-1c9e32130403
bp-ae86c7b1-11e5-42c3-bbb1-6c6882130402
bp-62b37a35-6d19-4b64-9ba2-d519d2130402
bp-c6068ee0-13fd-4c51-8c87-5df372130402
bp-0f1f7f4c-d1f8-42d7-978a-22aff2130401
bp-c897baf1-72be-4807-8300-739752130401
bp-a98da93c-5f99-425e-9860-317132130401

Comment 13

[:aceman]

Not sure what nsTArray.RemoveElementAt(index) does when index is out of bounds (uses MOZ_ASSERT to check bounds but not sure if that does anything in release builds). But until somebody finds the reason this is crashing we can at least add a bounds check at http://hg.mozilla.org/releases/comm-esr17/annotate/705a2b7c7e19/mailnews/base/src/nsMsgGroupThread.cpp#l227 as is usual in these type of accessors.

Comment 14

[Leho Kraav (:macmaN @lkraav)]

Would this hypothetical patch make it into a 17.0.5 drop, or is the next major version already behind the corner?

Comment 15

[:aceman]

Yes, we do port crash fixes to 17.0.x. The next major version (TB 24) is still some 6 months away. But 17.0.5 is already out since yesterday.

Aryx, is it possible to create a TB17.0.x try build, or does that work only for trunk?

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

I don't know enough about the build environment to answer this, better ask Mark or Irving.

Comment 17

[Mark Banner (:standard8)]

You can just push comm-esr17 with the appropriate patches applied to try-comm-central and it should give you some builds.

Comment 18

[:aceman]

Aryx, do you know how that is done?

Comment 19

[:aceman]

Attachment: test patch

Aryx, if you could make a try-build with this patch and paste the link to download it.

Comment 20

[:aceman]

Standard8, we still couldn't find out how to make a TB17 try build. Can you do it from the patch here?

Comment 21

[Mark Banner (:standard8)]

Its here:

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=9163d6e9752e

For reference, this is what I have done:

$ cd comm-esr17
$ hg pull -u
$ hg qimport -n 842964 https://bugzilla.mozilla.org/attachment.cgi?id=733047
$ hg qrefresh -m "Message..."
$ hg push -f trycc

Comment 22

[:aceman]

Thanks standard8.

Guys who see the problem, please try the appropriate build from here (windows one is probably broken):
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-9163d6e9752e/

It is TB17.0.5 with the patch applied, so should be compatible with your profile. Just try the .tar.bz2 archive, extract to your home directory and run 'thunderbird' binary from the extracted directory. No need to uninstall your main TB installation from your package manager.

The build should not crash and tell us if any operation you do is now not working properly.

Comment 23

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #22)
> Thanks standard8.
> 
> Guys who see the problem, please try the appropriate build from here
> (windows one is probably broken):
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/
> [e-mail]-9163d6e9752e/
> 
> It is TB17.0.5 with the patch applied, so should be compatible with your
> profile. Just try the .tar.bz2 archive, extract to your home directory and
> run 'thunderbird' binary from the extracted directory. No need to uninstall
> your main TB installation from your package manager.
> 
> The build should not crash and tell us if any operation you do is now not
> working properly.

leho?
Mahai?

Comment 24

[Mihai Sucan [:msucan]]

That URL is no longer valid. Also Ubuntu PPA updated me to Thunderbird 22, no longer using TB17.

The crash still happens with TB22. I had two crashes today.

Unfortunately, I don't have time to do my own Thunderbird builds with custom patches applied.

Comment 25

[Wayne Mery (:wsmwk)]

(In reply to Mark Banner (:standard8) from comment #21)
> Its here:
> 
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=9163d6e9752e
> 
> For reference, this is what I have done:
> 
> $ cd comm-esr17
> $ hg pull -u
> $ hg qimport -n 842964 https://bugzilla.mozilla.org/attachment.cgi?id=733047
> $ hg qrefresh -m "Message..."
> $ hg push -f trycc

aryx, josiahone, can one of you please resubmit this try build which has expired?

Comment 26

[Josiah Bruner [:jsbruner]]

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=76378446bfce

Comment 27

[Wayne Mery (:wsmwk)]

This should have been fixed in TB17.0.6 via bug 616229

Comment 29

[Leho Kraav (:macmaN @lkraav)]

So yeah I'm getting this in 24 beta 2. Bug 616229 says "gone in 17.0.7", isn't this supposed to be in 24 betas?

Comment 30

[Wayne Mery (:wsmwk)]

(In reply to Leho Kraav (:macmaN @lkraav) from comment #29)
> So yeah I'm getting this in 24 beta 2. Bug 616229 says "gone in 17.0.7",
> isn't this supposed to be in 24 betas?

impossible to say, given that we never heard results from anyone, both on the newer 17.0.x nor the try build we offered. It's time to man up!

(until then I reserve judgement whether your crash is actually the same issue)

Comment 31

[Leho Kraav (:macmaN @lkraav)]

Yeah like I reported in another bug, I just recently figured out how to get betas going on Gentoo, and I even have a working profiler now, too. Look out below!

Comment 32

[Mihai Sucan [:msucan]]

Thanks for the work on fixing the crasher! Much appreciated. Apologies for the delays - the email client is central to the work I do and I can't crash it often.

I still get the same crasher in Thunderbird 24 from the Ubuntu Thunderbird PPAs, using the beta channel. I just tested today and it crashed for me - no crash ID because Ubuntu's apport kicked-in (yay). I did report the crash through apport.

"Same crasher" simply means very similar repro steps to those I reported.

Comment 33

[:aceman]

The patch attached here is no no official Tb version (either TB17 nor TB24beta). It is only in the special build pointed to by comment 22 or comment 26. So please try with that one. But maybe that is already expired too. Can anybody make a trybuild of TB24 (so that the testers do not try a downgraded version) with this patch?

Comment 34

[Leho Kraav (:macmaN @lkraav)]

(v24) I am motivated to run a new trybuild for this. It's the only thing regularly crashing TB for me. So close to mail heaven..!

Comment 35

[Wayne Mery (:wsmwk)]

Leho, how's things doing with the patch?

Comment 36

[Leho Kraav (:macmaN @lkraav)]

Last crash of any kind was exactly 2 months. Here's my timelines:

Tue Feb 11 12:51:10 2014 >>> mail-client/thunderbird-bin-24.3.0
Sat Nov 23 14:17:34 2013 >>> mail-client/thunderbird-bin-24.1.1
Wed Sep 18 06:59:30 2013 >>> mail-client/thunderbird-bin-24.0
Fri Aug 30 04:14:46 2013 >>> mail-client/thunderbird-bin-24.0_beta2
...

bp-e8b9685e-01d4-4915-ab75-6e4392131212	12.12.2013	16:16
bp-7b623aa9-dfad-435d-ae07-58a5b2131127	27.11.2013	15:54
bp-42a6658b-715a-4675-a522-4d6972131118	18.11.2013	10:49
...

It would seem mail heaven arrived with 24.1.1. I haven't been applying any patches.

Comment 37

[Mihai Sucan [:msucan]]

I still see this, or similar crash, in Thunderbird 27. I cant stay much in a virtual folder with filters - TB crashes after a few minutes. I recreated my entire profile - same issue.

It's also troubling that TB27 takes a lot of time doing nothing obvious, consuming 100% of my cpu, even in normal IMAP folders, every now and then (every 1-2 hours). Even if I dont use TB, it's just open.

Comment 38

[Mihai Sucan [:msucan]]

Just went in about:crashes. bp-5c788107-aa13-41a6-99fb-9898c2140202 -- looks like a JS engine-related crasher.

Comment 39

[:aceman]

So is there any crash with a signature belonging to this bug?

Comment 40

[:aceman]

Attachment: patch v2

I see nsCOMArray.RemoveObjectAt skips removing the object if index is out of bounds (just returns false that we do not check), I am not so sure with nsTArray.RemoveElementAt (there are some MOZ_ASSERTS so does it crash hard?). So maybe we can return earlier with a nicer error code.

Comment 41

[[e-mail]]

I don't suppose you would mind rebasing your patch on top of the one in bug 499995?

Comment 42

[:aceman]

Sure, if that one is checked in :)

Comment 43

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #39)
> So is there any crash with a signature belonging to this bug?

bug 662091?

(In reply to :aceman from comment #42)
> Sure, if that one is checked in :)

bug 499995 is not progressing, so perhaps we should proceed to check this in and get the benefits for TB31, and let bug 499995 do the tiny rebase

Comment 44

[:aceman]

Yes, looks like bug 499995 is nowhere near completion. Neil, can you do review in the current version of the patch here?

Comment 45

[[e-mail]]

(In reply to aceman from comment #44)
> Yes, looks like bug 499995 is nowhere near completion. Neil, can you do
> review in the current version of the patch here?

I gave the patch r+ almost 50 days ago, it should have been checked in.

Comment 46

[:aceman]

You didn't give r+ in this bug. If you mean bug 499995 then from the discussion it looks like the patch does not actually solve the bug. If the patch should still be checked in please say so in the bug.

Comment 47

[:aceman]

Attachment: patch v3

Updated after bug 499995.

Comment 48

[[e-mail]]

Comment on attachment 8404277 [details] [diff] [review]
patch v3

> NS_IMETHODIMP nsMsgGroupThread::RemoveChildAt(uint32_t aIndex)
> {
>+  NS_ENSURE_TRUE(aIndex < m_keys.Length(), NS_MSG_MESSAGE_NOT_FOUND);
...
> NS_IMETHODIMP nsMsgXFGroupThread::RemoveChildAt(uint32_t aIndex)
> {
>-  nsMsgGroupThread::RemoveChildAt(aIndex);
>+  NS_ENSURE_TRUE((int32_t)aIndex < m_folders.Count(), NS_MSG_MESSAGE_NOT_FOUND);
>+
>+  nsresult rv = nsMsgGroupThread::RemoveChildAt(aIndex);
>+  NS_ENSURE_SUCCESS(rv, rv);
So, you have a couple of choices here. You could do assume that m_keys and m_folders will be the same length, in which case you could rely on the test in nsMsgGroupThread. Or you could do an unsigned compare against m_folders.Length(). (GetNumChildren should really use Length() too.)

Comment 49

[:aceman]

Attachment: patch v4

Thanks, I didn't notice nsCOMArray has a .Length() too. Of course that one is a better match for uint32_t. As we are crashing due to unknown reasons I'll better not assume anything and make the checks robust.

Comment 50

[[e-mail]]

Comment on attachment 8404907 [details] [diff] [review]
patch v4

(Actually it occurs to me that RemoveObjectAt already bounds-checks. RemoveElementAt doesn't, of course, but it doesn't in nsTArray either.)

Comment 51

[:aceman]

Yes, as we do our own check now I changed removeObject to removeElement to not do the check twice.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/c8031029a680