1242925 - crash in nsMsgLocalMailFolder::DeleteMessages

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Wayne Mery (:wsmwk)]

#35 crash for 38.5.1

Typical comment is "moving lots of messages". However, most crashes moving many messages end up as OOM.

bp-bd6ce5d4-3c9c-406c-a148-ed4072160113.
 0 	xul.dll	nsMsgLocalMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, bool, bool, nsIMsgCopyServiceListener*, bool)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/local/src/nsLocalMailFolder.cpp:1228
1 	xul.dll	xul.dll@0x1a00793	
2 	xul.dll	nsMsgLocalMailFolder::EndMove(bool)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/local/src/nsLocalMailFolder.cpp:2638
3 	xul.dll	nsCopyMessageStreamListener::EndCopy(nsISupports*, nsresult)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/base/src/nsCopyMessageStreamListener.cpp:134
4 	xul.dll	nsCopyMessageStreamListener::OnStopRequest(nsIRequest*, nsISupports*, nsresult)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/base/src/nsCopyMessageStreamListener.cpp:143 

bp-aec19541-f94f-46db-afe4-167422160114 and bp-d9da919c-55a0-4e18-8478-c38b62160114 claims to only be viewing messages.

Mostly Windows crashes. Less than 1% other OS. For example bp-0a8528f0-30ec-42c4-a950-7d5bd2160121 bp-fcd6d373-f825-4cbe-88ff-ca9742160104

perhaps Bug 1229384 - crash in moz_abort | pages_commit - will be of help

Comment 1

[Kent James (:rkent)]

If I understand this correctly, the code is:

          if (mDatabase)
          {
            for (uint32_t i = 0; i < messageCount; ++i)
            {
              msgDBHdr = do_QueryElementAt(messages, i, &rv);
              rv = mDatabase->DeleteHeader(msgDBHdr, nullptr, false, true);
            }
          }

with the rv = mDatabase .... being the crash with null mDatabase. This could only happen if DeleteHeader has side effects that result in nulling mDatabase, or if there is some other thread that is nulling it (which is not supposed to happen). Neither seems likely, but here we are with a crash.

The simple thing to try, which I have suggested in similar cases, is to use a local variable for the database rather than rely on the class member. In this case the local variable already exists. I'll do a patch to use it.

Comment 2

[Kent James (:rkent)]

Attachment: Use a local reference for the db

Comment 3

[:aceman]

Comment on attachment 8712282 [details] [diff] [review]
Use a local reference for the db

Sorry, I do not follow this kind of hacks.
And doesn't it just paper over the side-effects causing mDatabase to become null?

Comment 4

[[e-mail]]

Comment on attachment 8712282 [details] [diff] [review]
Use a local reference for the db

No, I don't know how mDatabase can become null either. Hopefully if it's a problem it will just make crashes happen somewhere nearer the problem ;-)

Comment 5

[Kent James (:rkent)]

Comment on attachment 8712282 [details] [diff] [review]
Use a local reference for the db

[Approval Request Comment]
User impact if declined: Program crashes occasionally
Testing completed (on c-c, etc.): Still needed
Risk to taking this patch (and alternatives if risky): Low, just uses a different variable that already exists and has the correct value.

We should wait ideally a couple of days between pushes, but no reason not to push forward into aurora, beta, esr

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 8712282 [details] [diff] [review]
Use a local reference for the db

Sure. I'll land it when I have more than one patch, or is there a rush?

Comment 7

[Jorg K (CEST = GMT+2)]

Would be nice if you could paste the landed changeset link in here ;-)
https://hg.mozilla.org/comm-central/rev/817c2d391be1

Comment 8

[Jorg K (CEST = GMT+2)]

Aurora (TB 46):
https://hg.mozilla.org/releases/comm-aurora/rev/a97f08cd575e

Comment 9

[Kent James (:rkent)]

Comment on attachment 8712282 [details] [diff] [review]
Use a local reference for the db

http://hg.mozilla.org/releases/comm-esr45/rev/d963f0afdaf2

Comment 10

[Kent James (:rkent)]

Comment on attachment 8712282 [details] [diff] [review]
Use a local reference for the db

https://hg.mozilla.org/releases/comm-esr38/rev/84ca41c78166