nsIAbDirectory.deleteCards() incorrectly throws NS_ERROR_INVALID_POINTER exception

RESOLVED WORKSFORME

Status

Thunderbird
Address Book
RESOLVED WORKSFORME
10 years ago
6 years ago

People

(Reporter: Leni, Unassigned)

Tracking

Trunk
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

1.37 KB, application/x-javascript
Details
1.08 KB, application/x-javascript
Details
(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: version 2.0.0.16 (20080708)

deleteCards() throws an exception when it shouldn't.


Reproducible: Always

Steps to Reproduce:
To reproduce, run the attached testcase.js


Actual Results:  
The second call to add_and_delete_card() throws an NS_ERROR_INVALID_POINTER exception.


Expected Results:  
Expected the second call to add_and_delete_card() to work.

The put_mork_into_a_wierd_state() function is admittedly artificial, but it is the critical ingredient in reproducing this bug via a test case.  What is actually putting mork into a wierd state out in the field I have no idea.

Opening the addressbook UI seems to clear the wierd state.  The second call to add_and_delete_card() works!

Here's what appears in the console under a debug build of 2.0.0.14 when the NS_ERROR_INVALID_POINTER is thrown.

WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
/home/L/wrk/mozilla/mailnews/addrbook/src/nsAbMDBDirectory.cpp, line 559

This is:
   rv = RemoveCardFromAddressList(card);
   NS_ENSURE_SUCCESS(rv,rv);

The NS_ERROR_NULL_POINTER is returned by RemoveCardFromAddressList() here:
   if (!m_AddressList)
   {
     rv = mDatabase->GetMailingListsFromDB(this);
     NS_ENSURE_SUCCESS(rv, rv);
     // Ensure that the previous call did set the address list pointer
     if (!m_AddressList)
       return NS_ERROR_NULL_POINTER;
   }

Discussion on dev-apps-thunderbird:
http://groups.google.com/group/mozilla.dev.apps.thunderbird/browse_thread/thread/8100b8dd0d6becce
(Reporter)

Comment 1

10 years ago
Created attachment 334616 [details]
test case
I think I know someone else who has been seeing this (adding to cc).

Does this only happen on branch?
(Reporter)

Comment 3

10 years ago
I'm not up to speed with the current state of the trunk addressbook api but I'll get across it around b1 code-freeze time and adapt the attached testcase.js to suit.

From the looks of:
https://bugzilla.mozilla.org/show_bug.cgi?id=448165
it might not be easy to put the .mab into a wierd state on trunk - which is the key to reproducing it.

Just to restate the interaction with addressbook UI more succinctly:
- addressbook UI not open ==> bug manifests
- addressbook UI open ==> no bug

Comment 4

10 years ago
For me on branch, the exception was thrown during the first call to add_and_delete_card.  Passing {} to DeleteCards in put_mork_into_a_wierd_state() should throw something like:
'JavaScript component does not have a method named: "Count"'
on this line:
http://mxr.mozilla.org/mozilla1.8/source/mailnews/addrbook/src/nsAbMDBDirectory.cpp#476

But it sounds like you were able to get to the second call to add_and_delete_card(), correct?

I got the following warnings before the exception was thrown, as you mentioned in the newsgroup, after the card is edited to the database.  Without looking at the source, it looks like the listeners aren't setup at all or aren't setup properly before the address book is opened.
WARNING: NS_ENSURE_TRUE(mListeners) failed, file /home/josh/mozilla-1.8/mozilla/mailnews/addrbook/src/nsAddrBookSession.cpp, line 94
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file /home/josh/mozilla-1.8/mozilla/mailnews/addrbook/src/nsAbMDBDirectory.cpp, line 196
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file /home/josh/mozilla-1.8/mozilla/mailnews/addrbook/src/nsAbMDBDirectory.cpp, line 874
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file /home/josh/mozilla-1.8/mozilla/mailnews/addrbook/src/nsAddrDatabase.cpp, line 299

While looking at nsAbMDBDirectory::RemoveCardFromAddressList, it appears that NS_ERROR_NULL_POINTER is returned if no mail lists are found.  To test this theory, I added a mailing list to the personal address book and ran the test again (without put_mork_into_a_wierd_state) and it passed.  Can you confirm this behavior?

Should NS_ERROR_NULL_POINTER be changed to NS_OK in this code fragment?
http://mxr.mozilla.org/mozilla1.8/source/mailnews/addrbook/src/nsAbMDBDirectory.cpp#108

I changed the return value to NS_OK, rebuilt, and ran the test without the put_mork_into_a_wierd_state() function (to avoid the Count error) and it worked without a mail list in the PAB.

I'm not sure why it works when the Address Book is open, but it would seem that m_AddressList is not null when stepping through with gdb and listTotal is 0.  I'll look into this more possibly tomorrow.  It looks like m_AddressList is initialized somewhere when the Address Book is first opened.

When I briefly tried to reproduce on trunk with a similar test case it appeared to work.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 5

10 years ago
The try/catch in put_mork_into_a_wierd_state() is to allow the testcase to continue on after the "does not have a method named: Count" exception.

Throwing+catching that exception puts mork into a wierd state which:
- definitely causes deleteCards() to misbehave
- likely causes addCards() to generate those warning msgs
- gets cleared by opening the addressbook UI

I stumbled across deleteCards({}) as one way of generating this wierd state but I think it's likely that there are other ways too.  I think it'd be interesting to know what it is about that exception that could be leaving the mab's internal state screwy.

Re: the listener warnings, I just wonder whether they are a downstream effect of the dodgy state being created earlier.  

> I added a mailing list to the personal
> address book and ran the test again (without
> put_mork_into_a_wierd_state) and it passed.
> Can you confirm this behavior?

Yep - confirmed.  With a mailing list present, the second call to add_and_delete_card() works fine.

> Should NS_ERROR_NULL_POINTER be changed to NS_OK
> in this code fragment?

I don't think so.  If it tries to set m_AddressList and fails, then returning NS_ERROR_NULL_POINTER might be the right thing to do.

Do you understand why it is in RemoveCardFromAddressList() in the first place when there aren't any mailing lists in the .mab?  That seems odd to me and I wonder whether it's a downstream effect of some earlier corruption.

Final thought/question - my guess is that this isn't likely to be fixed on branch, and given that on trunk:
a) mork is disappearing and
b) the mailing list code is on the table for open-heart surgery
I wonder whether it's worth the effort to pursue this?

Comment 6

10 years ago
(In reply to comment #5)
> The try/catch in put_mork_into_a_wierd_state() is to allow the testcase to
> continue on after the "does not have a method named: Count" exception.
> 
> Throwing+catching that exception puts mork into a wierd state which:
> - definitely causes deleteCards() to misbehave
> - likely causes addCards() to generate those warning msgs
> - gets cleared by opening the addressbook UI
> 
> I stumbled across deleteCards({}) as one way of generating this wierd state but
> I think it's likely that there are other ways too.  I think it'd be interesting
> to know what it is about that exception that could be leaving the mab's
> internal state screwy.
I don't know how/why it causes any problems, but I don't know much about mork.  It should fail here (http://mxr.mozilla.org/mozilla1.8/source/mailnews/addrbook/src/nsAbMDBDirectory.cpp#476) and quit before doing anything.
> 
> Re: the listener warnings, I just wonder whether they are a downstream effect
> of the dodgy state being created earlier.

I get the warnings during the first and second calls to editCardToDatabase on 2.0.0.17pre, do you only get them during the second?

> > Should NS_ERROR_NULL_POINTER be changed to NS_OK
> > in this code fragment?
> 
> I don't think so.  If it tries to set m_AddressList and fails, then returning
> NS_ERROR_NULL_POINTER might be the right thing to do.
On trunk it returns NS_OK in the same situation (http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbMDBDirectory.cpp#182) but many other things changed in nsAbMDBDirectory, as well.

> Do you understand why it is in RemoveCardFromAddressList() in the first place
> when there aren't any mailing lists in the .mab?  That seems odd to me and I
> wonder whether it's a downstream effect of some earlier corruption.

That's what I thought at first, but I think that RemoveCardFromAddressList checks every mailing list within the directory and removes the card from it if the mailing list contains that card.  If it reaches that point, neither the card nor the directory was found to be a mail list, and the card was already removed from the directory.  So it does that to make sure the card is removed from any and all lists in the directory.

If m_AddressList is null then it tries to get the mailing lists, but if there aren't any then m_AddressList will remain null and, on branch, return NS_ERROR_NULL_POINTER, or NS_OK on trunk.  Once the address book is opened, m_AddressList is not null whether or not there are any mailing lists in the directory so the test passes.

> Final thought/question - my guess is that this isn't likely to be fixed on
> branch, and given that on trunk:
> a) mork is disappearing and
> b) the mailing list code is on the table for open-heart surgery
> I wonder whether it's worth the effort to pursue this?

I'm not sure if it will be fixed or not.  I could write a patch if the problem isn't too difficult to fix, but it probably wouldn't appear in Thunderbird 2 at least until 2.0.0.17, so extensions could still run into the problem until then.

I'll change the test so it works on trunk and attach it soon.

Comment 7

10 years ago
Created attachment 334909 [details]
test case for trunk

Here is a test case for trunk that passes for me.  However, the put_mork_in_weird_state function doesn't throw an exception.

Comment 8

10 years ago
Comment on attachment 334909 [details]
test case for trunk

I accidentally put the type as application/octet-stream
Attachment #334909 - Attachment mime type: application/octet-stream → application/x-javascript
(Reporter)

Comment 9

10 years ago
> I get the warnings during the first and second calls to editCardToDatabase on
> 2.0.0.17pre, do you only get them during the second?

I only have a debug build of 2.0..14 handy but I see the same as you - the listener warnings happen on the first call to add_card().  So yeah, that issue isn't downstream of put_mork_into_a_weird_state.

The reason I raised it in the newsgroup posting is that like the deleteCards() bug, if the addressbook UI is open, I don't see those warnings at all.

Opening the addressbook UI is doing something to make both these bad behaviours go away.
Blocks: 448165
Keywords: regression
Version: unspecified → Trunk
I really don't see how this is a regression considering it exists on 2.x and trunk. It wouldn't surprise me if it existed on 1.5 as well. This bug also isn't related to bug 448165 although they do touch the same function.

(In reply to comment #9)
> Opening the addressbook UI is doing something to make both these bad behaviours
> go away.

I think what it is probably doing is calling GetChildNodes on the top-level nsAbBSDirectory, which in turn initialises all the directories correctly, especially as I know it opens up the database files to see if there are any mailing lists in them.
No longer blocks: 448165
Keywords: regression
(Reporter)

Comment 11

9 years ago
(In reply to comment #10)
> I think what it is probably doing is calling GetChildNodes on the top-level
> nsAbBSDirectory, which in turn initialises all the directories correctly,

Just to confirm that calling something that directly or indirectly calls GetChildNodes is enough to clear the bad state, after which deleteCards() works.

Comment 12

6 years ago
Is this problem still existing?

Comment 13

6 years ago
Leni?
Whiteboard: [CLOSEME 2012-03-01]
(Reporter)

Comment 14

6 years ago
I can't reproduce it in Thunderbird 10.0

Comment 15

6 years ago
Great. So if you encounter any new testcase reproducing this same bug, then please reopen.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WORKSFORME
Whiteboard: [CLOSEME 2012-03-01]
You need to log in before you can comment on or make changes to this bug.