Closed Bug 1312254 Opened 5 years ago Closed 5 years ago

Crash in nsImapMailFolder::NotifyMessageFlagsFromHdr

Categories

(MailNews Core :: Networking: IMAP, defect)

Unspecified
Linux
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 52.0

People

(Reporter: Sylvestre, Assigned: rkent)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-b14e1aa8-f06f-4b0c-b886-2cdc02161023.
=============================================================
sylvestre, is it reproducible for you?

#113 crash rank for TB45.4.0.
I'd guess related to bug 610472.

bp-b14e1aa8-f06f-4b0c-b886-2cdc02161023.
0 	libxul.so	nsImapMailFolder::NotifyMessageFlagsFromHdr	/builds/slave/tb-rel-c-beta-l64_bld-00000000/build/mailnews/imap/src/nsImapMailFolder.cpp:4895
1 	libxul.so	nsImapMailFolder::SyncFlags	/builds/slave/tb-rel-c-beta-l64_bld-00000000/build/mailnews/imap/src/nsImapMailFolder.cpp:4874
2 	libxul.so	nsImapMailFolder::UpdateImapMailboxInfo	/builds/slave/tb-rel-c-beta-l64_bld-00000000/build/mailnews/imap/src/nsImapMailFolder.cpp:2839
3 	libxul.so	SyncRunnable2<nsIImapMailFolderSink, nsIImapProtocol*, nsIMailboxSpec*>::Run	/builds/slave/tb-rel-c-beta-l64_bld-00000000/build/mailnews/imap/src/nsSyncRunnableHelpers.cpp:146
Component: Folder and Message Lists → Networking: IMAP
Depends on: 610472
Product: Thunderbird → MailNews Core
nope, happened twice for me but not occurring anymore :/
I would guess that a null check should be enough...
The fix is a little more complex than a null check, but this is now a fairly well understood issue. Basically, there is lots of code that rests the database for various reasons, so it is not safe to assume that the folder reference sticks around. We need to use a local reference.
Assignee: nobody → rkent
Status: NEW → ASSIGNED
Attachment #8804044 - Flags: review?(acelists)
(In reply to Kent James (:rkent) from comment #3)
> The fix is a little more complex than a null check, but this is now a fairly
> well understood issue. Basically, there is lots of code that rests the
> database for various reasons, so it is not safe to assume that the folder
> reference sticks around. We need to use a local reference.

"Rests the database"? Why do we need a local pointer to the same object? What is the difference in execution? Can mDatabase pointer get broken in parallel to execution of nsImapMailFolder::NotifyMessageFlagsFromHdr so that latter lines in it get a null mDatabase?
(In reply to :aceman from comment #4)
> (In reply to Kent James (:rkent) from comment #3)
> > The fix is a little more complex than a null check, but this is now a fairly
> > well understood issue. Basically, there is lots of code that rests the
> > database for various reasons, so it is not safe to assume that the folder
> > reference sticks around. We need to use a local reference.
> 
> "Rests the database"?

That was a typo, I meant "resets".

> Why do we need a local pointer to the same object?

There was another bug with this same issue, there were similar objections that it was not needed, but I did it anyway and the crash went away. The reality is that there are various existing paths that null the database, and we cannot reliably control when we hit those paths. They seem to get called in some unknown circumstances as a side effect of other calls, so at this point best practice is to use a local database variable reference.

> What is the difference in execution? Can mDatabase pointer get broken in
> parallel to execution of nsImapMailFolder::NotifyMessageFlagsFromHdr so that
> latter lines in it get a null mDatabase?

I believe yes.
Comment on attachment 8804044 [details] [diff] [review]
Use local database reference

Review of attachment 8804044 [details] [diff] [review]:
-----------------------------------------------------------------

So if we can't trust mDatabase (but there really run parallel treads that affect the same variable?), that seems to be a bug. Can we add some debug output to notice the cases? E.g. output a message (in debug build) at all places that set mDatabase to null. Then we would see which place nulls it right before a crash.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +4889,5 @@
>                                              nsMsgKey msgKey, uint32_t flags)
>  {
>    nsresult rv = GetDatabase();
>    NS_ENSURE_SUCCESS(rv, rv);
> +  nsCOMPtr<nsIMsgDatabase> database(mDatabase);

So can you at least add a comment that this is a hack (hopefully temporary) and reference this bug?
Attachment #8804044 - Flags: review?(acelists) → review+
"Can we add some debug output to notice the cases? E.g. output a message (in debug build) at all places that set mDatabase to null. Then we would see which place nulls it right before a crash."

I think this is a lost cause, the whole model of managing database lifetime is flawed and needs reworking. In the short run, the workaround is to do exactly what I did in this bug, namely that any time in a subclass of nsMsgDBFolder, when accessing the database after any kind of non-trivial method call, you really need to grab a local reference. We also now have the nsIMsgFolder attribute (which you and I added) databaseOpen that is useful in JS scripts in particular to test to decide if they should null the database after access. That is also a kludge.

"So can you at least add a comment that this is a hack (hopefully temporary) and reference this bug"

I'll add a comment, but I'm not going to call it (hopefully temporary) as it is now best practice until we totally rework the problem of database lifetime.
http://hg.mozilla.org/comm-central/rev/e73781b7dffe
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
You need to log in before you can comment on or make changes to this bug.