549336 - Custom filters generate an error in searchSpec

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Kent James (:rkent)]

From a report in my FiltaQuilla forum:

I have try to test supper interesting regexp filtering but unfortunately on some folders I receive next error:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIMsgSearchValidityTable.getAvailable]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///K:/ThunderbirdPortable3/App/thunderbird/modules/searchSpec.js :: SearchSpec_applySearch :: line 417"  data: no]
It happens on regexp and JavaScript filters (I think they are near the same in internal source code)
TB 3.0.2 with IMAP folders.

The issue here is that the .getAvailable check in searchSpec.js does not specifically support custom filters, as it should. My bad, since I added that in the first place!

Comment 1

[Kent James (:rkent)]

Attachment: look for custom terms when doing getAvailable

The issue here is that I needed to check for custom search terms prior to doing the getAvailable checks. This is a simple patch that does this.

The patch can be tested manually using the FiltaQuilla extension, with a Subject Regex search term. Open the advanced search dialog, pick "Subject Regex" "Matches" "test", search and you will get the errors without this patch.

I'm not really sure how to go about doing automated tests for this. I guess it is a mozmill project, but I am not qualified to do those yet. I'll let the review guide whether this can be accepted without an automated test.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 435362 [details] [diff] [review]
look for custom terms when doing getAvailable

This isn't a mozmill testing project.  searchSpec exists at the dbViewWrapper layer of abstraction; there are xpcshell dbViewWrapper tests in mail/base/test/unit/.

nsIMsgSearchSession exposes the search scopes via getNthSearchScope so it seems like you should be able to define one or more custom terms and then verify that the logic does what is expected.

I'm assuming there's already some test code for the custom filters so this shouldn't be too much work.  (The test dosn't need to be a fancy end-to-end test, I'd just like to have verified branch coverage.  Clobbering the internal state of the search scope / db view wrapper is even cool to set things up for minimal work.)

Comment 3

[Kent James (:rkent)]

Thanks for looking at this so quickly. I'll see what I can do with xpcshell tests.

I've just added body regular expression search to FiltaQuilla as a custom search term, which is a much requested (104 votes in bug 19442) feature. This bug is the only thing that stands in the way of using those also in advanced search, so I really want to get this in 3.1 and possibly in 3.0.* since it is really a bug and not a feature.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Indeed I expect a lot of people will be very happy to have that feature available to them!  Getting this in if tests are provided should not be a problem.

As an aside, have you noticed any problems from the additional GC complications of implementing search terms in javascript?  (At least I am assuming you are implementing the search terms in javascript...)

Comment 5

[Kent James (:rkent)]

Attachment: add unit tests

I spent many hours figuring out that I needed the "yield false" after "make_virtual_folder" in the unit test. The test I copied from, which did not use the imap async folder create, did not have that. You might confirm that it makes sense to you as well to do that. Without the yield false, "yield wait_for_async_promises();" in add_sets_to_folders was continuing after the folder loaded message, and not after the OnStopRunningUrl like is needed.

I also confirmed bienvenu's metric here, that it takes 10x the effort to write the unit test as it does to fix the bug. And that was for a case where the unit testing framework already existed, though I had to use it in a unique way, and therefore solve some new problems, since the existing tests did not use imap. I have mixed feelings about whether that is the best approach to simple bugs like this.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

I think I've agreed elsewhere that the current explicit async style turns out to be conceptually expensive to work with, especially since a lot of operations when working with local injection are not asynchronous and so corners can be cut which explode if the code is copied and pasted into an IMAP context.  The solution is the mozmill event-loop spinning idiom (although that will have its own jabby edges, of course), but that's not a small undertaking and so needs a really good amortized justification.

The place you actually need the yield is after the call to make_folder_with_sets and it should be a "yield wait_for_message_injection();" so that the code works in the local case as well should someone down the road decide to copy and paste that code.  The documentation I previously wrote at your request demonstrates this:
https://developer.mozilla.org/en/MailNews/AsyncTestUtils_Extended_Framework#Creating_.2f_injecting_messages

Sadly, the actual function documentation for make_new_sets_in_folders assumes some degree of psychic powers or that the user has read its source code and noticed the call to add_sets_to_folders and then read that function's documentation which, happily, does mention the asynchronous contract going on.

(In reply to comment #5)
> I also confirmed bienvenu's metric here, that it takes 10x the effort to write
> the unit test as it does to fix the bug. And that was for a case where the unit
> testing framework already existed, though I had to use it in a unique way, and
> therefore solve some new problems, since the existing tests did not use imap. I
> have mixed feelings about whether that is the best approach to simple bugs like
> this.

I think everyone is open to any approach that lets us fix things and avoid regressions in the future while taking less effort.  tb-planning is probably the right place for such proposals.

I agree that the patch itself is reasonably straightforward.  I'm assuming that you verified this made your extension happy and so the chances of flubbed logic, at least on the hot code path, are nil.  But that does not change the fact that we need a test to avoid regressions of this functionality in the future.  The fact that any patch was required is proof of it.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 435532 [details] [diff] [review]
add unit tests

Mainly nits.


on file: mail/base/modules/searchSpec.js line 418
>             if (term.attrib == Ci.nsMsgSearchAttrib.Custom)
>             {

same-line squiggly brace


on file: mail/base/modules/searchSpec.js line 430
>             else
>             {

same line squiggly brace


on file: mail/base/test/unit/test_viewWrapper_virtualFolderCustomTerm.js line 27
>   getEnabled: function subject_getEnabled(scope, op)
>   {

same line squiggly braces for the functions around here, please.


on file: mail/base/test/unit/test_viewWrapper_virtualFolderCustomTerm.js line 63
>   yield false;

As noted in my previous comment, this should be "yield
wait_for_message_injection();" and should happen after the call to
make_folder_with_sets, not here.

Comment 8

[Kent James (:rkent)]

Attachment: Fixed nits, patch to checkin

Comment 9

[Kent James (:rkent)]

Comment on attachment 435962 [details] [diff] [review]
Fixed nits, patch to checkin

Checked in http://hg.mozilla.org/comm-central/rev/dff247dd5353

Comment 10

[Kent James (:rkent)]

This is a bug fix not a feature, so I am requesting it for Thunderbird 3.05 as well.

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #0)
> From a report in my FiltaQuilla forum:
> 
> I have try to test supper interesting regexp filtering but unfortunately on
> some folders I receive next error:
...
> The issue here is that the .getAvailable check in searchSpec.js does not
> specifically support custom filters, as it should. My bad, since I added that
> in the first place!

Apart from the errors, what's the user-visible impact of this bug?

Comment 12

[Kent James (:rkent)]

Without the patch, custom search terms fail in non-filter contexts, such as advanced search or virtual folders, since the error causes the javascript to abort at that point.

The new code is only executed "if (term.attrib == Ci.nsMsgSearchAttrib.Custom)" which means if the search uses the custom search term feature defined in nsIMsgSearchCustomTerm.idl There is no use of this in the core code, the only use I am currently aware of is in my FiltaQuilla extension. So this patch should have no impact at the moment to any user except those who are trying to use some features in my FiltaQuilla extension. (A typical example of use would be a new search term "Subject Regex" which does a search for a regular expression in the subject of the email.)

So I think this patch is low risk, but also low impact.

Comment 13

[Kent James (:rkent)]

Comment on attachment 435962 [details] [diff] [review]
Fixed nits, patch to checkin

Checked in http://hg.mozilla.org/releases/comm-1.9.1/rev/a7606c8630d2