506199 - Filter rules dialog is corrupted when there are many rules (Error: Missing custom search term undefined, Error: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIStringBundle.GetStringFromName] at mailWidgets.xml::get_valueLabel::line 1086)

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[stepand76]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; cs; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1 (.NET CLR 3.5.30729)
Build Identifier: 3.0b3

Please see the steps to reproduce.

Reproducible: Always

Steps to Reproduce:
1. Create filter with many rules (emails). In msgfilterRules.dat it should be something like:

version="9"
logging="yes"
name="test"
enabled="yes"
type="17"
action="Move to folder"
actionValue="mailbox://nobody@Local%20Folders/Trash"
condition="AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND (from,contains,[e-mail]) (from,contains,[e-mail])"

2. Open the filter dialog again. It looks like in attached screenshot.

Comment 1

[stepand76]

Attachment: Corrupted Filter Rules Dialog

Comment 2

[[:Aureliano Buendía]]

Could you try to see if it still happens with (3.0b4pre)
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-comm-1.9.1/ ?

It seems an older regression, but I'm not sure.

Comment 3

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #0)
> (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND
> (from,contains,[e-mail]) (from,contains,[e-mail])"

Really no "AND" between (from,contains,[e-mail]) and (from,contains,[e-mail])? Or simply pasting miss?
(from,contains,[e-mail]) is lost in msgFilterRules.dat?  

Can you attach msgFilterRules.dat file to this bug?

Comment 4

[stepand76]

Attachment: Corrupted Filter Rules Dialog

Corrupted Filter Rules Dialog (missing row 18).

Comment 5

[stepand76]

Attachment: msgFilterRules.dat

corresponding msgFilterRules.dat

Comment 6

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #5)
> msgFilterRules.dat

All of 1 to 19 is properly held in msgFilterRules.dat.
> AND (from,contains,[e-mail])
>                        |
> AND (from,contains,[e-mail])

(In reply to comment #4)
> Corrupted Filter Rules Dialog (missing row 18).
(In reply to comment #0)
> (from,contains,[e-mail]) AND (from,contains,[e-mail]) AND
> (from,contains,[e-mail]) (from,contains,[e-mail])"

It looks that "AND (from,contains,[e-mail]) AND" is lost, if msgFilterRules.dat is saved after the "Corrupted Filter Rules Dialog (missing row 18)".

Comment 7

[stepand76]

I cannot close corrupted dialog. It says "This filter cannot be saved because searched expression is invalid in current context". You are able to close it?

Note: I have translated this message from czech language.

Comment 8

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #7)
> I cannot close corrupted dialog. It says "This filter cannot be saved because
> searched expression is invalid in current context". 

When/How msgFilterRules.dat entry of comment #0, filter rule of lost "AND (from,contains,[e-mail]) AND", was generated? 

> You are able to close it?

I don't try to duplicate the problem with attached msgFilterRules.dat(contains long filter rule but single filter rule only) yet.

How many filter rules are defined in real msgFilterRules.dat? The filter rule you attached to comment #5 only? What is file size of msgFilterRules.dat?

Comment 9

[stepand76]

(In reply to comment #8)
> When/How msgFilterRules.dat entry of comment #0, filter rule of lost "AND
> (from,contains,[e-mail]) AND", was generated? 

I'm not sure. Now I cannot repeat it. Sorry.

> I don't try to duplicate the problem with attached msgFilterRules.dat(contains
> long filter rule but single filter rule only) yet.
> 
> How many filter rules are defined in real msgFilterRules.dat? The filter rule
> you attached to comment #5 only? What is file size of msgFilterRules.dat?

Yes I can reproduce corrupted dialog with msgFilterRules.dat attached in comment #5. I put this file to my profile, restarted TB and seen corrupted dialog as in attached screenshot. Dialog cannot be confirmed because of message (see commen 7). msgFilterRules.dat is not changed. So there is no data lost, but filter dialog is unusable.

Comment 10

[WADA:World Anti-bad-Duping Agency]

I could observe Corrupted Filter Rules Dialog (missing several rows) with 2009/7/15 build, using msgFilterRules.dat you attached.
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1pre) Gecko/20090715 Shredder/3.0b3pre

Following two errors were displayed for each missing row at Error Console.

(1)
> Error: Missing custom search term undefined
(2)
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIStringBundle.GetStringFromName]"
> nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame ::
> chrome://messenger/content/mailWidgets.xml :: get_valueLabel :: line 1086"  data: no]

Corruption of msgFilterRules.dat in comment #0 is probably produced by older builds(worse builds than current).

Confirming.
I'm not sure this bug is regressions from Bug 474701, but set Bug 497199 in Blocks:. Remove it if wrong, please.

Comment 11

[WADA:World Anti-bad-Duping Agency]

Same result as Comment #10 with 2009/7/24 build(1.9.1).

Comment 12

[Magnus Melin [:mkmelin]]

Opening the filter dialog i get 
Error: Missing custom search term undefined

But nothing else, the filter is savable.

stepand76: did you try http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-comm-1.9.1/ ?

Comment 13

[stepand76]

(In reply to comment #12)
> stepand76: did you try
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-comm-1.9.1/ ?

I tried it. It is the same.
Magnus, did you tried to scroll to the bottom of filter dialog? Are all of rows visible and correct?

Comment 14

[Magnus Melin [:mkmelin]]

I just tried the test case which has one rule, so there's no scrolling to be done.

Comment 15

[stepand76]

(In reply to comment #14)
> I just tried the test case which has one rule, so there's no scrolling to be
> done.
It only happens when there are more rules defined (see the title of this issue). Please try the attached msgFilterRules.dat and check the dialog (see attached screenshot).

Comment 16

[Magnus Melin [:mkmelin]]

Yes it's the attached msgFilterRules.dat testcase that only shows one row - which is a bug of course...

Comment 17

[Kent James (:rkent)]

This is probably my regression, so taking.

Comment 18

[Kent James (:rkent)]

I can't seem to reproduce any of the symptoms of this bug - which makes it hard for me to fix. My original fear is that it was a regression from bug 495519, which is in TB 3.0 beta 3 but not in TB 3.0 beta 2. Changes in that bug now check for invalid filters, and give the message that was quoted earlier "This filter cannot be saved because searched expression is invalid in current context" if the filter is invalid. If the bug can be reproduced, it would be interesting to see if it is in TB 3 beta 2 (though it would not have that error message).

Or can anyone give me some hints of how to reproduce this? I loaded, displayed, and edited the attached msgFilterRules.dat just fine.

Comment 19

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #18)
> Or can anyone give me some hints of how to reproduce this? I loaded, displayed,
> and edited the attached msgFilterRules.dat just fine.

Number of conditions in the test case is 19. All of 19 rows is displayed without scroll bar in your environment?

I use 1600x768 panel with MS Win. Max row is 18 in my environment.
(a) Window height of min or max (1 or 18 rows with scroll bar, 18 or one row is hidden or partially displayed).
    => Only on set of exceptions of comment #10.
(b) Medium window height. 8 rows with scroll bar. No partially displayed row.
    => Eight sets of exceptions of comment #10.

Comment 20

[Kent James (:rkent)]

Well they are displayed with a scroll bar, but scrolling works ok. I'm on a 1024x768 windows XP.

Comment 21

[WADA:World Anti-bad-Duping Agency]

I cordoned window height for all 19 rows by moving Task Bar from bottom to right of Desktop(MS Win-XP SP3). 
All 19 rows were displayed without scroll bar, and no error occurred. If height is reduced to medium size(8 rows) and window is closed, eight sets of exception appear in Error Console again upon next "Edit" of same message filter rule.

Comment 22

[stepand76]

(In reply to comment #18)
> Or can anyone give me some hints of how to reproduce this? I loaded, displayed,
> and edited the attached msgFilterRules.dat just fine.

There is a video demonstrating the bug. 
www.reliance.cz/temp/CorruptedMsgFilterRulesDialog.zip (12MB)
Sorry for print size...
I hope this helps.

Comment 23

[Kent James (:rkent)]

Looking at the video, I'm going to make a wild guess that this bug is related to timing of XBL application, and that the XBL bindings are not getting applied. (On my system, the scrolling is very slow, but the results are OK).

But I still cannot reproduce this. I'm going to remove my name from the ASSIGN for that reason for now, and because XBL issues are not my area of expertise. It would be better if someone who can reproduce this worked on it.

I would still be interested to know if this is a regression from my bug 495519, which landed 2009-07-07 and is in beta 3.

Comment 24

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

This does not sound like a folder display-related regression, removing from blocks list.

Comment 25

[stepand76]

(In reply to comment #24)
> This does not sound like a folder display-related regression, removing from
> blocks list.

Should I understand that it will not be fixed? Filter dialog is unusable for me. Now I must edit msgFilterRules.dat file manually instead.

Comment 26

[Kent James (:rkent)]

(In reply to comment #25)
> 
> Should I understand that it will not be fixed? Filter dialog is unusable for
> me. Now I must edit msgFilterRules.dat file manually instead.

I think what Andrew and I each are saying is that the character of the bug so far does not indicate it to be a regression from one our our recent patches, and so it is likely to be a long-standing (though perhaps rare) bug. I would be happy to fix it if I could reproduce it. What the bug now needs is more QA, that is perhaps a regression range, or clearer STR.

Comment 27

[WADA:World Anti-bad-Duping Agency]

Regression range.
(Phenomenon of comment #10 with attached msgFilterRules.dat doesn't occur.)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1pre) Gecko/20090707 Shredder/3.0b3pre
(Phenomenon of comment #10 with attached msgFilterRules.dat started to occur.)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1pre) Gecko/20090708 Shredder/3.0b3pre

Comment 28

[WADA:World Anti-bad-Duping Agency]

Change log around 7/07.
> http://hg.mozilla.org/comm-central/log/9c9440b5a999
Problem started after next change?
> db4d4379d404: Bug 495519 - "Allow extensions to add custom search terms"
>               Tue, 07 Jul 2009 20:14:40 +0100 - rev 3031

Comment 29

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #26)
> clearer STR.

To see exception, (1) save attached msgFilterRules.dat and (2) Edit the filter rule via Tools/Message filter(scroll bar appears), is sufficient for bug opener and me. Too see grayed-out/blank row, (3) scroll at filter edit panel, is sufficient for us.

As I wrote, phenomenon(number of exceptions, number/position of grayed out rows, ...) depends on number of rows and height of filter edit panel.
19 rows may be too small for your environment.
Kent James, you can't reproduce problem even with more rows than 19?
> condition="OR (from,contains,1) OR (from,contains,2) OR ... OR (from,contains,98) OR (from,contains,99)"
> or
> condition="OR (subject,contains,1) OR (subject,contains,2) OR ... OR (subject,contains,98) OR (subject,contains,99)"

Comment 30

[Kent James (:rkent)]

Well you seem to be pointing the finger pretty effectively at my bug 495519 as the cause. Let me try harder to reproduce, maybe run on some different machines.

Comment 31

[Kent James (:rkent)]

OK, I can see it on my development machine. I'll investigate again.

Comment 32

[Kent James (:rkent)]

Attachment: use getAttribute("value") instead of .value [Checkin: Comment 42]

This fixes the problem for me. I suppose when you create a new XBLified element like menulist, you can't use the XBL binding right away. I'll check it again before I ask for review.

Comment 33

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

(In reply to comment #32)
> This fixes the problem for me. I suppose when you create a new XBLified element
> like menulist, you can't use the XBL binding right away. I'll check it again
> before I ask for review.

What I understand is that for dynamically created nodes that want XBL styling, the XBL bindings aren't bound until the widget actually gets displayed.  This is particularly problematic when you are relying on XUL properties that automatically expose the values of DOM attributes.

Another apparent gotcha is that you should not rely on XUL destructors...

Comment 34

[Kent James (:rkent)]

Attachment: centralized isCustomTerm, added false check

Another aspect of this bug was the false "Missing Custom Term" notice. I centralized the checking of whether an item is a custom term, then checked the value for true as well. So "undefined" and "null" will no longer get reported as a missing custom term, which will confuse the vast majority of users who have never heard of custom search terms. So, in this case, without the fix from my previous patch but with the centralized isCustomTerm, the error of this bug would still occur, but it reports "Subject" as the search term, and has an invalid operator. Still an error, but less confusing.

Comment 35

[David :Bienvenu]

Comment on attachment 394324 [details] [diff] [review]
centralized isCustomTerm, added false check

looks OK to me, pending Neil's r...

Comment 36

[[e-mail]]

What's the source of the undefined/null that you're working around?

Comment 37

[Kent James (:rkent)]

Just to be clear, this bug is solved through the changed line:

this.value = newSelection.getAttribute("value");

The working around the null is designed to get rid of misleading error messages.

"What's the source of the undefined/null that you're working around?"
The undefined/null should not occur if everything works correctly. In this bug, it seems like layout in its infinite wisdom decided to preload some of the offscreen lines in the message filter editor, thus kicking off the XBL constructor. For some reason that I cannot fully explain, those offscreen search rows did not get fully loaded before the newSelection.value was hit. That value should have been an integer, instead it came out as undefined which was then interpreted (falsely) as a custom search term.

The point of all of the working around is just to keep the user from seeing "Custom Search Term" when they have no knowledge of such things. The point of the normal "Missing Custom Search Term" message is for the case where users have added a custom search term through an extension, and then unloaded that extension. At that point they presumably know at least a little about the existance of custom search terms, and so the message is more appropriate.

As a secondary goal, I'm contemplating forcing some structure on the custom search term id string, so that we can tell the difference between a missing custom term and random garbage. By centralizing the detection of custom terms, I make that easy to do if I decide to do it.

Comment 38

[[e-mail]]

(In reply to comment #37)
> "What's the source of the undefined/null that you're working around?"
> The undefined/null should not occur if everything works correctly. In this bug,
> it seems like layout in its infinite wisdom decided to preload some of the
> offscreen lines in the message filter editor, thus kicking off the XBL
> constructor. For some reason that I cannot fully explain, those offscreen
> search rows did not get fully loaded before the newSelection.value was hit.
Actually the blame lies fairly and squarely on listboxes, which do weird things that stop XBL from taking effect when you expect. Indeed, you can't even reliably programmatically select an off-screen listitem.

> The point of all of the working around is just to keep the user from seeing
> "Custom Search Term" when they have no knowledge of such things.
I'm not sure swapping it for a misleading "Subject" error is an improvement.

Comment 39

[Kent James (:rkent)]

> I'm not sure swapping it for a misleading "Subject" error is an improvement.

IMHO error messages that complain about obscure features that the user has never heard of, or that falsely "interpret" the problem, are worse. "Subject" occurs because XPCOM interprets the undefined as 0, which is the attribute value for subject. But the operator portion is not as "lucky", and ends up blank.

If this error handling is going to be controversial, perhaps we should split it into a separate bug, and get the real fix landed. The previous patch (attachment 394238 [details] [diff] [review]) is all that is needed, you could review that instead.

Comment 40

[Kent James (:rkent)]

Comment on attachment 394238 [details] [diff] [review]
use getAttribute("value") instead of .value
[Checkin: Comment 42]

Carrying over bienvenu's sr. Patch to land.

Comment 41

[Kent James (:rkent)]

See bug 510603 -  Review error handling of custom search terms and filter actions.

Comment 42

[Serge Gautherie (:sgautherie)]

Comment on attachment 394238 [details] [diff] [review]
use getAttribute("value") instead of .value
[Checkin: Comment 42]


http://hg.mozilla.org/comm-central/rev/1d473ed592a0