Closed Bug 132070 Opened 22 years ago Closed 3 years ago

Fail to save an existing Doc erases the data

Categories

(Core :: Internationalization, defect)

defect
Not set
critical

Tracking

()

RESOLVED INVALID

People

(Reporter: amyy, Unassigned)

References

Details

(Keywords: dataloss, intl, Whiteboard: [adt3])

Attachments

(1 file, 3 obsolete files)

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.
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
Keywords: intl, nsbeta1
QA Contact: ruixu → ylong
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0
ylong: is this regression from yesterday?
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.
ftang/shanjain: can you review or if you can tell me better solution?
cc nhotta and shanjian
ftang/nhotta/shanjian: can you review?
Just curious:
If we remove it from Save as charset menu, do we need to do same thing in
browser View | Character Coding menu?
 
>do we need to do same thing in
>browser View | Character Coding menu?
No.  We do support encoding from ISO-2022-KR to Unicode.
+      // 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.
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
>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?
nsbeta1+ per i18n triage

because of the dataloss of the original file (the comment #1)
Keywords: nsbeta1nsbeta1+
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?
Attachment #75081 - Attachment is obsolete: true
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?

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.
Attachment #75662 - Attachment is obsolete: true
jfrancis/jst: can you review my code? I'd appreciate any comment.
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
Attachment #75688 - Flags: superreview+
One question, will remove "ISO-2022-KR" from the menu solve this issue?
Why do we need this patch to do that work?
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 
>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.

Blocks: 133346
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.
> 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. 
>  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. 
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
1) Fail to save an existing doc erases the data. => this bug
2) Need to remove "ISO-2022-KR" from SaveAs dlgbox  => 133615
Summary: Failed to save a document that charset set to ISO-2022-KR → Fail to save an existing Doc erases the data
Attached patch latestSplinter Review
Attachment #75688 - Attachment is obsolete: true
Comment on attachment 76298 [details] [diff] [review]
latest

Carry over /sr=jst from 
prev. patch.

nhotta/jfrancis: can someone review?
Attachment #76298 - Flags: superreview+
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.
I don't know -- I wonder if Naoki might?

cvsblame pins it on mjudge, but that's probably because of the sweeping string
changes.
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)
anybody can review my patch?  I've been waiting for awhile.
thanks
Comment on attachment 76298 [details] [diff] [review]
latest

r=brade
Attachment #76298 - Flags: review+
Comment on attachment 76298 [details] [diff] [review]
latest

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #76298 - Flags: approval+
 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
add adt1.0.0 for checkin approval
Whiteboard: adt1.0.0
Keywords: adt1.0.0
Whiteboard: adt1.0.0
this is a adt2 bug
Whiteboard: adt2
adt1.0.0- (on ADT's behalf).
Keywords: adt1.0.0adt1.0.0-
Whiteboard: adt2 → [adt3]
setting new milestone due to adt1.0.0-

Target Milestone: mozilla1.0 → mozilla1.0.1
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
Keywords: adt1.0.0-adt1.0.0, dataloss
Target Milestone: mozilla1.0.1 → mozilla1.0
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"?
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.
Keywords: adt1.0.0adt1.0.0-
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
I got the error message box on 04-02 trunk build, and the result still same as
before. 
making off my radar for 1.0 milestone
Target Milestone: mozilla1.0 → mozilla1.0.1
move from nsbeta1+ to nsbeta1-
Keywords: nsbeta1+nsbeta1-
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.
Severity: normal → critical
assign to ftang
Assignee: yokoyama → ftang
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Target Milestone: mozilla1.0.1 → ---
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.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → WONTFIX
Mass Reassign Please excuse the spam
Assignee: ftang → nobody
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
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Reassigning Franks old bugs to Jungshik Shin for triage - Sorry for spam
Assignee: nobody → jshin1987
Status: REOPENED → NEW
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
QA Contact: amyy → i18n
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.
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.
Target Milestone: --- → mozilla2.0

composer no longer exists.

Assignee: jshin1987 → nobody
Status: NEW → RESOLVED
Closed: 19 years ago3 years ago
Resolution: --- → INVALID
Target Milestone: mozilla2.0 → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: