crash in nsMsgLocalMailFolder::DeleteMessages

RESOLVED FIXED in Thunderbird 47.0

Status

defect
--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: wsmwk, Assigned: rkent)

Tracking

({crash})

Trunk
Thunderbird 47.0
x86
Windows NT
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird44 wontfix, thunderbird45 fixed, thunderbird46 fixed, thunderbird47 fixed, thunderbird_esr3844+ fixed)

Details

(crash signature)

Attachments

(1 attachment)

#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
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.
Assignee: nobody → rkent
Status: NEW → ASSIGNED
Attachment #8712282 - Flags: review?(acelists)
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?
Attachment #8712282 - Flags: review?(neil)
Attachment #8712282 - Flags: review?(acelists)
Attachment #8712282 - Flags: review?(Pidgeot18)
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 ;-)
Attachment #8712282 - Flags: review?(neil) → review+
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
Attachment #8712282 - Flags: review?(Pidgeot18)
Attachment #8712282 - Flags: approval-comm-esr38?
Attachment #8712282 - Flags: approval-comm-beta?
Attachment #8712282 - Flags: approval-comm-aurora?
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0
Version: 38 Branch → Trunk
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?
Attachment #8712282 - Flags: approval-comm-aurora? → approval-comm-aurora+
Would be nice if you could paste the landed changeset link in here ;-)
https://hg.mozilla.org/comm-central/rev/817c2d391be1
Comment on attachment 8712282 [details] [diff] [review]
Use a local reference for the db

https://hg.mozilla.org/releases/comm-esr38/rev/84ca41c78166
Attachment #8712282 - Flags: approval-comm-esr38?
Attachment #8712282 - Flags: approval-comm-esr38+
Attachment #8712282 - Flags: approval-comm-beta?
Attachment #8712282 - Flags: approval-comm-beta+
Attachment #8712282 - Flags: approval-comm-beta+ → approval-comm-esr45+
You need to log in before you can comment on or make changes to this bug.