165296 - Crash in TB09 [@ nsAddrDatabase::GetListRowByRowID ] importing an address book with a mailing list for the 2nd time.

Status: VERIFIED FIXED

(MailNews Core :: Import, defect)

Description

[[e-mail] (gone - use [e-mail] instead)]

Build ID: 2002-08-23, Windows 2000 (MOZILLA_1_0_BRANCH bits, need to check trunk
later).
Summary: Crash in [@ nsAddrDatabase::GetListRowByRowID ] importing an address
book with a mailing list for the 2nd time.

Here's how to reproduce: 

1. Import a pab.na2 that contains a mailing list from a 4.x profile into our
Address Book. 
2. Expand the mailing list, and hit delete.
3. Delete the address book entry.
4. Import the address book entry again and note that the list is expanded
underneath the address book entry (the twisty).
5. Select the mailing list and you will crash.

Expected Results:

No crash.

Actual Results:

nsAddrDatabase::GetListRowByRowID
[d:\builds\seamonkey\mozilla\mailnews\addrbook\src\nsAddrDatabase.cpp, line 3093]
nsListAddressEnumerator::nsListAddressEnumerator
[d:\builds\seamonkey\mozilla\mailnews\addrbook\src\nsAddrDatabase.cpp, line 2785]
nsAddrDatabase::EnumerateListAddresses
[d:\builds\seamonkey\mozilla\mailnews\addrbook\src\nsAddrDatabase.cpp, line 2919]
nsAbMDBDirectory::GetChildCards
[d:\builds\seamonkey\mozilla\mailnews\addrbook\src\nsAbMDBDirectory.cpp, line 448]
nsAbView::EnumerateCards
[d:\builds\seamonkey\mozilla\mailnews\addrbook\src\nsAbView.cpp, line 283]
nsAbView::Init [d:\builds\seamonkey\mozilla\mailnews\addrbook\src\nsAbView.cpp,
line 222]
XPTC_InvokeByIndex
[d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp,
line 106]
XPCWrappedNative::CallMethod
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp, line 2028]
XPC_WN_CallMethod
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednativejsops.cpp,
line 1267]
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 840]
js_Interpret [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 2792]
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 856]
js_InternalInvoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 931]
JS_CallFunctionValue [d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 3433]
nsJSContext::CallEventHandler
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp, line 1019]
nsJSEventListener::HandleEvent
[d:\builds\seamonkey\mozilla\dom\src\events\nsJSEventListener.cpp, line 182]
nsEventListenerManager::HandleEventSubType
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp, line
1273]
nsEventListenerManager::HandleEvent
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp, line
1872]
nsXULElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 3479]
nsTreeSelection::FireOnSelectHandler
[d:\builds\seamonkey\mozilla\layout\xul\base\src\tree\src\nsTreeSelection.cpp,
line 743]
nsTreeSelection::Select
[d:\builds\seamonkey\mozilla\layout\xul\base\src\tree\src\nsTreeSelection.cpp,
line 369]
XPTC_InvokeByIndex
[d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp,
line 106]
XPCWrappedNative::CallMethod
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp, line 2028]
XPC_WN_CallMethod
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednativejsops.cpp,
line 1267]
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 840]
js_Interpret [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 2792]
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 856]
js_InternalInvoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 931]
JS_CallFunctionValue [d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 3433]
nsJSContext::CallEventHandler
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp, line 1019]
nsJSEventListener::HandleEvent
[d:\builds\seamonkey\mozilla\dom\src\events\nsJSEventListener.cpp, line 182]
nsXBLPrototypeHandler::ExecuteHandler
[d:\builds\seamonkey\mozilla\content\xbl\src\nsXBLPrototypeHandler.cpp, line 448]
DoMouse [d:\builds\seamonkey\mozilla\content\xbl\src\nsXBLMouseHandler.cpp, line
118]
nsXBLMouseHandler::MouseDown
[d:\builds\seamonkey\mozilla\content\xbl\src\nsXBLMouseHandler.cpp, line 124]
nsEventListenerManager::HandleEvent
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp, line
1358]
nsXULElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 3479]
PresShell::HandleEventInternal
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6224]
PresShell::HandleEvent
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6142]
nsViewManager::HandleEvent
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 2081]
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 306]
nsViewManager::DispatchEvent
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 1892]
HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 83]
nsWindow::DispatchEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 1033]
nsWindow::DispatchWindowEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 1050]
nsWindow::DispatchMouseEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4939]
ChildWindow::DispatchMouseEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 5194]
nsWindow::ProcessMessage
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3765]
nsWindow::WindowProc
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 1295]
USER32.DLL + 0x1d0a (0x77e11d0a)
USER32.DLL + 0x1bc8 (0x77e11bc8)
USER32.DLL + 0x72b4 (0x77e172b4)
nsAppShellService::Run
[d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsAppShellService.cpp, line 458]
main1 [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1492]
main [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1839]
WinMain [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1857]
WinMainCRTStartup()
KERNEL32.DLL + 0x1ca90 (0x77e9ca90)

Comment 1

[Stephen Donner [:stephend] Not actively reading bugmail]

David, Scott: this bug also shows up using Thunderbird -- any chance you guys
have cycles to take a look at it?

See
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=nsAddrDatabase%3A%3AGetListRowByRowID&vendor=All&product=All&platform=All&buildid=&sdate=&stime=&edate=&etime=

Comment 2

[Jay Patel [:jay]]

Adding TB08 to summary for tracking since I see a few crashes in Thunderbird 0.8
Talkback data.  Not enough to call it a topcrash, but still something worth
looking into.

Comment 3

[Jay Patel [:jay]]

Updating summary with TB09 since I see a couple of incidents with Thunderbird
0.9.  Still not enough to mark this a topcrash, but just wanted to keep the bug
updated for tracking.

Comment 4

[timeless]

some code null checks that field, some doesn't. some constantly calls GetStore().

Comment 5

[Mark Banner (:standard8)]

I've just tried this on latest trunk but couldn't replicate it. Anyone got a way
of replicating it or an import file that would do it that I could try? Talkback
is getting crashes here still.

Comment 6

[Wayne Mery (:wsmwk)]

version 1.6a1 (20050905)

viewing a list (that shouldn't be there) on machine B - crash 

I deleted a list on machine A, exported the address book on machine A ldif
format, deleted the (approximately) same address book on machine B, imported the
book on machine B, the list was still visible in the book on machine B.  Clicked
on the list in machine B and crash.

I didn't restart TB or close (and reopen) the address book after I imported on
machine B.

haven't tried to reproduce problem.


TBID 9259733

Stack Signature	 nsAddrDatabase::GetListRowByRowID 55afa5b3
Product ID	ThunderbirdTrunk
Build ID	2005090605
Trigger Time	2005-09-11 19:28:26.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	thunderbird.exe + (00489da8)
URL visited	
User Comments	viewing a list (that shouldn't be there) on machine B - after I
deleted it on machine A, exported the address book, imported it on machine B
Since Last Crash	274008 sec
Total Uptime	274008 sec
Trigger Reason	Access violation
Source File, Line No.
e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/mailnews/addrbook/src/nsAddrDatabase.cpp,
line 3488
Stack Trace 	
nsAddrDatabase::GetListRowByRowID 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/mailnews/addrbook/src/nsAddrDatabase.cpp,
line 3488]
nsListAddressEnumerator::nsListAddressEnumerator 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/mailnews/addrbook/src/nsAddrDatabase.cpp,
line 3160]
nsAbMDBDirectory::GetChildCards 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/mailnews/addrbook/src/nsAbMDBDirectory.cpp,
line 428]
nsAbView::EnumerateCards 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/mailnews/addrbook/src/nsAbView.cpp,
line 295]
nsAbView::Init 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/mailnews/addrbook/src/nsAbView.cpp,
line 234]
XPTC_InvokeByIndex 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp,
line 102]
XPCWrappedNative::CallMethod 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,
line 2139]
XPC_WN_CallMethod 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1402]
js_Invoke 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/js/src/jsinterp.c,
line 1163]
js_Interpret 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/js/src/jsinterp.c,
line 3459]
js_Invoke 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/js/src/jsinterp.c,
line 1183]
js_InternalInvoke 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/js/src/jsinterp.c,
line 1260]
JS_CallFunctionValue 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/js/src/jsapi.c,
line 4016]
nsJSContext::CallEventHandler 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/dom/src/base/nsJSEnvironment.cpp,
line 1414]
nsJSEventListener::HandleEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/dom/src/events/nsJSEventListener.cpp,
line 185]
nsEventListenerManager::HandleEventSubType 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1685]
nsEventListenerManager::HandleEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1786]
nsXULElement::HandleDOMEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/content/xul/content/src/nsXULElement.cpp,
line 2201]
nsTreeSelection::FireOnSelectHandler 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/layout/xul/base/src/tree/src/nsTreeSelection.cpp,
line 789]
nsTreeSelection::Select 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/layout/xul/base/src/tree/src/nsTreeSelection.cpp,
line 378]
XPCWrappedNative::CallMethod 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,
line 2139]
XPC_WN_CallMethod 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1402]
js_Invoke 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/js/src/jsinterp.c,
line 1163]
js_Interpret 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/js/src/jsinterp.c,
line 3459]
js_Invoke 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/js/src/jsinterp.c,
line 1183]
js_InternalInvoke 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/js/src/jsinterp.c,
line 1260]
JS_CallFunctionValue 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/js/src/jsapi.c,
line 4016]
nsJSContext::CallEventHandler 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/dom/src/base/nsJSEnvironment.cpp,
line 1414]
nsJSEventListener::HandleEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/dom/src/events/nsJSEventListener.cpp,
line 185]
nsXBLPrototypeHandler::ExecuteHandler 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp,
line 502]
nsXBLEventHandler::HandleEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/content/xbl/src/nsXBLEventHandler.cpp,
line 86]
nsEventListenerManager::HandleEventSubType 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1685]
nsEventListenerManager::HandleEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1786]
nsXULElement::HandleDOMEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/content/xul/content/src/nsXULElement.cpp,
line 2201]
PresShell::HandleEventInternal 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6198]
PresShell::HandleEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6034]
nsViewManager::HandleEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/view/src/nsViewManager.cpp,
line 2553]
nsViewManager::DispatchEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/view/src/nsViewManager.cpp,
line 2245]
HandleEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/view/src/nsView.cpp,
line 174]
nsWindow::DispatchEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1060]
nsWindow::DispatchMouseEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 5803]
ChildWindow::DispatchMouseEvent 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 6054]
nsWindow::WindowProc 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1249]
USER32.dll + 0x8734 (0x77d48734)
USER32.dll + 0x8816 (0x77d48816)
USER32.dll + 0x89cd (0x77d489cd)
USER32.dll + 0x8a10 (0x77d48a10)
nsAppShell::Run 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/widget/src/windows/nsAppShell.cpp,
line 159]
nsAppStartup::Run 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp,
line 146]
main 
[e:/builds/tinderbox/thunderbird-trunk/WINNT_5.0_Depend/mozilla/mail/app/nsMailApp.cpp,
line 62]
kernel32.dll + 0x16d4f (0x7c816d4f)

Comment 7

[Mark Banner (:standard8)]

I have a patch in progress for this - adds lots of checking to nsAddrDatabase.cpp.

Comment 8

[Mark Banner (:standard8)]

Attachment: Check for null pointers (diff -w)

This patch adds lots of checks for null pointers - so at least we won't crash and hopefully we can pick up what is going wrong at some stage.

I've also removed all the calls to GetStore, and the ones to GetEnv that are called from within the class, this makes it clearer as to what is actually being used for future editing.

I have also done a lot of whitespace cleanup, hence the -w, I'll add the normal one in a mo.

Comment 9

[Mark Banner (:standard8)]

Attachment: Check for null pointers

Comment 10

[Mark Banner (:standard8)]

Based on talkback reports, this affects all our main platforms -> Hardware/OS = All/All

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 202728 [details] [diff] [review]
Check for null pointers (diff -w)

rs=dmose; these seem like good cleanups even though I haven't eyeballed every single line :-)

Comment 12

[Mark Banner (:standard8)]

Patch checked in.

Hopefully this has solved the crasher, so we'll have to keep a look out for any adverse affects. I'll let this run on trunk for a while before deciding whether or not to get it on branch.

/cvsroot/mozilla/mailnews/addrbook/src/nsAddrDatabase.h,v  <--  nsAddrDatabase.h
new revision: 1.59; previous revision: 1.58
done
Checking in mailnews/addrbook/src/nsAddrDatabase.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsAddrDatabase.cpp,v  <--  nsAddrDatabase.cpp
new revision: 1.138; previous revision: 1.137

Comment 13

[Stephen Donner [:stephend] Not actively reading bugmail]

0 crashes found where the Stack Signature contains 'nsaddrdatabase::getlistrowbyrowid' and the Deployment ID looks like 'MozillaOrgThunderbirdTrunk%%'

0 crashes found where the Stack Signature contains 'nsaddrdatabase::getlistrowbyrowid' and the Deployment ID looks like 'MozillaOrgMozillaTrunk%%'

Verified FIXED; this is certainly gone on trunk.

Comment 14

[timeless]

*** Bug 352146 has been marked as a duplicate of this bug. ***

Comment 15

[Ian Neal]

Ping to Mark, still thinking about this for branch?

Comment 16

[Mark Banner (:standard8)]

(In reply to comment #15)
> Ping to Mark, still thinking about this for branch?
> 
Hmm, forgotten about that comment. If someone can check it will apply cleanly to branch then I will be happy for it to go across.

Comment 17

[timeless]

*** Bug 344197 has been marked as a duplicate of this bug. ***

Comment 18

[Ian Neal]

Attachment: Branch patch (diff -w)

Branch equivalent patch (diff -w)

Comment 19

[Ian Neal]

Attachment: Branch patch

Branch equivalent patch that would be used for check in.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 239735 [details] [diff] [review]
Branch patch

I'm fairly sure this is miss-merged:

@@ -3136,16 +3169,17 @@ protected:
 
 nsAddrDBEnumerator::nsAddrDBEnumerator(nsAddrDatabase* db)
     : mDB(db), mRowCursor(nsnull), mCurrentRow(nsnull), mDone(PR_FALSE)
 {
     mDbTable = mDB->GetPabTable();
     mCurrentRowIsList = PR_FALSE;
 }
 
+  NS_ENSURE_ARG_POINTER(aResult);
 nsAddrDBEnumerator::~nsAddrDBEnumerator()
 {
   NS_IF_RELEASE(mRowCursor);
 }

and several instances below it. 

The nsAddrDBEnumerator changed from being dervied from nsIEnumerator to nsISimpleEnumerator on trunk after the branch.

Comment 21

[Mark Banner (:standard8)]

(In reply to comment #20)
> The nsAddrDBEnumerator changed from being dervied from nsIEnumerator to
> nsISimpleEnumerator on trunk after the branch.
> 
Ian, Actually, now I have a quick look at it again, I'm not fussed if we drop the changes to ns*Enumerator for branch. That was full bullet proofing and shouldn't really be required to stop the crashes. Thanks for looking at the patch for me.

Comment 22

[Mark Banner (:standard8)]

Requesting blocking thunderbird 2.0. This crasher appears not just to happen with import but we've seen it in other cases as well (e.g. bug 352146) - and talkback suggests it happens quite a bit.

We should have the branch version of the patch very soon now in any case, but I wanted to get it on the blocking radar.

Comment 23

[Scott MacGregor]

Yeah I think seamonkey and tb will want this for the branch once Iann merges the patch. 

Comment 24

[Ian Neal]

Attachment: Branch patch v0.1

This patch:
* Same as for trunk but without Enumerator changes

Few notes:
Someone needs to test against all the different ways of crashing it is supposed to fix.
AddRowValue is in branch but not trunk but isn't used anyway so not changed.
There are still a few GetEnv() but they also exist in trunk so that would be a separate bug if need to get rid of them.

Comment 25

[Mark Banner (:standard8)]

(In reply to comment #24)
> Created an attachment (id=240380) [edit]
> Someone needs to test against all the different ways of crashing it is supposed

Unfortunately I don't think there are any specific test cases for this that are consistently reproduceable. However I'll look at the patch later and give it a good thrashing.

Comment 26

[Mark Banner (:standard8)]

Comment on attachment 240380 [details] [diff] [review]
Branch patch v0.1

Although AddRowValue isn't used, there is still a GetStore() call in there that causes compilation failures as you've removed GetStore() in the header.

Fix that to m_mdbStore and r=me.

I've compared this patch with the trunk patch and I'm happy with the differences.

Given we haven't seen problems in this area on trunk after this patch, I think I'm happy with putting it into 1.8.1 branch. I'm going to run it on my standard build between now and the releases, so I should pick up if there are any problems.

I'm assuming you'll fix that minor compilation problem for the patch later, so I'll push this forward and request sr now so we can hopefully get it in as soon as possible.

Comment 27

[David :Bienvenu]

Comment on attachment 240380 [details] [diff] [review]
Branch patch v0.1

sr=bienvenu, if you fix the the compile error.

Comment 28

[Ian Neal]

Attachment: Fixed compile branch patch v0.1a (Checked into 1.8 branch)

Changes since v0.1:
* Fixed compile problem (I'd just got rid AddRowValue locally).

Carrying forward r= and sr=, requesting a= for TB 2

Comment 29

[Ian Neal]

Comment on attachment 240671 [details] [diff] [review]
Fixed compile branch patch v0.1a (Checked into 1.8 branch)

Checking in (1.8 branch)
nsAddrDatabase.cpp;
new revision: 1.131.2.2; previous revision: 1.131.2.1
nsAddrDatabase.h;
new revision: 1.53.2.2; previous revision: 1.53.2.1
done

Comment 30

[Mark Banner (:standard8)]

*** Bug 355336 has been marked as a duplicate of this bug. ***

Comment 31

[Mark Banner (:standard8)]

I'm currently looking at getting the 1.8 branch patch onto 1.8.0.x branch as well. The patch applies cleanly, but I'll need to test it for a couple of days before requesting approvals.

Comment 32

[Mark Banner (:standard8)]

(In reply to comment #31)
> I'm currently looking at getting the 1.8 branch patch onto 1.8.0.x branch as
> well. The patch applies cleanly, but I'll need to test it for a couple of days
> before requesting approvals.

The patch applies to the 1.8.0.x branch, but unfortunately I have other build errors. I'm not going to be able to look at it until next week. I think the next 1.8.0.x release will be around ff2 so there's time yet, but if anyone wants to test the patch out for me that'd be useful.

Comment 33

[Mark Banner (:standard8)]

Comment on attachment 240671 [details] [diff] [review]
Fixed compile branch patch v0.1a (Checked into 1.8 branch)

Requesting approval for 1.8.0.9 branch. This patch fixes a crash that is currently ranked 9 on the talkback crash list for Thunderbird 1.5.0.7.

This has been on trunk and 1.8.x branch for quite a while with no known regressions and no re-occurrence of the crash.

Comment 34

[Mark Banner (:standard8)]

Requesting blocking 1.8.0.9 for this topcrash bug as mozilla.dev.planning suggests I should do. The patch is ready to go, it has been on trunk and 1.8 branch for a while now with no known regressions. See also comment 33.

Comment 35

[Daniel Veditz [:dveditz]]

mscott:
It's an ancient bug, and people can upgrade to get the fix so it's not a blocker. Should we take the patch anyway? It's big, and the gain seems small: nsAddrDatabase::GetListRowByRowID is the #40 crash, not #9

Comment 36

[Mark Banner (:standard8)]

(In reply to comment #35)
> mscott:
> It's an ancient bug, and people can upgrade to get the fix so it's not a
> blocker. Should we take the patch anyway? It's big, and the gain seems small:
> nsAddrDatabase::GetListRowByRowID is the #40 crash, not #9
> 

Despite its title (and yes, my mistake) I believe this will fix all nsAddrDatabase* crashes in the current 1.5.0.7 list (nsAddrDatabase::GetRowFromAttribute has moved up to #7). If you compare 1.5.0.7 with the trunk & branch there are no nsAddrDatabase* crashes.

Comment 37

[Mabry Tyson]

(In reply to comment #36)

> Despite its title (and yes, my mistake) I believe this will fix all
> nsAddrDatabase* crashes in the current 1.5.0.7 list
> (nsAddrDatabase::GetRowFromAttribute has moved up to #7). If you compare
> 1.5.0.7 with the trunk & branch there are no nsAddrDatabase* crashes.
> 

I'm one of those with nsAddrDatabase::GetRowFromAttribute crashes. (Talkback TB26459596Y for instance.)  As with Bug 352146 (marked duplicate of this bug), TB died every time I tried to open a message. (I would actually see the message just before it died.)

Since it will be a while before 1.8 is released, I'll say how I relieved the problem for me, as it might help others to work around it.  

I had been using Mozilla mail, but I wanted to at least be able to use Thunderbird.  I ran 1.5.0.7 (which I presume grabbed stuff from Mozilla). As I couldn't use it at all because it crashed on any message, any folder, I gave up until 1.5.0.8.  

When the newer version also failed, I tried older versions 1.5.0.5, 1.5.0.2, 1.5, all of which failed.  Then I tried 1.0.8 and Thunderbird could display a message.  Then I went back to 1.5.0.8 and it worked ok.

I would guess that the conversion from Mozilla mail left something in my address book that would trigger this bug.  But 1.0.8 avoided the bug somehow and wrote the address book in a form that works for 1.5.0.8.

Comment 38

[David :Bienvenu]

this is a big patch, but I bet the actual fix is much smaller - this patch has a lot of other cleanup and whitespace changes. Mark, any chance of either identifying the actual crash fix, or creating a smaller patch?

Comment 39

[Daniel Veditz [:dveditz]]

Comment on attachment 240671 [details] [diff] [review]
Fixed compile branch patch v0.1a (Checked into 1.8 branch)

This patch is too big to consider for the 1.8.0.x security fix branch. If you can shrink it to the actual fix and remove the cleanup changes it will be easier to evaluate.

Comment 40

[Daniel Veditz [:dveditz]]

We probably do want to fix this crash on the old branch, sticking on the "wanted" list so we don't forget about it.

Comment 41

[Mark Banner (:standard8)]

Attachment: Reduced patch for 1.8.0.x branch

I've reduced this down to what I consider the bare minimum. The actual crash may be fixed by just one or two instances of the checks, but then I can't be sure which ones and if only checking in one or two instances would not cause other crashes.

I've still got to compile & test this before its ready for reviews.

Comment 42

[Mark Banner (:standard8)]

Attachment: Reduced patch for 1.8.0.x branch v2

Revised patch for 1.8.0.x branch - this one compiles and seems to work ok.

Comment 43

[David :Bienvenu]

Comment on attachment 250764 [details] [diff] [review]
Reduced patch for 1.8.0.x branch v2

this looks ok, thx, Mark.

Ideally, we should make it impossible to have an nsAddrDatbase with null m_mdbEnv...I think I did that for nsMsgDatabase but I could be wrong...

Comment 44

[Mark Banner (:standard8)]

Comment on attachment 250764 [details] [diff] [review]
Reduced patch for 1.8.0.x branch v2

This is a reduced version of the patch that has gone onto the trunk & 1.8 branch.

Comment 45

[Jay Patel [:jay]]

Comment on attachment 250764 [details] [diff] [review]
Reduced patch for 1.8.0.x branch v2

Approved for the 1.8.0 branch, a=jay for drivers.

Comment 46

[Mark Banner (:standard8)]

(In reply to comment #45)
> (From update of attachment 250764 [details] [diff] [review])
> Approved for the 1.8.0 branch, a=jay for drivers.
> 

checked in.

Comment 47

[Jay Patel [:jay]]

Does anyone have a testcase or a sample addressbook file QA can use to verify this bug?

If anyone on this bug was seeing this problem, can you try again with a recent nightly on the 1.8.0 branch and verify this fix?  Please replace the "fixed1.8.0.10" keyword with "verified1.8.0.10" if you can confirm that this crash is gone.  Thanks!

A quick look at Talkback data doesn't show any recent crashes (search for all crashes with the stack signature in this bug for 2007 and got 0 crashes):
http://talkback-public.mozilla.org/search/start.jsp?search=1&searchby=stacksig&match=contains&searchfor=nsAddrDatabase%3A%3AGetListRowByRowID&vendor=MozillaOrg&product=All&platform=All&buildid=2007&sdate=&stime=&edate=&etime=&sortby=bbid&rlimit=500

Marking v.fixed based on recent Talkback data.  No crashes found in 2.0.0.1 Talkback data either.  We should confirm with 1.5.0.10 Talkback data when it starts coming in.