652855 - De-RDF the address book

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

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

The address book currently uses the RDF service to look up and instantiate different address book types.

We want to eliminate our need for the RDF service within the address book, and replace it with a simpler look-up table managed by AbManager.

Comment 1

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

Attachment: Work-in-progress patch - v1

This is my first WIP patch for de-RDFing the address book.

I took a wrecking ball to this thing, and I guess I just was hoping for another pair of eyes to make sure I didn't wipe anything critical out.

Comment 2

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

I should also point out:

1)  All address book xpcshell tests are passing
2)  I haven't de-RDF'd any of the Outlook or OSX address book code, simply because I'm yet to set up a Windows / Mac machine to compile and test that stuff

Comment 3

[David :Bienvenu]

cool - I've looked through some of this. It occurs to me that perhaps CreateDirectory wants to take a URI, which might get rid of some of the need for Init().

I might have a whack at the Outlook stuff because I'd need that fixed in order to run the patch. If I get a chance to start on that, I'll let you know.

Comment 4

[David :Bienvenu]

Comment on attachment 528904 [details] [diff] [review]
Work-in-progress patch - v1

this patch didn't come close to applying - is it against the comm-central trunk?

Comment 5

[Joshua Cranmer [:jcranmer]]

If I may add a few comments:
1. As far as I can tell, nsIAbDirFactory::createDirectory is essentially a wrapper around Cc.createInstance(<magic instance>)? If there is no particular reason for anyone to implement it differently, I would just recommend that you replace it with the createInstance directly.

2. I'm concerned about the nsAddrDatabase::AddListNode implementation. The original code made the rdfService a proxied instance, so you should probably also set up a proxied service call to nsIAbManager, especially because nsInterfaceHashtable is not thread-safe.

Comment 6

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

Attachment: Work-in-progress patch-v2

Whoops - looks like I forgot to merge upstream comm-central.  Sorry about that.

Comment 7

[David :Bienvenu]

thx, the patch applies now.  I'm having a bit of fun with nsAbOutlookDirFactory::CreateDirectory because CreateDirectory conflicts with some built-in Windows function/define, so I have to remember how to work around that...

Comment 8

[David :Bienvenu]

Attachment: get prev patch to build on windows

Mike, you'll want to incorporate the changes to the outlook source files into your patch going forward. I haven't tested that they work, but they do build. I'll look at OS/X next.

Comment 9

[David :Bienvenu]

Attachment: get abmanager outside loop

actually, this is better, because it gets the abmanager outside the for loop.

Comment 10

[David :Bienvenu]

ah, sorry, libxul doesn't link w/ this patch - update coming, sorry for the bugspam.

Comment 11

[David :Bienvenu]

Attachment: fix link issues

ok, this should build on windows.

Comment 12

[David :Bienvenu]

Comment on attachment 529141 [details] [diff] [review]
Work-in-progress patch-v2

You should get abManager from MailServices. I know this patch is going to evolve some more, but marking feedback+.

Comment 13

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

Attachment: Work-in-progress patch-v3 - backend changes

These are the backend changes for de-RDFing the address book.  These changes also integrate bienvenu's work with the Outlook address book type.

Comment 14

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

Attachment: Work-in-progress patch-v3 - Javascript changes

These are the front-end Javascript changes for de-RDFing the address book.

Comment 15

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

Attachment: Work-in-progress patch-v3 - backend changes

Sorry for the bugspam - I'm breaking this patch down even further...

Comment 16

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

Attachment: Work-in-progress patch-v3 - other changes

This patch holds the simpler changes to the backend.

Comment 17

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

Attachment: Work-in-progress patch-v4 - other changes

I forgot the bump the uuid on nsIAbDirectory.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 529724 [details] [diff] [review]
Work-in-progress patch-v3 - Javascript changes

>+let Cu = Components.utils;
>+

Please drop this in both files. We don't define these globally as it actually causes namespace issues, and no-one has resolved them yet.

>diff -r a3de94be0dcc mailnews/addrbook/prefs/content/pref-directory-add.js
>+let Cu = Components.utils;

as above

>+Cu.import("resource:///gre/modules/mailServices.js");

This shouldn't have the gre in it as it breaks adding an LDAP dir.

Comment 19

[Mark Banner (:standard8)]

Attachment: Work-in-progress patch v3 - basic c++ changes

Not quite what I meant with the C++ split. This patch is more what I meant - just a few of the RDF -> AB manager call changes, and some of the include-only changes.

Comment 20

[Mark Banner (:standard8)]

Attachment: Work-in-progress patch v3 - backend changes

and this is the rest of the changes that aren't in the basic c++ one.

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 529758 [details] [diff] [review]
Work-in-progress patch v3 - basic c++ changes

>diff --git a/mailnews/addrbook/src/nsAbCardProperty.cpp b/mailnews/addrbook/src/nsAbCardProperty.cpp
>+    nsCOMPtr <nsIAbDirectory> mailList = nsnull;

nit: No need to initialise to nsnull, as nsCOMPtr does that for us.
nit: No space in-between nsCOMPtr and <

The rest of this seems to compile on Mac at least, so r=Standard8 with those fixed.

Comment 22

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

Attachment: Patch-v4 - Javascript changes

Thanks for the review, Standard8.  I've implemented your changes.

Comment 23

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

Attachment: [checked in] Basic c++ groundwork changes for de-RDFing address book (r+'d by Standard8)

Fixed those tiny nits that Standard8 found - thanks for the review!

Comment 24

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

Attachment: [checked in] Patch-v5 - Javascript changes

I accidentally based this patch off of trunk, when it relies on bug 422845.

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 529815 [details] [diff] [review]
[checked in] Patch-v5 - Javascript changes

Looks good. r=Standard8

Comment 26

[Mark Banner (:standard8)]

Comment on attachment 529786 [details] [diff] [review]
[checked in] Basic c++ groundwork changes for de-RDFing address book (r+'d by Standard8)

I checked this in with the js changes merged as one.

Comment 27

[Mark Banner (:standard8)]

Comment on attachment 529815 [details] [diff] [review]
[checked in] Patch-v5 - Javascript changes

Checked in: http://hg.mozilla.org/comm-central/rev/548d10bdab86

Comment 28

[Jens Hatlak (:InvisibleSmiley)]

Should I create a new bug for making the changes to the suite/ equivalent of abCommon.js, add the patch here (already have it locally) or leave it to you guys?

Comment 29

[Mark Banner (:standard8)]

(In reply to comment #28)
> Should I create a new bug for making the changes to the suite/ equivalent of
> abCommon.js, add the patch here (already have it locally) or leave it to you
> guys?

I'd suggest making the suite/ port separate - there's already a lot going on here.

Comment 30

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

Attachment: Work-in-progress patch-v4 - all address books de-RDF'd.

This work-in-progress patch de-RDF's the Mork, LDAP, OSX and Outlook address books.

I'm going to clean it up some on Monday and have a patch up for review soon after.

Comment 31

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

Attachment: De-RDF Patch - V1

Ok, this is it.  Fire away.

Comment 32

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

Comment on attachment 537834 [details] [diff] [review]
De-RDF Patch - V1

Whoops, put down the wrong reviewer.

Comment 33

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

I should also point out that my EDS Contacts integration work I've been doing relies on this patch, and so this needs to land in TB 7.0 if we're going to make the Ubuntu O release on time.

No pressure. :)

Comment 34

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

Comment on attachment 537834 [details] [diff] [review]
De-RDF Patch - V1

jcranmer mentioned that I'll probably need superreview for this - I'll assign bienvenu for now, but I'm open to suggestions.

Comment 35

[Joshua Cranmer [:jcranmer]]

Comment on attachment 537834 [details] [diff] [review]
De-RDF Patch - V1

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

Whee, my first review comment got swallowed :-( .

I'll start my review with some nits I've found from browsing source; I'll save a more thorough review of stuff for a bit later.

::: mailnews/addrbook/public/nsAbBaseCID.h
@@ +51,5 @@
>  //
> +// The start of the contract ID for address book directory types
> +//
> +#define NS_AB_DIRECTORY_TYPE_CONTRACTID_PREFIX \
> +  "@mozilla.org/addressbook/directory;1?type="

Since I presume this is being required as a contract-ID for directories, it would probably help to note this in the documentation for nsIAbDirectory.

@@ +66,4 @@
>  #define NS_ABMANAGER_CID \
>  { /* {ad81b321-8a8a-42ca-a508-fe659de84586} */ \
>    0xad81b321, 0x8a8a, 0x42ca, \
> +	{0xa5, 0x08, 0xfe, 0x65, 0x9d, 0x8e, 0x45, 0x86}	\

Nit: remove the tabs, if you're already changing these lines

::: mailnews/addrbook/public/nsIAbDirFactory.idl
@@ -43,4 +43,4 @@
> >  
> >  interface nsIAbDirectory;
> >  
> > -[scriptable, uuid(ad61b4fc-d8d8-40b2-b924-4c10f28a8a17)]
> > +[scriptable, uuid(3b1cf12e-2a7d-4719-8e51-23d2f6b6a680)]

Doesn't look like nsIAbDirFactory actually needs a UUID revision.

::: mailnews/addrbook/public/nsIAbDirectory.idl
@@ +46,5 @@
>  interface nsIMutableArray;
>  
>  %{C++
> +/* moz-abdirectory:// is the URI to access nsAbBSDirectory,
> + * which is the root directory for all types of address books */

If you're already touching this line, you might as well make it a regular documentation comment.

::: mailnews/addrbook/public/nsIAbMDBDirectory.idl
@@ +51,4 @@
>  [scriptable, uuid(744072be-1ba0-46bc-af24-46e22567a2ea)]
>  interface nsIAbMDBDirectory : nsISupports {
>  
> +	// Creates an directory component from the

Nit: s/an/a/

::: mailnews/addrbook/src/nsAbBSDirectory.cpp
@@ +69,5 @@
> +  mURI = aURI;
> +  return NS_OK;
> +}
> +
> +NS_IMPL_ISUPPORTS_INHERITED1(nsAbBSDirectory, nsAbDirProperty, nsIAbDirectory)

Since nsAbDirProperty already declares the QI for nsIAbDirectory, you don't need to do it again here. This comment also applies to the other ns*Directory implementations.

Comment 36

[Joshua Cranmer [:jcranmer]]

One complicated comment:

nsIAbManager is listed as THREADSAFE, which means any method can be called from any thread in theory. The hashtable for the URI->directory map is not threadsafe, which means adding new entries into the database might not be seen by other threads. Since acquiring a reference to nsIAbManager in the first place acts as a memory barrier (via the atomic ref count), I'm more concerned that someone might accidentally add a new directory off the main thread, which I'm not sure enforces memory visibility guarantees.

Now, judging from the fact that the RDF code appears to be blissfully unconcerned with the vagaries of multithreaded I/O, it could just be that I'm being extra-paranoid here...

Comment 37

[Joshua Cranmer [:jcranmer]]

Some more information: there apparently remains a use of address books-as-rdf in import code (if you set the ab import to import into a specific address book). Grepping indicates that this code does not appear to be used, and code coverage confirms that it's not being tested. Could you be so kind as to fix nsImportAddressBook's GetAddressBookFromUri method as well?

Comment 38

[Joshua Cranmer [:jcranmer]]

And I wish I would stop finding these:
* Spurious include of rdf.h in nsAbDirProperty.cpp
* mail/base/content/ABSearchDialog.js: nsIAbDirectory as RDF resource
  (equivalent one in suite's copy of this file)

I think that's all of them... most of the other nsIRDFService uses I've seen are either obviously folders or I've verified to not refer to the address book (at 347 instances according to MXR, I'm not going to go dumpster diving into every single nsIRDFService definition).

Comment 39

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

Attachment: De-RDF Patch - V2

> Since I presume this is being required as a contract-ID for directories, it 
> would probably help to note this in the documentation for nsIAbDirectory.

Done.

> Nit: remove the tabs, if you're already changing these lines

Done.

> Doesn't look like nsIAbDirFactory actually needs a UUID revision.

Reverted.

> If you're already touching this line, you might as well make it a regular 
> documentation comment.

Done.

> Nit: s/an/a/

Fixed.

> Since nsAbDirProperty already declares the QI for nsIAbDirectory,
> you don't need to do it again here. This comment also applies to
> the other ns*Directory implementations.

Hm.  On removing that line, linking produces the following error:

/media/Projects/mozilla/thunderbird/mailnews/addrbook/src/nsAbBSDirectory.cpp:58: undefined reference to `vtable for nsAbBSDirectory'
/usr/bin/ld: ../../staticlib/components/libmail.a(nsAbBSDirectory.o): relocation R_386_GOTOFF against undefined hidden symbol `vtable for nsAbBSDirectory' can not be used when making a shared object
/usr/bin/ld: final link failed: Bad value
collect2: ld returned 1 exit status

Thoughts on that?

> nsIAbManager is listed as THREADSAFE, which means any method can be called 
> from any thread in theory. The hashtable for the URI->directory map is not 
> threadsafe, which means adding new entries into the database might not be 
> seen by other threads. Since acquiring a reference to nsIAbManager in the 
> first place acts as a memory barrier (via the atomic ref count), I'm more 
> concerned that someone might accidentally add a new directory off the main 
> thread, which I'm not sure enforces memory visibility guarantees.
>
> Now, judging from the fact that the RDF code appears to be blissfully 
> unconcerned with the vagaries of multithreaded I/O, it could just be that I'm 
> being extra-paranoid here...

Hm.  From the sounds of it, this patch doesn't make this problem any worse than it already is.  While we might want to clean this up down the road, considering the time pressure I'm under to get this landed, we might want to push that off for later.

> Could you be so kind as to fix nsImportAddressBook's GetAddressBookFromUri 
> method as well?

Done

> And I wish I would stop finding these:
> * Spurious include of rdf.h in nsAbDirProperty.cpp
> * mail/base/content/ABSearchDialog.js: nsIAbDirectory as RDF resource
>  (equivalent one in suite's copy of this file)

Done

Comment 40

[David :Bienvenu]

Comment on attachment 538918 [details] [diff] [review]
De-RDF Patch - V2

the "lucky 13" size initialization of the ab store and osx card store hash tables look a bit odd. I'm not sure it's worthwhile for the ab store, and it seems a bit small for the os/x card store. In any case, a meaningfully named constant (or two) would be more appropriate. Other than that, this continues to looks reasonable...

Comment 41

[Joshua Cranmer [:jcranmer]]

(In reply to comment #39)
> > Since nsAbDirProperty already declares the QI for nsIAbDirectory,
> > you don't need to do it again here. This comment also applies to
> > the other ns*Directory implementations.
> 
> Hm.  On removing that line, linking produces the following error:
> 
> /media/Projects/mozilla/thunderbird/mailnews/addrbook/src/nsAbBSDirectory.
> cpp:58: undefined reference to `vtable for nsAbBSDirectory'
> /usr/bin/ld: ../../staticlib/components/libmail.a(nsAbBSDirectory.o):
> relocation R_386_GOTOFF against undefined hidden symbol `vtable for
> nsAbBSDirectory' can not be used when making a shared object
> /usr/bin/ld: final link failed: Bad value
> collect2: ld returned 1 exit status
> 
> Thoughts on that?

It's not that you want to remove it, you can replace it with NS_IMPL_ISUPPORTS_INHERITED0(nsAbBSDirectory, nsAbDirProperty).

Comment 42

[Joshua Cranmer [:jcranmer]]

Comment on attachment 538918 [details] [diff] [review]
De-RDF Patch - V2

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

So I've looked at pretty much every file except nsAbOSXDirectory.*, as my familiarity with that stuff is very low.

The nsAbOutlookDirectory code looks fairly wrong, but as you're not really changing anything that wasn't already the case, I'll leave fixing that to another bug.

Modulo these issues and the NS_IMPL_ISUPPORTS issues, r+.

::: mailnews/addrbook/src/nsAbManager.cpp
@@ +279,5 @@
> +    NS_ENSURE_SUCCESS (rv, rv);
> +
> +    nsCAutoString scheme;
> +    rv = nsService->ExtractScheme(aURI, scheme);
> +    NS_ENSURE_SUCCESS (rv, rv);

I'm dubious that it's worth using nsIIOService just to get the scheme; why not just use

PRInt32 colon = aURI.FindChar(':');
if (colon <= 0)
  return NS_ERROR_MALFORMED_URI;
scheme = Substring(aURI, 0, colon);

?

::: mailnews/addrbook/src/nsAbOSXCard.mm
@@ +88,4 @@
>    else if (!oldValue.Equals(aValue)) {
>      aCard->SetPropertyAsAString(aMemberName, aValue);
>      
> +    nsISupports *supports = NS_ISUPPORTS_CAST(nsIAbCard*, aCard);

nsAbCardProperty, not nsIAbCard...

Actually, is NS_ISUPPORTS_CAST even necessary? Try this code without it and see if it compiles.

::: mailnews/addrbook/src/nsAbOutlookDirFactory.cpp
@@ +48,4 @@
>  
>  #include "prlog.h"
>  
> +#undef CreateDirectory

Why do you need this #undef here?

::: mailnews/addrbook/src/nsAbOutlookDirectory.cpp
@@ +192,2 @@
>  
> +  if (NS_SUCCEEDED(rv)) {

As long as you're already touching the code here, you might as well go all the way and fix up style issues in this code.
For example, this if block could just be replaced with `NS_ENSURE_SUCCESS(rv, rv);' to allow you to reindent the others.

In addition, you can move the `{'s to the next line per style guidelines, and remove spaces from before semicolons.

@@ +242,5 @@
> +              NotifyItemAddition(card) ;
> +          }
> +      }
> +      else
> +        NS_WARNING("Card wasn't stored");

Since this event is apparently expected (judging from the comment at line 219), I'm not sure this ought to be an NS_WARNING.

@@ +303,5 @@
> +
> +	nsCAutoString stub;
> +    nsAbWinType objType = getAbWinType(kOutlookDirectoryScheme, uri.get(), stub, aEntry) ;
> +
> +    return NS_OK ;

Nit: you're touching these lines as well, so fix indentation, extra whitespace issues...

::: mailnews/addrbook/src/nsAddrDatabase.cpp
@@ +3222,5 @@
> +    rv = proxyObjMgr->GetProxyForObject( NS_PROXY_TO_MAIN_THREAD,
> +                               NS_GET_IID( nsIAbDirectory),
> +                               parentDir,
> +                               NS_PROXY_SYNC | NS_PROXY_ALWAYS,
> +                               getter_AddRefs(proxiedParentDir));

As long as you're changing this line, can you fix the whitespace?

Comment 43

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

Attachment: De-RDF Patch - V3 (r+ from jcranmer, sr+ from bienvenu)

Hey - thanks for the reviews!  There's a question for bienvenu below - that's why I've added him for feedback.

> the "lucky 13" size initialization of the ab store and osx card store hash
> tables look a bit odd. 

Removed for the address book store, and increased to 100 for the OSX card store.

> It's not that you want to remove it, you can replace it with
> NS_IMPL_ISUPPORTS_INHERITED0(nsAbBSDirectory, nsAbDirProperty).

Done.

> I'm dubious that it's worth using nsIIOService just to get the scheme

Fixed, thanks.

> nsAbCardProperty, not nsIAbCard...

Done.

> Actually, is NS_ISUPPORTS_CAST even necessary? Try this code without it 
> and see if it compiles.

Yeah, it failed out.  It seems to want the cast here.

> Why do you need this #undef here?

I think that got put in by bienvenu at somepoint during my WIP patches.  Bienvenu - is there a reason to keep this in?

> As long as you're already touching the code here, you might as well go
> all the way and fix up style issues in this code.

Done.

> Since this event is apparently expected (judging from the comment at line 
> 219), I'm not sure this ought to be an NS_WARNING.

Removed.

> Nit: you're touching these lines as well, so fix indentation, extra 
> whitespace issues...

Done.

> As long as you're changing this line, can you fix the whitespace?

Done.

Comment 44

[David :Bienvenu]

(In reply to comment #43)
> > Why do you need this #undef here?
> 
> I think that got put in by bienvenu at somepoint during my WIP patches. 
> Bienvenu - is there a reason to keep this in?

Yes, because it won't compile without it, or it least it didn't when I was using the patch. CreateDirectory is defined in a windows header file, and it conflicts with our method.  There may be other ways around it, but you'd want to make sure you still build on windows if you take this out.

Comment 45

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

> Yes, because it won't compile without it, or it least it didn't when 
> I was using the patch. 

Ah, gotcha.  Yeah, it compiles without it now, since the CreateDirectory method was removed from nsIAbDirFactory.

Comment 46

[Mark Banner (:standard8)]

Comment on attachment 540093 [details] [diff] [review]
De-RDF Patch - V3 (r+ from jcranmer, sr+ from bienvenu)

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

Generally looking good, though I'm concerned by the thread safety stuff because we're declaring safety when we're not.

General note: it'd be useful if you could turn on -p for generating patches as this gives the function name which is helpful.

::: mailnews/addrbook/public/nsIAbDirectory.idl
@@ +136,5 @@
> +  /**
> +   * Initializes a directory, pointing to a particular
> +   * URI
> +   */
> +  void init (in string aURI);

nit: no space before (

::: mailnews/addrbook/src/nsAbDirProperty.cpp
@@ +76,4 @@
>  #endif
>  }
>  
> +NS_IMPL_THREADSAFE_ISUPPORTS4(nsAbDirProperty, nsIAbDirectory, nsISupportsWeakReference,

I'm not convinced making this threadsafe is the right thing to do. We're definitely not managing threads here, and really the derived classes should also be threadsafe.

This seems wrong, but I guess the question is really what is actually trying to access these as non-threadsafe? hence why do we need to change it now?

@@ +293,5 @@
> +NS_IMETHODIMP
> +nsAbDirProperty::Init(const char *aURI)
> +{
> +
> +  mURINoQuery = aURI;

nit: no blank line necessary.

::: mailnews/addrbook/src/nsAbMDBDirectory.cpp
@@ +476,4 @@
>      rv = database->AddListener(this);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    nsCOMPtr<nsIAbDirectory> directory;

nit: Generally prefer to create this variable just before it is used (at least two places in this file).

::: mailnews/addrbook/src/nsAbManager.cpp
@@ +296,5 @@
> +
> +    // If all went well, add the address book to the look-up table, and
> +    // return it.
> +
> +    mAbStore.Put(aURI, directory);

This isn't going to keep address books that are based on queries alive is it? iirc we RDF didn't keep those alive, and we generally need them to go away.

::: mailnews/addrbook/src/nsAbOSXDirFactory.cpp
@@ +82,4 @@
>  {
>    return NS_OK;
>  }
> +

nit: this change seems a bit pointless.

::: mailnews/addrbook/src/nsAbOSXDirectory.h
@@ +55,5 @@
>  class nsIAbBooleanExpression;
>  
> +#ifdef __OBJC__
> +@class NSString;
> +@class ABRecord;

I don't see why these need defining here.

::: mailnews/addrbook/src/nsAbOSXDirectory.mm
@@ +87,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NS_IF_ADDREF(*aResult = directory);
> +
> +  return NS_OK;

nit: no blank line necessary (several places in this file).

@@ +92,4 @@
>  }
>  
>  static nsresult
> +GetCard(ABRecord *aRecord, nsIAbCard **aResult)

Do we really want to get the OSX Directory each time? As we generally call this from loops, it seems like it may affect perf a bit more than necessary.

@@ +569,5 @@
> +    // to create were already created in the root nsAbOSXDirectory,
> +    if (mURINoQuery.Length() > sizeof(NS_ABOSXDIRECTORY_URI_PREFIX))
> +      rv = GetCard([cards objectAtIndex:i], getter_AddRefs(card));
> +    // If we're not a Group, that means we're the root nsAbOSXDirectory,
> +    // which means we have to create the cards from scratch.

Unusual(?) way of doing comments, I'd have expected this after the else.

Comment 47

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

Attachment: Patch v4 - r+ from jcranmer, sr+ from bienvenu

Standard8:

Thanks for the review!

> General note: it'd be useful if you could turn on -p for generating patches as > this gives the function name which is helpful

Done.

> nit: no space before (

Done.

> This seems wrong, but I guess the question is really what is actually trying > to access these as non-threadsafe? hence why do we need to change it now?

Very strange / unsettling, is that without it, everything works just fine.  I may have had a reason for changing it, but unfortunately it escapes me at this point.  Perhaps it's an unfortunate leftover from one of my earlier iterations? Regardless, I've removed it, and everything seems to work just peachy.

> nit: no blank line necessary.

Removed.

> nit: Generally prefer to create this variable just before it is used (at
> least two places in this file).

Ack, fixed.

> This isn't going to keep address books that are based on queries alive is it? > iirc we RDF didn't keep those alive, and we generally need them to go away.

Hm.  If they're destroyed using RDF, I cannot seem to tell when.  At bienvenu's advice, I set a breakpoint in nsDirectoryDataSource::OnItemRemoved, made some autocomplete queries, sent a message, refreshed my inbox, and waited around for a bit for it to hit my breakpoint, and it never did.  So I'm forced to wonder if the query directories are cleared out at all...if not, then my patch doesn't make the situation better or worse.

> nit: this change seems a bit pointless.

Fixed.

> I don't see why these need defining here.

Removed.

> nit: no blank line necessary (several places in this file).

Removed.

> Do we really want to get the OSX Directory each time? As we generally call 
> this from loops, it seems like it may affect perf a bit more than necessary.

Ok, instead of querying for the root OSX directory each time, I have directories cache the root OSX directory, and pass it into the GetCard function.

> Unusual(?) way of doing comments, I'd have expected this after the else.

Fixed!

Thanks again for the review!  Appears to behave correctly, and all tests appear to pass:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=52559cd0fe79

Let me know if there's anything else I can do.

Comment 48

[Mark Banner (:standard8)]

(In reply to comment #47)
> > This isn't going to keep address books that are based on queries alive is it?
> > iirc we RDF didn't keep those alive, and we generally need them to go away.
> 
> Hm.  If they're destroyed using RDF, I cannot seem to tell when.  At
> bienvenu's advice, I set a breakpoint in
> nsDirectoryDataSource::OnItemRemoved, made some autocomplete queries, sent a
> message, refreshed my inbox, and waited around for a bit for it to hit my
> breakpoint, and it never did.  So I'm forced to wonder if the query
> directories are cleared out at all...if not, then my patch doesn't make the
> situation better or worse.

I experimented a bit more, in the process I found and fixed bug 669151, however, with a bit of digging, I found that if nsAbManager stores references to queries, then LDAP connections don't get dropped. We don't currently re-use the LDAP connections, hence keeping them open until shutdown isn't a good thing.

So I think we really should not have queries stored in the manager. This seems to work and pass tests, this is the bit of code I used:

    PRBool isQuery = PR_FALSE;
    rv = directory->GetIsQuery(&isQuery);
    NS_ENSURE_SUCCESS(rv, rv);

    if (!isQuery)
      mAbStore.Put(aURI, directory);

Comment 49

[Mark Banner (:standard8)]

Comment on attachment 543047 [details] [diff] [review]
Patch v4 - r+ from jcranmer, sr+ from bienvenu

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

General nit: There's a few places throughout the patch where you have NS_ENSURE_SUCCESS (rv, rv); this should just be NS_ENSURE_SUCCESS(rv, rv); i.e. no space.

I've now noticed that the import tests are failing with threadsafe errors - that's strange as they were ok without the patch, I'm currently looking into that.

Comment 50

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

> So I think we really should not have queries stored in the manager. This seems
> to work and pass tests, this is the bit of code I used:

Excellent, thanks for the tip!  Will do.

> General nit: There's a few places throughout the patch where you have
> NS_ENSURE_SUCCESS (rv, rv); this should just be NS_ENSURE_SUCCESS(rv, rv); i.e.
> no space.

On it.

> I've now noticed that the import tests are failing with threadsafe errors - 
> that's strange as they were ok without the patch, I'm currently looking into 
> that.

Ah, yes!  That sounds familiar.  I have a feeling this is the explanation for my NS_IMPL_THREADSAFE_ISUPPORTS4 code you mentioned in Comment 46.  Yes, any and all assistance there is appreciated.

Thanks again for the review,

-Mike

Comment 51

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

Attachment: Patch v5

Thanks for the review!  This patch addresses most of the issues raised in Comment 48 and Comment 49.

The lone exception being the thread-safety problem, which Standard8 is currently investigating.

Comment 52

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

Attachment: Patch v6

This patch incorporates Standard8's thread-safety fix in the address book importer.

This revision also patched a big where imported address books did not appear in the UI until TB restart.  This was because the AbManager was instantiating a new root address book directory, as opposed to returning it's cached copy.  Should probably have the root address book directory be a singleton.  Future plans.

Comment 53

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

Results from my try build will be available here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=72399fa022e2

Comment 54

[Mark Banner (:standard8)]

Comment on attachment 543833 [details] [diff] [review]
Patch v6

That looks better. I'll ask bienvenu to double-check the import changes tomorrow, but I think this can land as is for now given that the unit tests show up clear.

Comment 55

[Jim Porter (:squib)]

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