167128 - [FIX]Japanese named printer cannot be recognized.

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P2)

Description

[Frank Tang]

here is one japanese bug report we saw about Mozilla 1.0

Operating System: 	Win98
Language: 		Japanese
Issue Summary: 		Problem of print
Component: 		International
Doing What: 		Using Japanese localized
Severity: 		SomethingDidNotWorkRight
Can Reproduce: 		Always
Try this URL: 		http://
Issue Detail:
The printer cannot be recognized that Japanese is
included in the name of the
printer used usually.
Email Address: 		[e-mail]

and our QA cannot create the environment to test it:
Kasumi Ketron wrote:

Unfortunately, we can't confirm it since we can't change  printer name having
DBCS on server.


Jay Yan: Can you folks create printer name in Chinese and help us debug it ?

Comment 1

[Pete Zha]

WFM on Win2000 in Chinese locale
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.0) Gecko/20020530

Comment 2

[Pete Zha]

In comment 1, I just tried to setup local printer's name to let it include some
Chinese character. Mozilla1.0 works well. It can list the printer's name and
print use that printer without problem.

Comment 3

[Louie Zhao]

On Linux, even locale is Chinese, can't change printer name to Chinese
character. And On Solaris, printer was selected from a printer list, which can't
be modified to Chinese character also. That is to say, on Linux or Solaris this
case can't be reproduced. It's a bug only on Windows.

Comment 4

[rods (gone)]

Problem is probably:
http://lxr.mozilla.org/mozilla/source/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp#777
http://lxr.mozilla.org/mozilla/source/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp#891
http://lxr.mozilla.org/mozilla/source/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp#1198
(this isn't turned on yet)

Fix coming....

Comment 5

[rods (gone)]

Attachment: patch

Uses the GetACPString, to get the properly localized string.

Comment 6

[Roy Yokoyama]

Comment on attachment 98462 [details] [diff] [review]
patch 

Here is what I think you should do:

@@ -777,7 +777,10 @@
 #ifdef UNICODE
-     prtName.AppendWithConversion((PRUnichar *)aPrintName);
+     prName.Append(aPrintName);  // <====  since aPrintName is WCHAR when
UNICODE is on.
 #else 
-    prtName.AssignWithConversion((char*)aPrintName);
+    PRUnichar* dbStr = GetUCS2String(nsCString(prtName)); // <==== you need to
write GetUCS2String() which calls MultiByteToWideChar()
+    prtName.Assign(dbStr);
+    free(dbStr);
 #endif

----------------------------------
and for the last section, 

@@ -1198,7 +1204,9 @@
 #ifdef UNICODE
     hGlobalDevMode = CreateGlobalDevModeAndInit(printerName, aPrintSettings);
 #else 
-    hGlobalDevMode =
CreateGlobalDevModeAndInit((char*)NS_ConvertUCS2toUTF8(printerName).get(),
aPrintSettings);
+  char* dbStr = GetACPString(nsString(printerName));  // <==== you can do this
since printerName is PRUnichar*
+  hGlobalDevMode = CreateGlobalDevModeAndInit(dbStr, aPrintSettings);
+  free(dbStr);
 #endif
   }

Comment 7

[rods (gone)]

Attachment: revised patch v2

With Roy's suggestion, plus there is no need to convert the Printer Name using
GetACPString inside CreateGlobalDevModeAndInit, it is always passed in
correctly. 

We could convert the name inside CreateGlobalDevModeAndInit, but all the
platform calls require a LPTSTR, so this is the best way.

Comment 8

[Christopher Blizzard (:blizzard)]

Comment on attachment 98716 [details] [diff] [review]
revised patch v2

sr=blizzard

Comment 9

[Roy Yokoyama]

How about prName?  Do we need to convert to UCS2 by calling MultiByteToWideChar()?

Comment 10

[rods (gone)]

Attachment: patch v3

I have removed the UNICODE #ifdefs (no one compiles with this on and it only
confuses things)

I also found where I was using UTF8 and shouldn't have been.

Comment 11

[Roy Yokoyama]

Comment on attachment 100602 [details] [diff] [review]
patch v3

looks good.
/r=yokoyama

Comment 12

[Christopher Blizzard (:blizzard)]

Comment on attachment 100602 [details] [diff] [review]
patch v3

sr=blizzard

Comment 13

[rods (gone)]

fixed

Note this fixed Bug 169400 which is an issue for 7.0 may want to take this in
the respin)

Comment 14

[Michael Buckland]

Got this nomination from Rod via email.  The Blackbird team (adt) will consider
bugs for the release which meet all of the following criteria:
          o Meet Blackbird release criteria
                + Required feature of the release. 
                + Regression caused by changes made in this release.  
                + Top crash
                + Security
          + Other product dependencies:  I.e., a bug blocking a TBird pull
          o Has baked on the trunk
          o Has a patch for the branch including r= and sr= for that patch
          o Passes additional review as required by Blackbird team
          o Has adt1.0.2 keyword
          o Can be tested, given very limited QA resources in this release

If you believe the proposed fix meets the criteria above, please make sure to
add the keyword adt1.0.2 to the bug.  The team looks at bugs regularly and we
will update the bug with additional information we might need, or else a
decision on this nomination.

Comment 15

[Kevin McCluskey (gone)]

nsbeta1+

Comment 16

[rods (gone)]

Attachment: patch for the 1.0.2 tree

patch for the branch (applied and built directly from the trunk patch)

Comment 17

[Christopher Blizzard (:blizzard)]

Comment on attachment 103347 [details] [diff] [review]
patch for the 1.0.2 tree

sr=blizzard

Comment 18

[dcone (gone)]

Comment on attachment 103347 [details] [diff] [review]
patch for the 1.0.2 tree

r=dcone

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 103347 [details] [diff] [review]
patch for the 1.0.2 tree

a=[e-mail] for 1.0 branch; change mozilla1.0.2+ to fixed1.0.2 when
checked in

Comment 20

[sujay]

Can someone from International please verify this and mark.

Yuying, I have changed you to the qa_contact on this bug.

thanks.

Comment 21

[Yuying Long]

Like the comment in original report, there is no such kind of printer names
existing in our network server, so it's really hard for us to verify this bug
fix here.

Is there any one who can set Japanese printer name can help us to verify it?

Kat, can we find someone in mozilla-Japan to do this? thanks!

Comment 22

[Yuying Long]

OK, I still haven't get any response from some one who has real network Japanese
named printer.

Here is what I did for verification on Win98-Japanese:
1. Change an exisiting network Enlish printer name to Japanese name.
2. Launch 11-07 branch (1.0.2) build, File | Print.

Result:
There is no print dialog show up, but with an error message instead: can't not
find printer.

IE or Wordpad:
The print dialog will pop-up, and I'm able to see the Japanese printer name. 

3. Change the printer name from Japanese to some ascii characters, then try to
print.
- now the print dialog will show-up.

So, is the problem real get fixed?

Comment 23

[rods (gone)]

I guess the good news is, it doesn't crash, because this was cuasing a crash for
a user.

Does a current build behave the same way?

Comment 24

[Yuying Long]

> Does a current build behave the same way?

I was using the lateset 11-07 branch (1.0.2) build, since there is fixed1.0.2
keyword, I assume the fix was checked in branch build, but the result in comment
#22 seems show the problem is not really get resolved. 

Comment 25

[Gayatri Rimola]

So do you want to reopen this bug in the blackbird branch? If yes, please remove
the fixed1.0.2 keyword. If you are satisfied that atleast it does not crash than
mark this bug verified1.0.2 and open a new bug for the symptoms in comment #22.
Thanks.

Comment 26

[Yuying Long]

Based on my test result and the summary of this bug (the Japanese named printer
does NOT be recognized on current build), I'm re-open this bug and remove the
fixed 1.0.2 keyword.

Comment 27

[rods (gone)]

I can back the change out and we can test again?

Comment 28

[rods (gone)]

Yuying, does the current 7.0 release behave better and not crash when you have a
Japanese printer name? If so, then we should back it out.

Comment 29

[Yuying Long]

If there was some one has a crash before this patch, the behave of the latest
branch build is better because I didn't see the crash.

And I tried in on N7.0RTM, the result is the same as in latest branch build.

But in order to verify the real problem, I think we better to have some one who
has the real Japanese network printer name to test it.  Because what I did was
just rename the exisiting english printer name to Japanese, and I got a "not
found" error.

Comment 30

[Kevin McCluskey (gone)]

-> jkeiser

Comment 31

[Kevin McCluskey (gone)]

nsbeta1-. John is overloaded with higher priority issues. 

Adding qawanted to try out a current build on a "real Japanese network printer"

Comment 32

[Prognathous]

We've also encountered reports of unrecognized printers whose name include
Hebrew characters. Judging from this and other bugs (such as Bug 169206), this
issue is not Japanese-specific.

Prog.

Comment 33

[Prognathous]

*** Bug 187399 has been marked as a duplicate of this bug. ***

Comment 34

[Hidehiro Kozawa]

Now WinXP is OK. Win9x is not.
http://lxr.mozilla.org/seamonkey/source/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp#763
I think that 
WinXP cat treat unicode (unicode API OpenPrinter
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/gdi/prntspol_9qnm.asp
) but Win 9x in not.
I think that we need to change here like widget/src/windows/nsToolkit.cpp,
NS_IMM_GETCOMPOSITIONSTRINGW of
widget/src/windows/nsWindows.cpp, etc)
and WinXP/2K use unicode and Win9x use OS native coding that depend on
region/language of OS.

Comment 35

[Hidehiro Kozawa]

Attachment: make convert correct

My guess of comment 34 is wrong.

http://lxr.mozilla.org/seamonkey/source/gfx/src/windows/nsDeviceContextSpecWin.cpp#201

broke the rule of 
http://www.mozilla.org/projects/xpcom/string-guide.html
It is coincidence that it works well recent code on WinXP/JP.

Comment 36

[Hidehiro Kozawa]

Comment on attachment 143086 [details] [diff] [review]
make convert correct

--- nsDeviceContextSpecWin.cpp.org	2004-02-29 07:33:57.000000000 +0900
+++ nsDeviceContextSpecWin.cpp	2004-03-06 15:56:37.656250000 +0900
@@ -50,6 +50,9 @@
 #include "nsReadableUtils.h"
 #include "nsGfxCIID.h"

+// For NS_CopyNativeToUnicode
+#include "nsNativeCharsetUtils.h"
+
 // File Picker
 #include "nsILocalFile.h"
 #include "nsIFile.h"
@@ -198,12 +201,8 @@
   LPTSTR lpPrtName;
   GlobalPrinters::GetInstance()->GetDefaultPrinterName(lpPrtName);
   nsString str;
-#ifdef UNICODE
-  str.AppendWithConversion((PRUnichar *)lpPrtName);
-#else 
-  str.AssignWithConversion((char*)lpPrtName);
-#endif
-  printerName = ToNewUnicode(str);
+  NS_CopyNativeToUnicode(nsDependentCString((char *)lpPrtName), str);
+  printerName = (PRUnichar *)str.get();
   free(lpPrtName);
   return printerName;
 }

Comment 37

[Jungshik Shin]

(In reply to comment #36)

>    nsString str;

> +  NS_CopyNativeToUnicode(nsDependentCString((char *)lpPrtName), str);
> +  printerName = (PRUnichar *)str.get();

You're returning a pointer to a buffer in the stack. You should duplicate the
buffer with ToNewUnicode() here. 

Comment 38

[Hidehiro Kozawa]

Attachment: convert make well

(In reply to comment #37)
Yes I take basically mistake.
It works well on XP and 98Se.

please review it.

Comment 39

[Hidehiro Kozawa]

If Bug 169206 is differnt from this bug I don't know why this bug still opening.

My patch for gfx\src\windows\nsDeviceContextSpecWin.cpp.org seems to be Bug
169206's one.

I don't know how I do.
But I want to review my patch...

Comment 40

[Hidehiro Kozawa]

Comment on attachment 143155 [details] [diff] [review]
convert make well

Please review it

Comment 41

[Hidehiro Kozawa]

Comment on attachment 143155 [details] [diff] [review]
convert make well

Please review it

Comment 42

[noririty]

Comment on attachment 143155 [details] [diff] [review]
convert make well

It also have I18N aspects. Jshin:
Could you mind sr?

Comment 43

[Jungshik Shin]

Comment on attachment 143155 [details] [diff] [review]
convert make well

I can't sr, but here's r=jshin.

However, this patch is just a temporary measure. Even with this patch, mozilla
can't use a printer with Japanese name on Win2k/XP with the default locale set
to French while MS IE can. I'll work on this later.

Comment 44

[Hidehiro Kozawa]

Attachment 100602 [details] [diff] and attachment 103347 [details] [diff] [review] have that problem too.
Will you fix them?
I once tryed writing patch that is uncomely...
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2025&action=view

Comment 45

[Jungshik Shin]

Yes, I'm gonna take care of it as well. 

BTW, it'd be nice if you can replace nsString with nsAutoString here:

@@ -198,11 +201,7 @@
   LPTSTR lpPrtName;
   GlobalPrinters::GetInstance()->GetDefaultPrinterName(lpPrtName);
   nsString str;      <===== here 

Comment 46

[Hidehiro Kozawa]

Attachment: for unicode on NT

More usefull(?) patch but still not complete.
Comment 45 is not dealed too.

Comment 47

[Hidehiro Kozawa]

Attachment: path it woks for me both XP and 98SE

This patch have problem
1st There are alike code in MapPaperSizeToNativeEnumA and
+MapPaperSizeToNativeEnumW. I don't know how optimize this patch.
2nd in 98SE assertion occur in gfx/src/windows/nsDeviceContextSpecWin.cpp # 867

Comment 48

[Hidehiro Kozawa]

Comment on attachment 149755 [details] [diff] [review]
path it woks for me both XP and 98SE

I want reviewer comment.
It works for me.
CJK people want this fix...

Comment 49

[Jungshik Shin]

Instead of cluttering the tree with two branches of code (for Win9x/ME and
Win2k/XP), we're discussing the possibility of either making Mozilla depend on
MSLU or rolling out our own version of MSLU (namely, 'MZLU' that stands for
Mozilla layer for Unicode). See bug 239279. 

Comment 50

[Jungshik Shin]

I'm very sorry for holding this up. I'm sure attachment 149755 [details] [diff] [review] does work, but
let's put off supporting printers whose name has characters outside the current
locale for the now. We'll be able to fix it more cleanly once bug 239279 is fixed. 

Instead, let's just go with the trivial fix (attachment 143155 [details] [diff] [review]). 

Comment 51

[Jungshik Shin]

Comment on attachment 143155 [details] [diff] [review]
convert make well

r=jshin

I'll replace |nsString str| with |nsAutoString str| when checking it in.

Asking for sr and a1.7. This is a trivial fix for  a typical 'misinflation' of
strings in a legacy encoding to UTF-16 I should have reviewed much earlier.

Comment 52

[Hidehiro Kozawa]

*** Bug 169206 has been marked as a duplicate of this bug. ***

Comment 53

[Jungshik Shin]

Comment on attachment 149755 [details] [diff] [review]
path it woks for me both XP and 98SE

remove 'r'. once bug 239279 is fixed, we won't have to clutter our code like
this any more.

Comment 54

[Mike Kaply [:mkaply]]

Comment on attachment 143155 [details] [diff] [review]
convert make well

a=mkaply for 1.7
I'm not rechnically an sr, but this is a platform specific patch, so I am OK
with it.

Comment 55

[Hidehiro Kozawa]

attachment 143155 [details] [diff] [review] replaced |nsString str| with |nsAutoString str|
works well of course in 98SE-ja and XP-ja.
It looks good.

Comment 56

[Jungshik Shin]

Thanks for sr and a. (I'm sorry I was mistaken you to be able to sr :-))

fixed on the trunk as well as on 1.7branch. I'm keeping this open for printers
whose name include characters not covered by the current system locale. 

Comment 57

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 143155 [details] [diff] [review]
convert make well

>-#ifdef UNICODE
>-  str.AppendWithConversion((PRUnichar *)lpPrtName);
>-#else 
>-  str.AssignWithConversion((char*)lpPrtName);
>-#endif
>+  NS_CopyNativeToUnicode(nsDependentCString((char *)lpPrtName), str);


if |lpPrtName| is *ever* in an encoding like UTF-16 where one byte can be 0 in
the middle of the string, then this patch is incorrect because the length
calculation performed by nsDependentCString's constructor is incorrect for such
use.

Comment 58

[Jungshik Shin]

(In reply to comment #57)

> if |lpPrtName| is *ever* in an encoding like UTF-16 where one byte can be 0 in

  It's NEVER NEVER in UTF-16 because we use Win32 'A' API (GetProfileString) to
retrieve the value.

Comment 59

[Hiro]

(In reply to comment #54)
> (From update of attachment 143155 [details] [diff] [review])
> a=mkaply for 1.7
> I'm not rechnically an sr, but this is a platform specific patch, so I am OK
> with it.
> 

AVIARY_1_0_20040515_BRANCH does not checkin yet.

Comment 60

[Scott MacGregor]

the latest patch is now checked into the aviary 1.0 branch per jshin

Comment 61

[Jungshik Shin]

(In reply to comment #56)

> fixed on the trunk as well as on 1.7branch. I'm keeping this open for printers
> whose name include characters not covered by the current system locale. 

I filed bug 250255 for this and similar problems. Resolving as fixed.