702165 - crash [@ nsMsgQuickSearchDBView::OnSearchDone ] - .. - [@ nsMsgSearchSession::InterruptSearch] when change folders before search is finished

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[NoOp]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:8.0) Gecko/20111110 Firefox/8.0 SeaMonkey/2.5
Build ID: 20111110143639

Steps to reproduce:

I have two folders set up to capture searches from an nntp list via gmane.org: 
gmane.comp.documentfoundation.libreoffice.user
The search searches the message body text for LibreOffice versions; 3.3. and 3.4. respectively. The resulting searchs are listed in my email folder as:
Documentfoundation
  - 3.3.4
  - 3.4.3



Actual results:

clickin on on of the folders (3.3.4 or 3.4.3) causes SeaMonkey to crash. However it is not consistent & doesn't happen every time.


Expected results:

Not crash.
Crash reports are located here:
<https://crash-stats.mozilla.com/report/index/bp-67259d78-f9bf-4e80-9c6d-364322111025>
<https://crash-stats.mozilla.com/report/index/bp-da5e1789-1e5f-43e2-8a56-9f5832111111>

Advise on the mozilla.dev.apps.seamonkey (Neil) is to include reference to:
https://bugzilla.mozilla.org/show_bug.cgi?id=545955
[(filterbar) quick search filtering outside of full searching (implement filter bar, separate quickfilter and global search)] as he believe the code in that bug report may be a contributing factor.

Comment 1

[NoOp]

CC'ing asuth (patch author) and :bienvenu (reviewer) per Neil's recommendation.

Comment 2

[H. Hofer]

I've got some of those under Windows, too (rare, but going on for quite a while):

<https://crash-stats.mozilla.com/report/index/bp-88b116a0-1293-45cf-a205-8f7222111022>
<https://crash-stats.mozilla.com/report/index/bp-3f118691-2dc8-4e41-af7a-015812110829>
<https://crash-stats.mozilla.com/report/index/bp-a288b26f-e4b9-446d-99b2-c1cbc2110718>

It usually happens, if I change folders while the search hasn't finished yet.

Comment 3

[NoOp]

Ah interesting. Thanks for that. If I recall that seems to have been what I had done as well (change folders while the search hasn't finished). I'll give it a try again today when I'm ready to close down SeaMonkey.

Comment 4

[NoOp]

That did the trick... changed folders while the search hadn't finished.
<https://crash-stats.mozilla.com/report/index/bp-51416062-c2f1-4173-ad93-275632111117>

Comment 5

[Wayne Mery (:wsmwk)]

bp-d4d3cb0a-ddbc-48d4-94ce-aa3652120217

Comment 6

[Tony Mechelynck [:tonymec]]

SeaMonkey (Linux64) bug 793447 has the same signature. Related?

Comment 7

[Kent James (:rkent)]

A call to nsMsgDBView::Close nulls m_db, yet the code at the crashing address assumes that m_db is valid, and crashes if not.

Seems pretty clear that all methods in the search views need to behave reasonably with null m_db. I think that the correct thing to do here, as I am starting to do in other places, is to acquire a local reference to the db from the folder and use that, else just return.

Comment 9

[Kent James (:rkent)]

Attachment: check m_db at the start

It seems to me that if the view has been closed so that m_db is null, there is no point in going through the bulk of this method, so we should just quit there.

I'm not sure if a Close() can happen while executing this method, but I don't think so, so I don't need to check m_db later at the crash point.

Comment 10

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 663926 [details] [diff] [review]
check m_db at the start

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

nsMsgDBView::Close() and nsMsgQuickSearchDBView::OnSearchDone() are both called on the UI thread, as far as I can tell, so we don't need to worry about synchronization.

Comment 11

[Kent James (:rkent)]

Comment on attachment 663926 [details] [diff] [review]
check m_db at the start

Checked in http://hg.mozilla.org/comm-central/rev/9448de242754

Comment 12

[Kent James (:rkent)]

Comment on attachment 663926 [details] [diff] [review]
check m_db at the start

This patch should land in tb-17 after baking in core.


User impact if declined: crash

Testing completed (on c-c, etc.): checked in, still needs baking

Risk to taking this patch (and alternatives if risky): None.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 663926 [details] [diff] [review]
check m_db at the start

[Triage Comment]
Yep, lets get this onto branches

Comment 14

[Mike Conley (:mconley) (:⚙️)]

comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/d271de33102d
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/7dbb40e04465

Comment 15

[NoOp]

Thanks for the fix. Can we please also ensure that this fix is included in SeaMonkey 14? Thanks.

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

(In reply to NoOp from comment #15)
> Thanks for the fix. Can we please also ensure that this fix is included in
> SeaMonkey 14? Thanks.

The fix landed on comm-beta after SM 2.14b3 was tagged, so it'll automatically be included in SM 2.14b4 (unless it's backed out before then).