244870 - nsAbMDBDirectory::InternalAddMailList uses getter_AddRefs inappropriately

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[David :Bienvenu]

http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAbMDBDirectory.cpp#645

    nsCOMPtr<nsIAbDirectory> newlist =
getter_AddRefs(NS_STATIC_CAST(nsIAbDirectory*, dblistproperty));

this line looks wrong - NS_STATIC_CAST doesn't addref, so we're going to
undercount our refs...unless there's something really tricky going on, like a
mismatched addref somewhere...

Comment 1

[Mark Banner (:standard8)]

(In reply to comment #0)
> http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAbMDBDirectory.cpp#645
>     nsCOMPtr<nsIAbDirectory> newlist =
> getter_AddRefs(NS_STATIC_CAST(nsIAbDirectory*, dblistproperty));
> this line looks wrong - NS_STATIC_CAST doesn't addref, so we're going to
> undercount our refs...unless there's something really tricky going on, like a
> mismatched addref somewhere...

The relevant line is now 623.

Note there's an NS_ADDREF on the line before which I think would mean we don't undercount our refs.

However, we're modifying list, which according to the idl probably isn't a good idea:

void addMailList (in nsIAbDirectory list);

However, once we've copied the list, we then QI to nsIAbMDBDirectory. I think what we should be doing, is to create the dblistproperty, QI it into dblist, and then copy the mail list. I think that would solve it.

The only test case I can think of would be dragging a outlook mail list to a mdb directory, which in theory we should be able to do now, but I can't test it.

I'll see if I can knock up a patch and find someone to test it in the next couple of days.

Comment 2

[Mark Banner (:standard8)]

Attachment: Possible patch

I'm not sure this is quite right, but I think it'll work better than the previous.

David, could you test this for me please? I think the only case we may invoke this part of the function is when we drag an outlook mail list to a mdb mail list.

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> David, could you test this for me please? I think the only case we may invoke
> this part of the function is when we drag an outlook mail list to a mdb mail
> list.

David: Ping, did you have chance to test this at all?

Comment 4

[David :Bienvenu]

sorry, I haven't had a chance to try this. I don't have MS Office on my normal desktop, and I haven't set up the Outlook AB integration feature anywhere, actually. I can try to get to it, but do we have any evidence from the field that this is broken?

FWIW, the patch looks OK to me...

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 224113 [details] [diff] [review]
Possible patch

(In reply to comment #4)
> sorry, I haven't had a chance to try this. I don't have MS Office on my normal
> desktop, and I haven't set up the Outlook AB integration feature anywhere,
> actually. I can try to get to it, but do we have any evidence from the field
> that this is broken?
> 
> FWIW, the patch looks OK to me...
> 

As far as I know we haven't had any problems out in the field, but it'd be nice to fix this bit of code to not use the static cast, so we can be more confident the ref counting is working correctly.

Comment 6

[Mark Banner (:standard8)]

Patch checked in:

Checking in mailnews/addrbook/src/nsAbMDBDirectory.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsAbMDBDirectory.cpp,v  <--  nsAbMDBDirectory.cpp
new revision: 1.52; previous revision: 1.51

Comment 7

[Mark Banner (:standard8)]

Ok, this broke adding new mail lists :-( New patch coming up.

Comment 8

[Mark Banner (:standard8)]

Attachment: Fix regression

Obviously people aren't using trunk that much...

The original patch managed to remove the "list = newlist" line. I don't like that as its assigning the newlist to a function argument and using that later. I'm therefore suggesting this version as I think its clearer what's actually be used where.

Comment 9

[Mark Banner (:standard8)]

Regression fix checked in.