1328237 - Search Messages window returns (NS_ERROR_FAILURE) [nsISupportsArray.RemoveElement]

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Mason]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0 SeaMonkey/2.47
Build ID: 20161201022155

Steps to reproduce:

I use the Search Messages window to search for messages between two dates. At some point, clicking the "Search" button no longer launches a new search.

Error console contains:

Timestamp: 03/01/2017 11:31:47
Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISupportsArray.RemoveElement]
Source File: chrome://messenger/content/searchTermOverlay.js
Line: 494

I think the trigger might be removing filter conditions, then adding back new ones?

Comment 1

[Nuno Silva [:njsg]]

This is still present, but now it is indexOf that throws NS_ERROR_FAILURE. It affects both the message and address book search windows.

If I'm understanding correctly, there is a separate list of search terms, independent from the UI. When a condition is removed in the UI, it is added to an array of removed terms (gSearchRemovedTerms). This is then used in saveSearchTerms() to update the separate list of search terms.

NS_ERROR_FAILURE will get thrown on the second search after a condition has been removed: by then, gSearchRemovedTerms still has the removed term(s) but those are not present in the separate list anymore, and indexOf() throws this when the provided element is not present (see bug 406336).

Comment 2

[Nuno Silva [:njsg]]

Attachment: 1328237-clear-removed-terms.patch

Discard already processed content in saveSearchTerms(), and add a try-catch block, with an error message similar to the one in the block below. As discussed on IRC, use Cu.reportError() instead of dump() in both catch blocks.

As far as I can tell, there is no need to keep the old contents of gSearchRemovedTerms (did I overlook something?).

I tried to see the evolution of this code (involving rule removal and gSearchRemovedTerms) through time: it appears to have been first added to the filter editor [1], and then moved into this file to be shared with the upcoming search dialog [2]. I wonder if the array contents are retained because, when it was first written, it was not intended to run more than once? (The NS_ERROR_FAILURE exception has only appeared more recently, probably in 2.35 (see bug 1338808 comment 4).)

In Thunderbird and comm-central this line was changed in bug 1681009: searchTerms became a JS array, and filter() is now used instead of indexOf()+removeElementAt().

[1] https://github.com/ehsan/mozilla-cvs-history/commit/e8ba4be30e683516a14be0c53f867c3ae492d8f1
[2] https://github.com/ehsan/mozilla-cvs-history/commit/56497d14b45a905d723c1da970bf9328aa7cb411

Comment 3

[Ian Neal]

Comment on attachment 9286192 [details] [diff] [review]
1328237-clear-removed-terms.patch

>+++ b/mailnews/base/search/content/searchTermOverlay.js
> function saveSearchTerms(searchTerms, termOwner)
> {
>     var matchAll = gSearchBooleanRadiogroup.value == 'matchAll';
>     var i;
>     for (i = 0; i < gSearchRemovedTerms.length; i++)
>-        searchTerms.removeElementAt(searchTerms.indexOf(0, gSearchRemovedTerms[i]));
>+        try {
>+            searchTerms.removeElementAt(
>+              searchTerms.indexOf(0, gSearchRemovedTerms[i]));
Nit: to match the formatting this needs an extra 2 space indent.

Until we backport the changes from Bug 1681009, this looks good.

Comment 4

[Nuno Silva [:njsg]]

Attachment: 1328237-clear-removed-terms.patch-v2

v2: updated with the missing spaces.

Comment 5

[Ian Neal]

Comment on attachment 9287846 [details] [diff] [review]
1328237-clear-removed-terms.patch-v2

[Triage Comment]
Carrying forward a+

Comment 6

[Frank-Rainer Grahl (:frg)]

2.53.14 and up only.
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/62921a248c2a6981ad39b3e2bcb772c4f46ef711
Clear gSearchRemovedTerms after use and handle exceptions. r=IanN a=IanN