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)
Thunderbird
Address Book
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 14.0
People
(Reporter: neil, Assigned: aceman)
References
Details
Attachments
(2 files, 6 obsolete files)
5.90 KB,
patch
|
Bienvenu
:
review+
neil
:
feedback+
|
Details | Diff | Splinter Review |
2.87 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
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) {} }
Reporter | ||
Comment 2•12 years ago
|
||
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?
Reporter | ||
Comment 4•12 years ago
|
||
(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'?
Reporter | ||
Comment 6•12 years ago
|
||
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.
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.
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)
Assignee | ||
Comment 10•12 years ago
|
||
Fixed patch, left debugging in there.
Attachment #598403 -
Attachment is obsolete: true
Attachment #598406 -
Flags: feedback?(mconley)
Attachment #598403 -
Flags: feedback?(mconley)
Comment 11•12 years ago
|
||
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?
Assignee | ||
Comment 12•12 years ago
|
||
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•12 years ago
|
||
If that's the case, I'd prefer to excise the code, instead of extending its life. :)
Assignee | ||
Comment 14•12 years ago
|
||
Isn't there any test? :)
Comment 15•12 years ago
|
||
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?
Assignee | ||
Comment 16•12 years ago
|
||
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•12 years ago
|
||
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+
Assignee | ||
Comment 18•12 years ago
|
||
So actually the bug was that the original 'subdirectory.indexOf' should have been 'subdirectory.addressLists.indexOf' and all would be solved?
Comment 19•12 years ago
|
||
Yes - though I think you're right to check to ensure that the subdirectory is a mailing list before doing the check.
Assignee | ||
Comment 20•12 years ago
|
||
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 21•12 years ago
|
||
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-
Assignee | ||
Comment 22•12 years ago
|
||
(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•12 years ago
|
||
(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.
Assignee | ||
Comment 24•12 years ago
|
||
Attachment #606893 -
Attachment is obsolete: true
Attachment #607304 -
Flags: review?(mconley)
Comment 25•12 years ago
|
||
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
Assignee | ||
Comment 26•12 years ago
|
||
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•12 years ago
|
||
aceman: What happens when, from your objdir, you type in the following: make SOLO_TEST=addrbook mozmill-one ? -Mike
Assignee | ||
Comment 28•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Comment on attachment 607304 [details] [diff] [review] patch v4 See above.
Attachment #607304 -
Flags: review?(mconley) → review-
Assignee | ||
Comment 31•12 years ago
|
||
So how comes the patch fixes it for me? Can you specify the STR more precisely?
Comment 32•12 years ago
|
||
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".
Assignee | ||
Comment 33•12 years ago
|
||
Ok, I'll try that. But those STR are quite different to what we talked about since comment 15.
Comment 34•12 years ago
|
||
(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.
Assignee | ||
Comment 35•12 years ago
|
||
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•12 years ago
|
||
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 37•12 years ago
|
||
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 38•12 years ago
|
||
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+
Comment 39•12 years ago
|
||
Thanks bienvenu - fixed in this version.
Attachment #610117 -
Attachment is obsolete: true
Reporter | ||
Comment 40•12 years ago
|
||
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+
Reporter | ||
Updated•12 years ago
|
Attachment #610166 -
Flags: feedback-
Assignee | ||
Comment 41•12 years ago
|
||
r=mconley. So were you able to fix the mozmill test?
Attachment #607304 -
Attachment is obsolete: true
Attachment #610208 -
Flags: review+
Comment 42•12 years ago
|
||
(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•12 years ago
|
||
(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.
Updated•12 years ago
|
Attachment #610117 -
Attachment is obsolete: false
Comment 44•12 years ago
|
||
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
Comment 45•12 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•