Last Comment Bug 514666 - Ignored exception in EditCardOKButton: subdirectory.indexOf is not a function
: Ignored exception in EditCardOKButton: subdirectory.indexOf is not a function
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Address Book (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 761852
  Show dependency treegraph
 
Reported: 2009-09-04 07:19 PDT by neil@parkwaycc.co.uk
Modified: 2012-06-07 06:30 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch experiment (2.26 KB, patch)
2012-02-17 15:37 PST, :aceman
no flags Details | Diff | Review
patch experiment, v2 (2.20 KB, patch)
2012-02-17 15:45 PST, :aceman
mconley: feedback+
Details | Diff | Review
patch v3 (2.90 KB, patch)
2012-03-17 12:14 PDT, :aceman
mconley: review-
Details | Diff | Review
patch v4 (2.86 KB, patch)
2012-03-19 13:58 PDT, :aceman
mconley: review+
Details | Diff | Review
Mozmill test case (5.44 KB, patch)
2012-03-26 13:44 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Review
Mozmill test case v2 (5.90 KB, patch)
2012-03-28 06:44 PDT, Mike Conley (:mconley) - (Needinfo me!)
mozilla: review+
neil: feedback+
Details | Diff | Review
Mozmill test case v3 (r+ from bienvenu - check-in after patch goes in) (5.86 KB, patch)
2012-03-28 09:27 PDT, Mike Conley (:mconley) - (Needinfo me!)
neil: feedback-
Details | Diff | Review
patch v5 (2.87 KB, patch)
2012-03-28 11:02 PDT, :aceman
acelists: review+
Details | Diff | Review

Description neil@parkwaycc.co.uk 2009-09-04 07:19:50 PDT
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.
Comment 1 :aceman 2011-12-22 12:50:28 PST
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) {}
  }
Comment 2 neil@parkwaycc.co.uk 2011-12-22 13:07:21 PST
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 ;-)
Comment 3 :aceman 2011-12-22 13:32:01 PST
Can't it be that somebody already confirmed that exception is harmless so he hid it?
Comment 4 neil@parkwaycc.co.uk 2011-12-22 14:33:55 PST
(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.
Comment 5 :aceman 2011-12-23 03:40:48 PST
So you say it is still worth pursuing as there shouldn't be exceptions thrown at this level even when masked by 'try catch'?
Comment 6 neil@parkwaycc.co.uk 2011-12-23 04:13:12 PST
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.
Comment 7 :aceman 2011-12-23 06:13:59 PST
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.
Comment 8 :aceman 2012-02-17 13:43:19 PST
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.
Comment 9 :aceman 2012-02-17 15:37:18 PST
Created attachment 598403 [details] [diff] [review]
patch experiment

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.
Comment 10 :aceman 2012-02-17 15:45:22 PST
Created attachment 598406 [details] [diff] [review]
patch experiment, v2

Fixed patch, left debugging in there.
Comment 11 Mike Conley (:mconley) - (Needinfo me!) 2012-03-02 13:34:47 PST
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?
Comment 12 :aceman 2012-03-02 13:41:56 PST
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 :)
Comment 13 Mike Conley (:mconley) - (Needinfo me!) 2012-03-02 13:44:07 PST
If that's the case, I'd prefer to excise the code, instead of extending its life. :)
Comment 14 :aceman 2012-03-02 13:51:33 PST
Isn't there any test? :)
Comment 15 Mike Conley (:mconley) - (Needinfo me!) 2012-03-02 14:04:46 PST
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?
Comment 16 :aceman 2012-03-02 14:20:28 PST
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 17 Mike Conley (:mconley) - (Needinfo me!) 2012-03-17 09:37:52 PDT
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.
Comment 18 :aceman 2012-03-17 09:42:46 PDT
So actually the bug was that the original 'subdirectory.indexOf' should have been 'subdirectory.addressLists.indexOf' and all would be solved?
Comment 19 Mike Conley (:mconley) - (Needinfo me!) 2012-03-17 10:48:02 PDT
Yes - though I think you're right to check to ensure that the subdirectory is a mailing list before doing the check.
Comment 20 :aceman 2012-03-17 12:14:44 PDT
Created attachment 606893 [details] [diff] [review]
patch v3

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.
Comment 21 Mike Conley (:mconley) - (Needinfo me!) 2012-03-19 07:23:14 PDT
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.
Comment 22 :aceman 2012-03-19 07:46:24 PDT
(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);
Comment 23 Mike Conley (:mconley) - (Needinfo me!) 2012-03-19 07:55:56 PDT
(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.
Comment 24 :aceman 2012-03-19 13:58:52 PDT
Created attachment 607304 [details] [diff] [review]
patch v4
Comment 25 Mike Conley (:mconley) - (Needinfo me!) 2012-03-19 14:08:12 PDT
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
Comment 26 :aceman 2012-03-19 14:17:14 PDT
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.
Comment 27 Mike Conley (:mconley) - (Needinfo me!) 2012-03-23 13:09:06 PDT
aceman:

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

make SOLO_TEST=addrbook mozmill-one

?

-Mike
Comment 28 :aceman 2012-03-23 14:22:40 PDT
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.
Comment 29 Mike Conley (:mconley) - (Needinfo me!) 2012-03-26 13:44:39 PDT
Created attachment 609466 [details] [diff] [review]
Mozmill test case

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 30 Mike Conley (:mconley) - (Needinfo me!) 2012-03-26 13:44:52 PDT
Comment on attachment 607304 [details] [diff] [review]
patch v4

See above.
Comment 31 :aceman 2012-03-26 13:49:52 PDT
So how comes the patch fixes it for me?
Can you specify the STR more precisely?
Comment 32 Mike Conley (:mconley) - (Needinfo me!) 2012-03-26 14:04:51 PDT
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".
Comment 33 :aceman 2012-03-26 14:10:22 PDT
Ok, I'll try that.
But those STR are quite different to what we talked about since comment 15.
Comment 34 Mike Conley (:mconley) - (Needinfo me!) 2012-03-26 14:11:05 PDT
(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.
Comment 35 :aceman 2012-03-26 14:52:49 PDT
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.
Comment 36 Mike Conley (:mconley) - (Needinfo me!) 2012-03-28 06:44:38 PDT
Created attachment 610117 [details] [diff] [review]
Mozmill test case v2

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. :)
Comment 37 Mike Conley (:mconley) - (Needinfo me!) 2012-03-28 06:55:29 PDT
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.
Comment 38 David :Bienvenu 2012-03-28 08:11:40 PDT
Comment on attachment 610117 [details] [diff] [review]
Mozmill test case v2

single "that" reads better.
+ * updated in mailing lists that that contact belongs to.
Comment 39 Mike Conley (:mconley) - (Needinfo me!) 2012-03-28 09:27:15 PDT
Created attachment 610166 [details] [diff] [review]
Mozmill test case v3 (r+ from bienvenu - check-in after patch goes in)

Thanks bienvenu - fixed in this version.
Comment 40 neil@parkwaycc.co.uk 2012-03-28 10:08:58 PDT
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".
Comment 41 :aceman 2012-03-28 11:02:02 PDT
Created attachment 610208 [details] [diff] [review]
patch v5

r=mconley.

So were you able to fix the mozmill test?
Comment 42 Mike Conley (:mconley) - (Needinfo me!) 2012-03-29 14:37:30 PDT
(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
Comment 43 David :Bienvenu 2012-03-29 15:00:37 PDT
(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.
Comment 44 Mike Conley (:mconley) - (Needinfo me!) 2012-03-29 15:48:25 PDT
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.
Comment 45 Mike Conley (:mconley) - (Needinfo me!) 2012-03-29 15:52:12 PDT
Patches consolidated and landed in comm-central as http://hg.mozilla.org/comm-central/rev/f82efc4704bb .

Note You need to log in before you can comment on or make changes to this bug.