Closed Bug 514666 Opened 15 years ago Closed 12 years ago

Ignored exception in EditCardOKButton: subdirectory.indexOf is not a function

Categories

(Thunderbird :: Address Book, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 14.0

People

(Reporter: neil, Assigned: aceman)

References

Details

Attachments

(2 files, 6 obsolete files)

While trying to debug some address card stuff I noticed some ignored exceptions flash by in Venkman. It looks as if the code is trying to do something, but I'm not sure whether, what or how it should be doing it these days.
There is code containing this call, but it is surrounded in a try {} block. Can it throw exceptions then? Has it changed since?

 // create a list of mailing lists and the index where the card is at.
  for ( i=0;  i < listDirectoriesCount; i++ ) {
    var subdirectory = directory.addressLists.queryElementAt(i, Components.interfaces.nsIAbDirectory);
    try {
      var index = subdirectory.indexOf(gEditCard);
      foundDirectories[foundDirectoriesCount] = {directory:subdirectory, index:index};
      foundDirectoriesCount++;
    } catch (ex) {}
  }
For "ignored" read "surrounded in a try {} with an empty catch {} block"

I can't remember what the exception was but I assume you could comment out the try/catch so you get to see the exception on the Error Console ;-)
Can't it be that somebody already confirmed that exception is harmless so he hid it?
(In reply to comment #2)
> I can't remember what the exception was
Which is why I put it in the summary, d'oh!

(In reply to :aceman from comment #3)
> Can't it be that somebody already confirmed that exception is harmless so he
> hid it?
That would possibly be appropriate for a component that returned a failure code, but not for this exception.
So you say it is still worth pursuing as there shouldn't be exceptions thrown at this level even when masked by 'try catch'?
Well, it's particularly suspicious that the code is calling a non-existent function. Maybe the code was always broken, or maybe the function got renamed or deprecated; I didn't actually have time to check.
Maybe subdirectory variable is just empty (or null) is some cases, so calling any members of it is useless.
I'll try to look into it.
Assignee: nobody → acelists
If subdirectory is a nsIAbDirectory, I am not sure it has a .indexOf() function:
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/public/nsIAbDirectory.idl

I think the comments say we want to find cards inside maillists. So I think we need to check if subdirectory is a maillist and only then find the index of the card in its .addressLists.

Then the existing code does several lines later, essentially: subdirectory.addressLists.replaceElementAt(..., index, ...) .

I'll prepare a patch.
Attached patch patch experiment (obsolete) — Splinter Review
So this seems to do something.

STR:
- create a mailinglist with one contact
- go to the parent addressbook
- notice that contact is visible in the list too
- edit that contact

Result:
The code here will find that that contact is in a mailing list and will update the contact there.

The problem is, it appeared visually that there is not problem also before I fixed the code. But now it seems more correct, does not throw and also finds contacts if they are in mailing list. Mconley please check what this code is supposed to do.
Attachment #598403 - Flags: feedback?(mconley)
Status: NEW → ASSIGNED
Attached patch patch experiment, v2 (obsolete) — Splinter Review
Fixed patch, left debugging in there.
Attachment #598403 - Attachment is obsolete: true
Attachment #598406 - Flags: feedback?(mconley)
Attachment #598403 - Flags: feedback?(mconley)
Comment on attachment 598406 [details] [diff] [review]
patch experiment, v2

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

You've found a solution, but I'm not sure it's an optimal - or even a necessary fix.

::: mail/components/addrbook/content/abCardOverlay.js
@@ +213,5 @@
>    // create a list of mailing lists and the index where the card is at.
> +  for (let i = 0; i < listDirectoriesCount; i++) {
> +    let subdirectory = directory.addressLists.queryElementAt(i, Components.interfaces.nsIAbDirectory);
> +    if (subdirectory.isMailList) {
> +      for (let index = 0; index < subdirectory.addressLists.length; index++) {

Looking at the code on the left...I'm not sure it's *ever* worked.  An nsIAbDirectory certainly does not have a function called "indexOf".

So, we've established it's broken.

Does fixing it give us anything?  Can you find out what foundDirectories is being used for?
You can see it in the patch. Several lines later the found cards (in foundDirectories) are replaced with gEditCard (probably the card being changed).
I already said in comment 9 I can't visually see any difference after fixing the bug so that actually any cards are replaced. I think they were replaced (updated) even without the fix. But there may be something different internally, that is up to you :)
If that's the case, I'd prefer to excise the code, instead of extending its life. :)
Isn't there any test? :)
Yes, and I *think* I just found out what this code is trying to do...

The problem occurs when we've got one or more compose windows open, and we've set some of those compose windows' TO, CC, or BCC fields to point to a mailing list.

If, after we've done this, we edit a contact in a mailing list, the change does not take effect until, well, TB restarts, it seems.  :(

Steps to reproduce:

1)  In an address book, create a contact with email address A
2)  Create a mailing list in that address book, and drag the contact in
3)  Send mail to that mailing list.  The contact should receive mail at A.
4)  Open the address book again, and change the card for the contact in the mailing list.
5)  Send mail to the mailing list.

What happens?

The contact still receives mail at A

What should have happened?

The contact should have received mail at the new, changed address.

aceman - can you reproduce?  Does your patch fix this problem?
It seems the compose window does not need to be open.
A change in a card which is a member of a mailing list seems OK in the addressbook manager, the mailing list content is shown updated. But any new composition to this mailinglist uses the old card info until restart.

And yes, the patch fixes this for me!
Could be worth some comment in the code?
Comment on attachment 598406 [details] [diff] [review]
patch experiment, v2

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

I've giving feedback+ because I think this is a good problem to fix, but I'd give it an r- for the reasons listed below (namely, we should probably use indexOf instead of a for-loop iteration for searching).

::: mail/components/addrbook/content/abCardOverlay.js
@@ +207,5 @@
>    }
>  
>    var listDirectoriesCount = directory.addressLists.length;
>    var foundDirectories = new Array();
>    var foundDirectoriesCount = 0;

Instead of keeping a count like this, why not just start with the empty foundDirectories Array, and .push items onto it?

@@ +213,5 @@
>    // create a list of mailing lists and the index where the card is at.
> +  for (let i = 0; i < listDirectoriesCount; i++) {
> +    let subdirectory = directory.addressLists.queryElementAt(i, Components.interfaces.nsIAbDirectory);
> +    if (subdirectory.isMailList) {
> +      for (let index = 0; index < subdirectory.addressLists.length; index++) {

Can we not use subdirectory.addressLists.indexOf?  https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIArray#indexOf%28%29  I think that would save us having to for-loop through each time.
Attachment #598406 - Flags: feedback?(mconley) → feedback+
So actually the bug was that the original 'subdirectory.indexOf' should have been 'subdirectory.addressLists.indexOf' and all would be solved?
Yes - though I think you're right to check to ensure that the subdirectory is a mailing list before doing the check.
Attached patch patch v3 (obsolete) — Splinter Review
Per discussion in IRC, .indexOf() didn't really work. So I left the patch at using a for loop. But converted the temporary array to use push and pop as requested.
Attachment #598406 - Attachment is obsolete: true
Attachment #606893 - Flags: review?(mconley)
Comment on attachment 606893 [details] [diff] [review]
patch v3

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

aceman:

Just a few small things here, but this looks mostly good.

Seeing what we have to do to make this work makes me sad, but I can't think of a better way.

Except by rebuilding the address book.  Maybe when we start getting that ball rolling, you'd be interested in lending a hand?  :)

Good work,

-Mike

::: mail/components/addrbook/content/abCardOverlay.js
@@ +206,5 @@
>      directory = GetDirectoryFromURI(parentURI);
>    }
>  
> +  let listDirectoriesCount = directory.addressLists.length;
> +  let foundDirectories = new Array();

This should be "[];" instead of "new Array();"

@@ +212,4 @@
>    // create a list of mailing lists and the index where the card is at.
> +  for (let i = 0; i < listDirectoriesCount; i++) {
> +    let subdirectory = directory.addressLists
> +                         .queryElementAt(i, Components.interfaces.nsIAbDirectory);

The .queryElementAt should be aligned so that the period aligns beneath the period of the "addressLists" above it.

@@ +215,5 @@
> +                         .queryElementAt(i, Components.interfaces.nsIAbDirectory);
> +    if (subdirectory.isMailList) {
> +      // See if any card in this list is the one we edited.
> +      // Must compare card contents using .equals() instead of .indexOf()
> +      // because gEditCard is not really a member of the .addressLists array.

Good comment.

@@ +219,5 @@
> +      // because gEditCard is not really a member of the .addressLists array.
> +      let listCardsCount = subdirectory.addressLists.length;
> +      for (let index = 0; index < listCardsCount; index++) {
> +        let card = subdirectory.addressLists
> +                     .queryElementAt(index, Components.interfaces.nsIAbCard);

align as suggested for lines 214-215.
Attachment #606893 - Flags: review?(mconley) → review-
(In reply to Mike Conley (:mconley) from comment #21)
> Except by rebuilding the address book.  Maybe when we start getting that
> ball rolling, you'd be interested in lending a hand?  :)
The addressbook is not yet a module I would dare to make any bigger changes in.

> > +    let subdirectory = directory.addressLists
> > +                         .queryElementAt(i, Components.interfaces.nsIAbDirectory);
> 
> The .queryElementAt should be aligned so that the period aligns beneath the
> period of the "addressLists" above it.

The line will be longer then 80 chars. Can I make it:
 let subdirectory = directory.addressLists
   .queryElementAt(i, Components.interfaces.nsIAbDirectory);
(In reply to :aceman from comment #22)
> The line will be longer then 80 chars. Can I make it:
>  let subdirectory = directory.addressLists
>    .queryElementAt(i, Components.interfaces.nsIAbDirectory);

Yes, that's acceptable.
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #606893 - Attachment is obsolete: true
Attachment #607304 - Flags: review?(mconley)
aceman:

This looks really good...I would love to have some Mozmill tests for this. How about tomorrow, you and I figure out how to get Mozmill tests working on your machine?

-Mike
Yes, we can try to make them work correctly.
But do not count on me writing any new ones :) I can't even do xpcshell ones.
aceman:

What happens when, from your objdir, you type in the following:

make SOLO_TEST=addrbook mozmill-one

?

-Mike
Is this what I should watch for?
INFO Passed: 15
INFO Failed: 0
INFO Skipped: 0
SUMMARY-PASS | test-address-book-panes.js::setupModule
SUMMARY-PASS | test-address-book-panes.js::test_hide_directory_pane
SUMMARY-PASS | test-address-book-panes.js::test_show_directory_pane
SUMMARY-PASS | test-address-book-panes.js::test_hide_contact_pane
SUMMARY-PASS | test-address-book-panes.js::test_show_contact_pane
SUMMARY-PASS | test-address-book-panes.js::test_persist_panes
SUMMARY-PASS | test-address-book-panes.js::teardownModule
SUMMARY-PASS | test-address-book.js::setupModule
SUMMARY-PASS | test-address-book.js::test_order_of_address_books
SUMMARY-PASS | test-address-book.js::test_persist_collapsed_and_expanded_states
SUMMARY-PASS | test-address-book.js::test_deleting_contact_causes_confirm_prompt
SUMMARY-PASS | test-address-book.js::test_deleting_contacts_causes_confirm_prompt
SUMMARY-PASS | test-address-book.js::test_deleting_mailing_lists
SUMMARY-PASS | test-address-book.js::test_writing_to_mailing_list
SUMMARY-PASS | test-address-book.js::teardownModule

This is with this patch applied.
Attached patch Mozmill test case (obsolete) — Splinter Review
Hey aceman,

So I've whipped up a quick Mozmill test case.  It's currently failing - and manual testing also verifies that this doesn't fix the mailing-list updating bug.  Have another go at it?  You can use my test to help verify.

-Mike
Comment on attachment 607304 [details] [diff] [review]
patch v4

See above.
Attachment #607304 - Flags: review?(mconley) → review-
So how comes the patch fixes it for me?
Can you specify the STR more precisely?
STR (these are the same steps the test uses):

1)  Create an address book, put a contact in it with address before@example.com.
2)  Create a mailing list in that address book, and drag the contact you created into it.
3)  Edit the contact, and change the address to after@example.com
4)  Click OK to close the editing dialog
5)  Double-click on the mailing list to list the cards assigned to it

What happens?

The list contains a single contact with the email address "before@example.com".

What's expected?

The list should contain a single contact with the email address "after@example.com".
Ok, I'll try that.
But those STR are quite different to what we talked about since comment 15.
(In reply to :aceman from comment #33)
> Ok, I'll try that.
> But those STR are quite different to what we talked about since comment 15.

True - I've minimized and simplified the STR somewhat.
Interestingly, the patch fixes the case according to the new STR, when doing them manually (linux).
But your test fails with or without the patch.
Sorry to heap another one on, bienvenu.  I'll ping you today and see if I can steal one of your reviews as a trade. :)
Attachment #609466 - Attachment is obsolete: true
Attachment #610117 - Flags: review?(dbienvenu)
Comment on attachment 607304 [details] [diff] [review]
patch v4

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

Modulo those two nits I found, this appears to do the job.  r=me.

::: mail/components/addrbook/content/abCardOverlay.js
@@ +212,3 @@
>    // create a list of mailing lists and the index where the card is at.
> +  for (let i = 0; i < listDirectoriesCount; i++) {
> +    let subdirectory = directory.addressLists

If you use the shorter variable name "subdir" instead of subdirectory, we can use:

let subdir = directory.addressLists
                      .queryElementAt(i, Components.interfaces.nsIAbDirectory);

(period before queryELementAt aligned with period before addressLists) Which is more readable.

@@ +235,3 @@
>      // Update the addressLists item for this card
> +    let foundItem = foundDirectories.pop();
> +    foundItem.directory.addressLists

I think I'd prefer:

foundItem.directory
         .addressLists
         .replaceElementAt(gEditCard.card, foundItem.cardIndex, false);

(period before addressLists and replaceElementAt aligned with period before directory) For easier readability.
Attachment #607304 - Flags: review- → review+
Comment on attachment 610117 [details] [diff] [review]
Mozmill test case v2

single "that" reads better.
+ * updated in mailing lists that that contact belongs to.
Attachment #610117 - Flags: review?(dbienvenu) → review+
Thanks bienvenu - fixed in this version.
Attachment #610117 - Attachment is obsolete: true
Comment on attachment 610117 [details] [diff] [review]
Mozmill test case v2

>+/**
>+ * Tests that if a contact's address is updated, then the address is also
>+ * updated in mailing lists that that contact belongs to.
>+ */
The "that that" is needed.
The first "that" introduces a defining clause. In other words, it's how you tell whether a given mailing list is updated. (Similarly, the "that" at the beginning sentence defines the test.) Some people erroneously use "which" instead of "that" when it's a defining clause, although "that" can be used instead of ", which" when it's an clause that provides ancillary information.
The second "that" determines that the contact that belongs to the mailing list is the one that was updated. (Yes, I know, I had to use two defining clauses to explain that. Oh, and there goes another determiner.) You could use some other way of referring to the updated contact, such as "the updated contact".
Attachment #610117 - Flags: feedback+
Attachment #610166 - Flags: feedback-
Attached patch patch v5Splinter Review
r=mconley.

So were you able to fix the mozmill test?
Attachment #607304 - Attachment is obsolete: true
Attachment #610208 - Flags: review+
(In reply to :aceman from comment #41)
> Created attachment 610208 [details] [diff] [review]
> patch v5
> 
> r=mconley.
> 
> So were you able to fix the mozmill test?

Yes indeed - although we seem to have hit some kinda grammatical roadblock.

neil / bienvenu:  So which should I go with?

-Mike
(In reply to Mike Conley (:mconley) from comment #42)
> (In reply to :aceman from comment #41)
> > Created attachment 610208 [details] [diff] [review]
> > patch v5
> > 
> > r=mconley.
> > 
> > So were you able to fix the mozmill test?
> 
> Yes indeed - although we seem to have hit some kinda grammatical roadblock.
> 
> neil / bienvenu:  So which should I go with?
> 
> -Mike

oh, I should have never brought it up in the first place. go with that that.
Attachment #610117 - Attachment is obsolete: false
Comment on attachment 610166 [details] [diff] [review]
Mozmill test case v3 (r+ from bienvenu - check-in after patch goes in)

Marking v3 as obsolete, and going with v2.
Attachment #610166 - Attachment is obsolete: true
Patches consolidated and landed in comm-central as http://hg.mozilla.org/comm-central/rev/f82efc4704bb .
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
Blocks: 761852
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: