438414 - crash when saved search is open after deleting a searched subfolder

Status: RESOLVED FIXED

(MailNews Core :: Search, defect)

Description

[Kent James (:rkent)]

This bug is reproducible on TB trunk from nightly download of 2008-06-08.

STR:

1) Create a subfolder to the inbox, say SUB1
2) Create a saved cross-folder search on the inbox, say with status is read, call it READXF
3) delete SUB1

Now randomly select different folders. About 5% of the time, when I select READXF, TB crashes.

In a debugger, the program is executing line 721 in nsMsgLocalSearch:

  m_scope->SetInputStream(null)

The error is "Unhandled exception ... Access violation reading location 0x00000209"

m_scope is defined, but its __vfptr is strange under nsISupports (0x000001f5)

I strongly suspect this is related to the issues of bug 378833 and friends. When the subfolder is removed, it remains in the definition of the saved search. VF listeners are attached for the non-existing folder.

Comment 1

[Kent James (:rkent)]

TB crashes regularly for me, so I attached a debugger to my main system (Windows XP). When it crashed, the symptoms were the same as this bug. That is, the system is crashing with m_scope->SetInputStream(null), where the scope term is set to a folder that has been deleted, and hence does not exist. I'm going to confirm this bug.

We need to do several things to make the code more robust against this error:

1) When a folder is moved or deleted, remove it from the scope of any XF virtual searches.

2) When adding search term listeners, make sure that the virtual folder exists.

3) Guard against a crash when working with a non-existing subfolder. 

Comment 2

[Kent James (:rkent)]

I suspect that the crash is happening because m_scope is implemented without reference counting, so we end up with a dangling pointer for some reason when the search scope folder does not actually exist. Scope term objects are being created without reference counting through code like:

  nsMsgSearchScopeTerm *pScopeTerm = new nsMsgSearchScopeTerm(this, scope, folder);

...

  m_scopeList.AppendElement(pScopeTerm);

But m_scope list is an nsVoidArray, that does not do reference counting. I'll take the bug.

Comment 3

[Kent James (:rkent)]

Attachment: Use reference conting for m_scope

I still want to review this more, but here is a patch that stops the crash, without generating any memory leaks in the process. Let me give more details of the problem.

First, updated STR. Follow steps 1-3 of comment 0. But to make the crash completely reliable, do this in addition:

4) Select the saved search folder.
5) Select the inbox
6) Do nothing for 10 seconds (this lets XPCOM garbage collect delete an nsMsgSearchSession object).
7) Select the saved search folder again. Crash occurs 100% of the time in my setup.

What is happening is this. A reference to nsMsgSearchScopeTerm exists in two places: in the array m_scopeList in nsMsgSearchSession, and also as the m_scope variable in nsMsgSearchAdapter. When garbage collection destroys the nsMsgSearchSession, it also destroy all elements in its m_scopeList, which leaves the m_scope dangling. Attempts to use that later then die.

To cure this, I changed the memory model of m_scopeList to rely on reference counting rather than directly deleting the underlying objects.

Comment 4

[David :Bienvenu]

We usually use interfaces with nsCOMArrays, not concrete classes, e.g., nsIMsgSearchScopeTerm instead of nsMsgSearchScopeTerm

Comment 5

[Kent James (:rkent)]

Attachment: Minimal code to cure crash

Let me abandon for now the effort to add reference counting, and just cure this problem directly within the existing memory management model.

This crash is only indirectly caused by the deleted folder in the virtual folder's search path. In this bug, I will only fix the crash. We still need to fix the fact that saved searches do not update when folders are moved.

Comment 6

[Kent James (:rkent)]

Attachment: Correct patch this time

Sorry, wrong patch before.

Comment 7

[David :Bienvenu]

Comment on attachment 333617 [details] [diff] [review]
Correct patch this time

looks good, thanks, Kent.

Comment 8

[Mark Banner (:standard8)]

Checked in: changeset id 96:458c052c75cb