132070 - Fail to save an existing Doc erases the data

Status: RESOLVED INVALID

(Core :: Internationalization, defect)

Description

[Yuying Long]

Build: 03-18 trunk build on WinXP and Mac 10.1.3

Steps:
1. Launch browser and open Composer.
2. Type anything, and save the document by go to File | Save As Charset.
3. Choose charset ISO-2022-KR, and save the file.

Result:
Got an error message "Save file failed", can not save the file with this charset.

Save as other Korean charsets(EUC-KR, JHC, JOHAB) don't have this problem.

Comment 1

[Yuying Long]

If you have a file that encoding as other charset, e.g. EUC-KR, and open it
through Composer, modify the charset as ISO-2022-KR will failed.

The problem here is, after you failed to save the file and then open the
original file again, that document will lost all the data.

-> nsbeta1

Comment 2

[Roy Yokoyama]

ylong: is this regression from yesterday?

Comment 3

[Roy Yokoyama]

ylong: forget my prevous question.
We don't have Unicode docoder for ISO-2022-KR.
We need to remove from the Save As charset list.

Comment 4

[Roy Yokoyama]

Attachment: Removing "Korean (ISO-2022-KR)" from SaveAsCharset dlg

ftang/shanjain: can you review or if you can tell me better solution?

Comment 5

[Roy Yokoyama]

cc nhotta and shanjian
ftang/nhotta/shanjian: can you review?

Comment 6

[Yuying Long]

Just curious:
If we remove it from Save as charset menu, do we need to do same thing in
browser View | Character Coding menu?
 

Comment 7

[Roy Yokoyama]

>do we need to do same thing in
>browser View | Character Coding menu?
No.  We do support encoding from ISO-2022-KR to Unicode.

Comment 8

[nhottanscp]

+      // No Unicode Decoder for ISO-2022-KR
+      if (charsetName == "Korean (ISO-2022-KR)")

Unicode Encoder (from Unicode) is used for SaveAs
Part of the string can be localized,  "Korean" in this case.

Comment 9

[Roy Yokoyama]

My bad. I meant the Encodeer not the Decoder.
+      // No Unicode Encoder for ISO-2022-KR
+      if (charsetName == "Korean (ISO-2022-KR)")


You are right. l10n issue is here.
Can we rely on "ISO-2022-KR" being part of string for all locales

Comment 10

[nhottanscp]

>Can we rely on "ISO-2022-KR" being part of string for all locales
I think so. But it should not be shown in Editor's charset menu either.
Could you look at nsCharsetMenu.cpp and check if we can do something there to
avoid unexisting encoder to be shown to the menu?

Comment 11

[nhottanscp]

nsbeta1+ per i18n triage

because of the dataloss of the original file (the comment #1)

Comment 12

[Roy Yokoyama]

Attachment: Adding Encoder check for nsDocumentEncoder::SetCharset()

Instead of removing ISO-2022-KR from SaveAs dlgbox, it will be better
to leave it there; but to generate a save file error msg (because 
we don't want to revisit this code once we implement a 
ISO-2022-KR Encoder)

MakeOutputStream(aFile, getter_AddRefs(outputStream))
in nsWebBrowserPersist.cpp NULLfy the output file.
This patch checks for the return value from 
rv = encoder->SetCharset(charsetStr); 
before calling MakeOutputStream() to make sure 
we have the Charset Encoder in question.

I lxr'ed for nsDocumentEncoder::SetCharset() usage in other
areas and only other place should check for the return value 
was in nsPlaintextEditor.cpp.  (this patch includes this fix as well)

nhotta: can you review?

Comment 13

[nhottanscp]

Looks good I have some comments. The patch needs review by either editor group
or content like jst.

+  nsAutoString charSet;
+  charSet = aCharset;
+  rv = mCharsetConverterManager->GetCharsetAtom(charSet.get(),
getter_AddRefs(charsetAtom));
I think you can do
PromiseFlatString(aCharset).get()

+    && !(aCharset.Equals(NS_LITERAL_STRING("null")))) {
Is this same as !aCharset.IsEmpty()

-    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
+    NS_ENSURE_SUCCESS(rv, rv);
Why the old code returns NS_ERROR_FAILURE?

Comment 14

[Roy Yokoyama]

Attachment: revised and add initialization code in nsDocumentEncoder::Init() for performace reason.

nhotta: thanks for the recommendation.

I also realized that nsDocumentEncoder::SetCharset() is called
few times at the start up.  I initialized mCharset to be "UTF-8" in 
nsDocumentEncoder::Init() to minimize the impact.

Comment 15

[Roy Yokoyama]

jfrancis/jst: can you review my code? I'd appreciate any comment.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 75688 [details] [diff] [review]
revised and add initialization code in nsDocumentEncoder::Init() for performace reason.

+  if (mCharset.IsEmpty()) {
+	nsresult rv;

Stick to two space indentation here.

 nsDocumentEncoder::SetCharset(const nsAReadableString& aCharset)
 {
-  mCharset = aCharset;
+  if (mCharset != aCharset) {

Please use !mCharset.Equals(aCharset) here in stead of the overloaded operator.

- In nsPlaintextEditor.cpp:

-  if (aCharset.Length() != 0
-    && !(aCharset.Equals(NS_LITERAL_STRING("null"))))
-    docEncoder->SetCharset(aCharset);
+  if (!aCharset.IsEmpty()) {
+    rv = docEncoder->SetCharset(aCharset);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }

I don't know what the old code tried to achieve here with the "null" string
check, but if you know it's not needed any more, then I'm fine with removing
it.

Other than that, sr=jst

Comment 17

[Frank Tang]

One question, will remove "ISO-2022-KR" from the menu solve this issue?
Why do we need this patch to do that work?

Comment 18

[Frank Tang]

what do we really want to change in this bug? if we just want to remove the ui
from the save as charset dialogbox, adding a line
iso-2022-kr.notForBrowser = true
to intl/uconv/src/charsetData.properties is good enough 

Comment 19

[Roy Yokoyama]

>One question, will remove "ISO-2022-KR" from the menu solve this issue?
>Why do we need this patch to do that work?
No.   Naoki and I discussed the issue and we decided to
not to change UI; but instead to fix the saving problem (see comment #1)
We don't want to revisit the code again if we implement
the ISO-2022-KR encoder.

jfrancis: can you give review?  I have jst's fix in my local machine.

Comment 20

[Jungshik Shin]

Roy's first patch is much better than others. We MUST remove ISO-2022-KR
from 'Save as charset list'. Mozilla MUST NOT encourage use of the
completely obsolete charset ISO-2022-KR. ISO-2022-KR encoder was
intentionally left out because there's absolutely NO USE for it.
Decoder is still necessary because there are some old/outdated
programs that produce it and Mozilla has to be able to decode it.

Comment 21

[Jungshik Shin]

> what do we really want to change in this bug? if we just want to >
> remove the ui
> from the save as charset dialogbox, adding a line
> iso-2022-kr.notForBrowser = true

  Would this make Mozilla unable to accept/interpret *incoming*
iso-2022-KR? As I wrote before, Mozilla should not produce
ISO-2022-KR, but shuld be able to accept it in *incoming* streams
(both mail and html). 

> Roy's first patch is much better than others.

  I'm sorry I didn't fully understand  your intent in the
second patch when making this comment. Your second patch is 
more generic and I like it. 

  In addition to that, it'd be great if we could remove ISO-2022-KR
from 'SaveAs charset list' as your first patch does. 

Comment 22

[Jungshik Shin]

>  In addition to that, it'd be great if we could remove ISO-2022-KR
> from 'SaveAs charset list' as your first patch does

Roy,

  If you don't like 'special-casing' ISO-2022-KR as you did in
your first patch to hide from 'SaveAs charset dialog', we may
make a new field for charset like 'iso-2022-kr.notForOutgoing'
and set it to 'True' in charsetData.properties.
Then, your first patch can be made generic and you don't have 'special-case' it. 

Comment 23

[Roy Yokoyama]

Jungshik: let's split this bug.
          1) Failed to save a existing doc erases the data.
          2) Need to remove "ISO-2022-KR" from SaveAs dlgbox

Comment 24

[Roy Yokoyama]

1) Fail to save an existing doc erases the data. => this bug
2) Need to remove "ISO-2022-KR" from SaveAs dlgbox  => 133615

Comment 25

[Roy Yokoyama]

Attachment: latest

Comment 26

[Roy Yokoyama]

Comment on attachment 76298 [details] [diff] [review]
latest

Carry over /sr=jst from 
prev. patch.

nhotta/jfrancis: can someone review?

Comment 27

[Joe Francis]

Wow.  I totally remember cc'ing other folks and asking them to review.  And now 
it's not here.

One more try:  Akkana, Charlie, Kathy: do any of you know why the plaintext 
editor was checking for the string "null" for charsets?  Roy wants to remove that 
and I have no idea what it was there for.

I need some review help here.

Comment 28

[Akkana Peck]

I don't know -- I wonder if Naoki might?

cvsblame pins it on mjudge, but that's probably because of the sweeping string
changes.

Comment 29

[Roy Yokoyama]

Guys, if _null_ check for charset is what's stoping from /r,
then I can remove it. It's not necessary for this patch.
I added because of Naoki's suggestion. (See comment #13)

Comment 30

[Roy Yokoyama]

anybody can review my patch?  I've been waiting for awhile.
thanks

Comment 31

[Kathleen :Brade]

Comment on attachment 76298 [details] [diff] [review]
latest

r=brade

Comment 32

[Asa Dotzler [:asa]]

Comment on attachment 76298 [details] [diff] [review]
latest

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 33

[Frank Tang]

 Impact Platform: ALL
Impact language users: Korean users only 25.5 M  4.55% of internet users
Probability of hitting the problem: MID, save file in composer with different
charset is normal action
Severity if hit the problem in the worst case: data lost
Way of recover after hit the problem: none.
Risk of the fix: MID
Potential benefit of fix this problem: other charsets

Comment 34

[Roy Yokoyama]

add adt1.0.0 for checkin approval

Comment 35

[Frank Tang]

this is a adt2 bug

Comment 36

[Jaime Rodriguez, Jr.]

adt1.0.0- (on ADT's behalf).

Comment 37

[Roy Yokoyama]

setting new milestone due to adt1.0.0-

Comment 38

[Kathleen :Brade]

Why is this adt1.0.0-?
This is causing dataloss, right?  
Is it OK to fix/checkin some of the error checking in this patch?
move back to milestone 1.0 to get explanation

Comment 39

[tao]

Does this problem happen even the target charset of "save as"  is not
"ISO-2022-KR"? If so, I think this problem is rather severe for users. If not, 
any idea the precentage of the Korean users use "ISO-2022-KR"?

Comment 40

[Frank Tang]

this should be adt1.0.0-
rational:
1. currently, it only impact ISO-2022-KR and ISO-2022-KR only. We know no other
cases which have this problem.
2. ISO-2022-KR is only used by South Korean usrs- There are 25.5 M Korean users
4.55% of internet users. With in these Korean users, most of the users won't use
ISO-2022-KR, which is a obsolete charset according to Jungshik Shin. Jungshik is
THE Korean expert working with mozilla for many many years with us. We simply
trust his word about any information about Korean internet usage. Therefore, the
real user which will use this encoding is much less than 25.5M users 

>------- Additional Comment #20 From Jungshik Shin 2002-03-25 14:26 -------
>We MUST remove ISO-2022-KR
>from 'Save as charset list'. Mozilla MUST NOT encourage use of the
>completely obsolete charset ISO-2022-KR. ISO-2022-KR encoder was
>intentionally left out because there's absolutely NO USE for it.
>Decoder is still necessary because there are some old/outdated
>programs that produce it and Mozilla has to be able to decode it.

3. even in the case of ISO-2022-KR, a warning message will show up and tell the
user the saving is failed so they still have chance to save it into other charset. 

by the above reason, I recommed this is to be a adt1.0.0- bug last time. I add
th - back now unless there are strong reason that we should not. 

>Does this problem happen even the target charset of "save as"  is not
"ISO-2022-KR"? 
the answer is NO. "ISO-2022-KR" is currently the only known case.

Comment 41

[Roy Yokoyama]

I thought I got the "save failed" dlgbox; but it doesn't come up with 0.9.9 build.
ylong: Can you try if you have Error msg popup in both cases? 
       Case 1) Steps described in the begining
       Case 2) Steps described in comment #1

Comment 42

[Yuying Long]

I got the error message box on 04-02 trunk build, and the result still same as
before. 

Comment 43

[Roy Yokoyama]

making off my radar for 1.0 milestone

Comment 44

[Frank Tang]

move from nsbeta1+ to nsbeta1-

Comment 45

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 46

[Roy Yokoyama]

assign to ftang

Comment 47

[Frank Tang]

what a hack. I have not touch mozilla code for 2 years. I didn't read these bugs
for 2 years. And they are still there. Just close them as won't fix to clean up.

Comment 48

[Travis Chase]

Mass Reassign Please excuse the spam

Comment 49

[Travis Chase]

Mass Re-opening Bugs Frank Tang Closed on Wensday March 02 for no reason, all
the spam is his fault feel free to tar and feather him

Comment 50

[Travis Chase]

Reassigning Franks old bugs to Jungshik Shin for triage - Sorry for spam

Comment 51

[Wayne Mery (:wsmwk)]

ISO-2022-KR wasn't offered to me, so I tried ISO-2022-JP, which WFM, no error

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.11) Gecko/20071128 SeaMonkey/1.1.7

Comment 52

[Simon Montagu :smontagu]

ISO-2022-JP would never have had this bug. ISO-2022-KR was removed from the "Save as Charset" menu in the spin-off bug 133615, but it's worth investigating whether the dataloss problem still exists in some other scenario.

Comment 53

[Simon Montagu :smontagu]

I can reproduce the bug as originally reported by trying to save as ISO-2022-CN. This is certainly not a 1.9 blocker, and the patch probably needs some updating, so let's target it for after we branch.

Comment 54

[Jim Mathies [:jimm]]

composer no longer exists.