illegal LANGUAGE command

VERIFIED FIXED

Status

MailNews Core
Networking: IMAP
P3
critical
VERIFIED FIXED
18 years ago
9 years ago

People

(Reporter: John G. Myers, Assigned: Scott MacGregor)

Tracking

({regression})

Trunk
x86
All
regression

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta2+])

Attachments

(1 attachment)

(Reporter)

Description

18 years ago
Looking at a protocol telemetry log from a day-old personal build, I see:

LOGIN jgmyers 2000/7/18 17:19:02
>0.000>1 OK User logged in
<0.007<2 XSERVERINFO MANAGEACCOUNTURL MANAGELISTSURL MANAGEFILTERSURL
>0.000>* XSERVERINFO MANAGEACCOUNTURL {74}
http://jgmyers@we-gotmail.red.iplanet.com:50001/bin/user/admin/bin/enduser 
MANAGELISTSURL NIL MANAGEFILTERSURL NIL
2 OK Completed
<0.163<3 LANGUAGE chrome://navigator/locale/navigator.properties
>0.000>3 NO Unrecognized language
(Assignee)

Comment 1

18 years ago
wow that looks pretty horrible. The language string is actually just a pref we
read from the client that stores their preferred language. Sounds like this pref
has been corrupted with a properties string.

cc'ing momoi. Kat, did anyone mess around with the accept charset pref recently? 

Comment 2

18 years ago
This looks like a problem caused by tao/ftang's recent check in
to move all localizable preference items from all.js into
.properties files. The accept-language preference has moved to

chrome://navigator/locale/navigator.properties

Tao, please write here what changes need to be made. 
Language exntension feature is crucially dependent on 
receiving correct Accept-Language value.
(Assignee)

Comment 3

18 years ago
Yeah it looks like the value of the pref is now coming out as a chrome url. 

Comment 4

18 years ago
Any idea where this accept-language is referenced in this code. The caller
should call Pref::getLocalizedUnicharPref() to get the value from all.js and
call SetUnicharPref() to set it.

You can use lxr to find usage of this new api. Let me know if I can be of any
help.

Thanks
(Assignee)

Comment 5

18 years ago
Yes we use it in mailnews in at least two places. Let me get this straight, you
guys moved the location of this pref, forcing it to use a new API and didn't fix
up all the callers? This seems like a disastrous kind of thing to be doing less
than a week away from beta2 freeze.

Did the review process not catch this either?

I'll start searching through lxr to look for any other places where this didn't
get fixed either. I see at least two places in mail where this pref is now broken.

c'mon guys we gotta be more careful if we are going to make 8/3. 
(Assignee)

Comment 6

18 years ago
Nominating for nsbeta2. We can't ship a mail product where the accept language
useage parameter is completely broken. 
Keywords: nsbeta2, regression
OS: Linux → All

Comment 7

18 years ago
My apology. I obviously overlook the usage in mailnews module. Are these the two
you are referring to?

http://lxr.mozilla.org/mozilla/source/mailnews/base/util/nsMsgI18N.cpp#729
http://lxr.mozilla.org/mozilla/source/mailnews/imap/src/nsImapProtocol.cpp#4230

I obviously overlooked them in making the changes.

This is certainly a nsbeta2. I should be more than happy to review your patch
or submit a patch to you for review.

Let me know either way.

Thanks
(Assignee)

Comment 8

18 years ago
Tao, can you please search through lxr for any other prefs whose location you
and ftang moved to make sure we didn't miss any more? I'd hate to have any
suprises next week after the tree gets locked down. Thanks!

I'll post a patch for this pref to this bug report.
Severity: normal → critical
(Assignee)

Comment 9

18 years ago
Created attachment 11551 [details] [diff] [review]
proposed fix submitted for code review

Comment 10

18 years ago
Hi, Scott:


Looks good. One minor point:

I notice that there are two instances of calling AssignWithConversion() to
assign the retrieved accept-pang to nsCString.

  >>      extractedLanguage.AssignWithConversion(acceptLanguages);

In the case of accept-lang pref, it is fine since the langcode wouldn't be
multi-byte encoded. However, in other cases, you might want to convert it
w/ NS_ConvertUCS2toUTF8() before passing it as (char*) arg.

And, yes, I will use lxr to search if any pref moved are calling the right
pref api.

Sorry for the oversight.



Comment 11

18 years ago
Changed QA contact to myself as this requires checking with
an International feature with an IMAP server.
QA Contact: lchiang → momoi

Comment 12

18 years ago
This bug is actually a dup of 39790. Some usage of the accept-lang pref were
not moved! Should we mark it so and use 39790 (nsbeta2+) to check in the fix?
Should save PDT a few minutes in triage.

IT's your call!
(Assignee)

Comment 13

18 years ago
I was thinking about this some more last night and I'm wondering why this pref
is localized? I'm sure you guys know best I'm just confused about the rationale.
The accept language pref is used to set the x-accept-language header on outgoing
mail. It's also used as part of the LANGUAGE extension for imap. And it's used
by http as well. In all of these cases I believe the language field is supposed
to be specified according to RFC 1766. This means strings like "ja", "en", etc.

Do you really want to start passing localized strings to the servers now? I
highly doubt http and imap servers are really going to properly recognized a
japanese localized string for "ja".

Am I way off base here?

Comment 14

18 years ago
HI, Scott:

See my comments below.

>I was thinking about this some more last night and I'm wondering why this pref
>is localized? I'm sure you guys know best I'm just confused about the 
>rationale.
>The accept language pref is used to set the x-accept-language header on 
>outgoing
>mail. It's also used as part of the LANGUAGE extension for imap. And it's used
>by http as well. In all of these cases I believe the language field is supposed
>to be specified according to RFC 1766. This means strings like "ja", "en", etc.

>Do you really want to start passing localized strings to the servers now? I
>highly doubt http and imap servers are really going to properly recognized a
>japanese localized string for "ja".

Your concern is valid. However, localization does not always mena translation.
The "localized" accept-language string for Japanese build will be "ja" as
opposed to its en-US counter part, "en". The primary purpose of accpet-lang is
for server content negotiation. 

Our web servers and mail server/client do reference this value for content
negotiation and message encoding/decoding (?).

BTW, do you consider this bug a dup,  blocker, or dependent of 39790?


thx
(Assignee)

Comment 15

18 years ago
Ahh okay that makes sense. Thanks for explaining that part. Let's just keep it a
separate bug since this fix isn't in that bug and I already have a fix in hand
for it. 

Comment 16

18 years ago
I'll mkae this a blocker of 39790, then.
Blocks: 39790

Updated

18 years ago
No longer blocks: 39790

Comment 17

18 years ago
Putting on [NEED INFO] radar. ftang will look into this and get back to us.
Whiteboard: [NEED INFO]
(Assignee)

Comment 18

18 years ago
I talked to PDT to make sure I could check this in. Since the meta bugs beta2+
this one is too. Adding the plus to this bug per my discussion with PDT and
clearing the need info comment.

Checking in the fix now. Thanks for the code review Tao.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Whiteboard: [NEED INFO] → [nsbeta2+]

Comment 19

18 years ago
** Checked with 7/25/2000 Win32 build **

Language extension is once again working. 
I have a server set up to to be able to deliver IMAP errors in Japanese if
the client sends a query for language with "ja" Accept-Language inquiry.
You won't get an error message in Japanese unless this Accept-Language info is correctly
sent. 
With the above build, I got the server to send me a Japanese error message.
This is good enough proof that we are seniding the right value.

Marking the fix verified.
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.