55424 - Message filter: Edit /Delete button is not working with non-ASCII filter name

Status: VERIFIED FIXED

(MailNews Core :: Internationalization, defect, P3)

Description

[ji]

****observed with win32, linux and mac 20001005 branch build*****

On the message filter window, after selecting a filter with non-ASCII filter name, the Edit button is grayed out and it's not 
functioning properly.

Steps to reproduce:
1. Launch Mail.
2. Select Edit | Message Filters
3. Click on New button
4. Create a filter with non-ASCII filter name.
5. Select the non-ASCII filter on the list, you'll see the Edit button is grayed out. If you click on it, it can bring up a new filter 
creation window with empty filter name field and empty drop-down list.

Comment 1

[ji]

Nominating for rtm.

Comment 2

[nhottanscp]

Reassign to alecf.
I think there is a separate bug that non-ASCII filter name does not work.
Jaime, Frank, do we need to support this for RTM?

Comment 3

[ji]

Delete button is not working either. Nothing happens when clicking on Delete button.

Comment 4

[marina]

It's a regression  because it works fine with 2000-09-29 build

Comment 5

[ji]

It started to happen from yesterday's build. (2000100409)

Comment 6

[Jaime Rodriguez, Jr.]

Changing priority to P2.

Comment 7

[Alec Flett]

this is the same problem that we had with local folders
We need that patch from waterson, I'll try to track it down

Comment 8

[Alec Flett]

Attachment: fix from waterson to support utf8 URIs

Comment 9

[Alec Flett]

Attached the patch from waterson. I think it's the right thing, so r=alecf

Chris - this is the patch you gave us a while back to make the template builder 
not escape UTF8 URIs. How do you want to do this? Can you be super reviewer on 
your own patch? :)

Comment 10

[Alec Flett]

oops, chris pointed out this patch is already in.
So I think the reason it isn't working is that I'm not using GetUnicodeResource
where appropriate.. attaching a patch... naoki/ji - can you try this patch and
let me know if it works?

Comment 11

[Alec Flett]

Attachment: use GetUnicodeResource since the URI in the DOM is UTF8-decoded into unicode

Comment 12

[nhottanscp]

I applied the patch (10/06/00 11:06) but looks like the problem is still there.
I got assertions in nsMsgFilterDelegateFactory::getFilterDelegate. The asserton
says the input is not valid UTF-8.

    // XXX convert from UTF8
    nsAutoString filterString;
    PRUnichar *unicodeString =
        nsTextFormatter::smprintf(unicodeFormatter, filterName);
    NS_ENSURE_TRUE(unicodeString, NS_ERROR_OUT_OF_MEMORY);


BTW, since waterson's RDF fix is now in, does that mean bug 54251 is fixed?

Comment 13

[nhottanscp]

I asked about bug 54251 because if that is not fixed, 8 bit filter name doesn't
work anyway even if this bug is fixed.

Comment 14

[nhottanscp]

Before the assertion I mentioned I am getting another one in JSData2Native().
That is getting  URI like "imap://[e-mail];filterName=????",
where ???? is actually unicode in \uE565\u2C67\u9E8A\u6E30. But it was truncated
to 0xE52C9E6E. The UTF-8 assertion is a result of getting this broken string.

I saw the same truncation before, when unicode string was passed to a "string"
version of interface instead of "wstring". For local mail folder, I used URL
escape to avoid that problem.

Comment 15

[selmer (gone)]

removing "fix in hand" from whiteboard, apparently not true at this time.

Is there a new patch?

Comment 16

[Alec Flett]

no new patch at this time, I'm not sure what's going on.

Comment 17

[selmer (gone)]

So, is the current problem that we've incorrectly marked an interface as string
instead of wstring?  Which interface?  Seems like this is potentially a one-liner.

Comment 18

[nhottanscp]

Here are the call stack dumps. JS stack says FilterListDialog.js":122.
120     var filter;
121     try {
122         var filterResource = rdf.GetResource(selection[0].id);

Part of the call stack
NTDLL! 77f7629c()
nsDebug::Assertion(const char * 0x01eed14c, const char * 0x01eed140, const char
* 0x01eed10c, int 0x00000267) line 256 + 13 bytes
XPCConvert::JSData2Native(JSContext * 0x06f81e70, void * 0x0012bab8, long
0x04a3dd64, const nsXPTType & {...}, int 0x00000000, const nsID * 0x00000000,
unsigned int * 0x0012bb48) line 615 + 32 bytes
nsXPCWrappedNativeClass::CallWrappedMethod(JSContext * 0x06f81e70,
nsXPCWrappedNative * 0x046c7280, const XPCNativeMemberDescriptor * 0x034b5354,
nsXPCWrappedNativeClass::CallMode CALL_METHOD, unsigned int 0x00000001, long *
0x04a87178, long * 0x0012bc6c) line 747 + 45 bytes
WrappedNative_CallMethod(JSContext * 0x06f81e70, JSObject * 0x04a3db90, unsigned
int 0x00000001, long * 0x04a87178, long * 0x0012bc6c) line 228 + 34 bytes


JSStack
0 [native frame]
1 currentFilter() ["chrome://messenger/content/FilterListDialog.js":122]
    selection = [object NodeList]
    filter = undefined
    filterResource = undefined
    this = [object Window]
2 updateButtons() ["chrome://messenger/content/FilterListDialog.js":232]
    filter = undefined
    this = [object Window]
3 onFilterSelect(event = [object KeyEvent]) ["chrome://messenger/content/FilterL
istDialog.js":144]
    this = [object Window]
4 onselect(event = [object KeyEvent]) ["<unknown>":0]
    this = [object XULTreeElement]
5 [native frame]
6 onxblmousedown(event = [object KeyEvent]) ["<unknown>":4]
    isSelected = ""
    this = [object XULElement]
7 <TOP LEVEL> ["<unknown>":0]

Comment 19

[nhottanscp]

Changed rdf.GetResource

Changed rdf.GetResource to rdf.GetUnicodeResource seems to fix the problem.
Edit/Delete filter works with Japanese name. The filter itself also functional.

Comment 20

[nhottanscp]

But GetUnicodeResource change was actually included in Alec's patch.
I will apply the patch and try again.

Comment 21

[nhottanscp]

I applied the Alec's path and it's working fine for Japanese filter names.
Must be something wrong in my build last time I tried. Sorry about the confusion.

Comment 22

[Alec Flett]

we have a fix now (the second patch in this bug) so marking rtm+ like it was
before

Comment 23

[Phil Peterson]

PDT marking [rtm-] because it's time to focus on P1 crash/data loss bugs.

Comment 24

[msanz]

Why Phil? The world will have filters with non-ASCII names out of the US, and
this is kind of "get it right the first time or you're stuck with your filter".
It's Tuesday, I don't know about Alec's bug count, but this seems done. Is the
fix risky? Removing the rtm-

Comment 25

[Alec Flett]

this bug is done, the fix is low-risk, and only affects the editing and deleting
of filters, nothing else. I have pleanty of time to check this fix in.

Comment 26

[Alec Flett]

also, the patch is a three-line fix
(it's the second patch listed here)

Comment 27

[selmer (gone)]

rtm-, workaround is to disable the filter, we're looking for crash bugs and
other very serious fixes now.

Comment 28

[msanz]

can this be discussed at today's meeting?

Comment 29

[nhottanscp]

I think using GetUnicodeResource instead of GetResource has no side effects for
7 bit ASCII data (see the implemention below).
Adding waterson to cc. Chris, is there any risk for 7 bit ASCII strings by
switching to GetUnicodeResource?

772 NS_IMETHODIMP 773 RDFServiceImpl::GetUnicodeResource(const PRUnichar* aURI,
nsIRDFResource** aResource)
774 {
775 NS_PRECONDITION(aURI != nsnull, "null ptr");
776 if (! aURI)
777 return NS_ERROR_NULL_POINTER;
778 779 return GetResource(NS_ConvertUCS2toUTF8(aURI), aResource);
780 }
781

Comment 30

[Alec Flett]

msanz: I'm more than willing to discuss this one at the PDT meeting - if pdt is
willing to talk about it, please give me a ring and I'll stop by.

Comment 31

[Alec Flett]

ok, PDT gave me permission - thanks guys. re-marking pdt+

Comment 32

[Phil Peterson]

rtm++ after talking to alecf

Comment 33

[Scott MacGregor]

sr=mscott

Comment 34

[Alec Flett]

fix checked into branch and trunk

Comment 35

[ji]

Verified with Win32, linux, Mac 20001013 branch builds. It is fixed.

Comment 36

[Jaime Rodriguez, Jr.]

I'm happy to see this one fixed.

Comment 37

[Katsuhiko Momoi]

This is fixed on Windows.
ji, can you look at this on Linux.
I'll look at Mac.

Comment 38

[ji]

Yes, this is working with linux 2000110409-mn6 build.

Comment 39

[laurel]

OK using:
2000-12-14-04 commercial trunk build win98
2000-12-14-11 commercial trunk build linux rh6.0
2000-12-14-08 commercial trunk build mac OS 9.0

Comment 40

[Jaime Rodriguez, Jr.]

Removing jaimejr and adding jenm.