439475 - Crash [@ConvertToCard] when deleting card/mailing list from Mac OS X Address Book and Thunderbird Address Book is open

Status: VERIFIED FIXED

(MailNews Core :: Address Book, defect, P1)

Description

[Mark Banner (:standard8)]

I had the Thunderbird address book open, and the Mac OS X Address Book. I deleted a mailing list via the Mac OS X Address Book and Thunderbird crashed:

2008-06-16 19:28:02.333 thunderbird-bin[20927:10b] *** -[NSCFString uniqueId]: unrecognized selector sent to instance 0x1cc51f10
2008-06-16 19:28:02.335 thunderbird-bin[20927:10b] Mozilla has caught an Obj-C exception [NSInvalidArgumentException: *** -[NSCFString uniqueId]: unrecognized selector sent to instance 0x1cc51f10]
2008-06-16 19:28:02.335 thunderbird-bin[20927:10b] Generating stack trace for Obj-C exception...
2008-06-16 19:28:10.206 thunderbird-bin[20927:10b] Stack trace:
Looking up symbols in process 20927 named:  thunderbird-bin
__raiseError (in CoreFoundation)
objc_exception_throw (in libobjc.A.dylib)
-[NSObject doesNotRecognizeSelector:] (in CoreFoundation)
___forwarding___ (in CoreFoundation)
_CF_forwarding_prep_0 (in CoreFoundation)
ConvertToCard(nsIRDFService*, ABRecord*, nsIAbCard**) (in libmail.dylib) (nsAbOSXDirectory.mm:92)
-[ABChangedMonitor ABChanged:] (in libmail.dylib) (nsAbOSXDirectory.mm:223)
_nsnote_callback (in Foundation)
__CFXNotificationPost (in CoreFoundation)
_CFXNotificationPostNotification (in CoreFoundation)
-[NSNotificationCenter postNotification:] (in Foundation)
__NSThreadPerformPerform (in Foundation)
CFRunLoopRunSpecific (in CoreFoundation)
CFRunLoopRunInMode (in CoreFoundation)
RunCurrentEventLoopInMode (in HIToolbox)
ReceiveNextEventCommon (in HIToolbox)
BlockUntilNextEventMatchingListInMode (in HIToolbox)
_DPSNextEvent (in AppKit)
-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit)
-[NSApplication run] (in AppKit)
nsAppShell::Run() (in libwidget_mac.dylib) (nsAppShell.mm:588)
nsAppStartup::Run() (in libtoolkitcomps.dylib) (nsAppStartup.cpp:181)
XRE_main (in XUL) (nsAppRunner.cpp:3170)
main (in thunderbird-bin) (nsMailApp.cpp:103)

Comment 1

[Mark Banner (:standard8)]

hwaara tells me this happens when deleting cards ormailing lists from the Mac OS X Address book (not just mailing lists) with TB open.

Comment 2

[Håkan Waara]

Attachment: Patch v1

Basically the problem is that when we get delete notifications, we get a list of UIDs of address book "things" that have changed, but we do not know the *types* of these "things" (groups / persons?). We also cannot use the UID to look that up in the system address book, because the items are already gone. Our current code got null back for that query, and was lucky that it didn't crash earlier using that null pointer. (Luckily you can send messages to null in objc, but it always returns 0 back, so if-statements will be messed up unless written with care.)

The solution:

* Maybe there's a better solution here - but this seems to work well enough... I couldn't find a simple way to look up if a resource exists for a specified UID, so what I did was this: When I get a UID for a delete notification, I just try use that UID up as a card resource, and if that fails, try to do it for a group resource. Since I get a bad return value from the rdf-service if the thing doesn't exist, this seems to make things work, and fixes the crashes. I get the assertions below though. Let me know if you know of a better way to do it, but otherwise it seems pretty harmless in our case.

-1608249440[609d20]: WARNING: NS_ENSURE_TRUE(card) failed: file /Users/hakan/Programmering/mozilla/thunderbird-work1/mozilla/mailnews/addrbook/src/nsAbOSXCard.mm, line 193
WARNING: NS_ENSURE_TRUE(card) failed: file /Users/hakan/Programmering/mozilla/thunderbird-work1/mozilla/mailnews/addrbook/src/nsAbOSXCard.mm, line 193
-1608249440[609d20]: ###!!! ASSERTION: unable to initialize resource: 'Error', file /Users/hakan/Programmering/mozilla/thunderbird-work1/mozilla/rdf/base/src/nsRDFService.cpp, line 1030
###!!! ASSERTION: unable to initialize resource: 'Error', file /Users/hakan/Programmering/mozilla/thunderbird-work1/mozilla/rdf/base/src/nsRDFService.cpp, line 1030

* I also clarified the name of these ConvertToGroup/ConvertToCard functions.
* Also some other really minor touch ups.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 325342 [details] [diff] [review]
Patch v1

So this patch seems to break mailing list update (add/rename) notifications, i.e. add a list in the OS X address book and it no longer adds itself to the results pane of the address book display.

If I understand what you're saying correctly, then we get notifications to a directory but the directory doesn't know what it contains, so doesn't update the right information (and does the wrong thing). I think I may be able to get around this and fix some other bugs at the same time. Let me have a play...

Comment 4

[Mark Banner (:standard8)]

Attachment: Better Option (WIP)

I think this is a better version. What I'm doing here is to use the lists that we already have in nsAbOSXDirectory and searching through them for the relevant entry, once we have that entry we are then able to go ahead and remove it from our UI.

This is currently working fine for mailing lists/groups, cards also work, although I'm getting various errors on the console:

###!!! ASSERTION: op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0: 'op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0', file pldhash.c, line 585
###!!! ASSERTION: PL_DHASH_ENTRY_IS_LIVE(entry): 'PL_DHASH_ENTRY_IS_LIVE(entry)', file pldhash.c, line 691
Going onto the next
Going onto the next
WARNING: resource was never registered: file /Users/moztest/mozilla/hg/mozilla/rdf/base/src/nsRDFService.cpp, line 1307
thunderbird-bin(43425,0xa023cfa0) malloc: *** error for object 0xb5b400: double free
*** set a breakpoint in malloc_error_break to debug

I've not had time to debug these yet.

Comment 5

[Håkan Waara]

Comment on attachment 325959 [details] [diff] [review]
Better Option (WIP)


>+  // See if this item is in our address list
>+  PRUint32 i, addressCount;
>+  rv = m_AddressList->GetLength(&addressCount);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCAutoString uri(NS_ABOSXDIRECTORY_URI_PREFIX);
>+  uri.Append(aUid);
>+
>+  for (i = 0; i < addressCount; ++i)
>+  {
>+    nsCOMPtr<nsIAbDirectory> directory(do_QueryElementAt(m_AddressList, i, &rv));
>+    if (NS_SUCCEEDED(rv))
>+    {
>+      nsCString dirURI;
>+      directory->GetURI(dirURI);
>+      if (dirURI == uri)
>+        // Match found, do the necessary and get out of here.
>+        return UnassertDirectory(abManager, directory);
>+    }
>+  }

I haven't looked closely at this case, but removing things from m_AddressList while enumerating it looks dangerous (because its length will change). If you want to do it, it's safer to do in reverse:

while (addressCount--) {
  // remove, or not
}

However, if you only ever remove one thing, then maybe this is no issue? But if someone reuses the code, it may become one...

Comment 6

[Mark Banner (:standard8)]

Attachment: The fix

This fixes the crash problem completely (in the last patch I was removing items from the hash twice, not a good idea).

This patch fixes the crash and correctly updates the address book data, it doesn't quite get the display updates right (it tends to leave lists in the results view), but I think that's really a different problem which I'll look at in bug 437903 and/or bug 439470.

Comment 7

[David :Bienvenu]

Comment on attachment 326871 [details] [diff] [review]
The fix

I don't know if it makes sense to put an nsCAutoString in a struct, even if the struct is created on the stack:

+struct nsRemoveSingleItemEnumeratorData
+{
+  nsIAbDirectory *mDirectory;
+  nsIAbManager *mManager;
+  nsCAutoString mURI;

+  NS_ENSURE_ARG_POINTER(m_AddressList);

technically, m_addressList is not an arg - mabye NS_ENSURE_TRUE(...,NS_ERROR_NULL_POINTER); ?

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #7)
> (From update of attachment 326871 [details] [diff] [review])
> I don't know if it makes sense to put an nsCAutoString in a struct, even if the
> struct is created on the stack:
> 
> +struct nsRemoveSingleItemEnumeratorData
> +{
> +  nsIAbDirectory *mDirectory;
> +  nsIAbManager *mManager;
> +  nsCAutoString mURI;

Fixed on checkin, I changed it to an nsCString.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with version 3.0a2pre (2008062803)