Closed
Bug 219493
Opened 21 years ago
Closed 21 years ago
nsPersistentProperties doesn't handle multi-line properties files correctly
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
People
(Reporter: pkwarren, Assigned: pkwarren)
Details
Attachments
(2 files, 1 obsolete file)
1.08 KB,
text/plain
|
Details | |
1.51 KB,
patch
|
darin.moz
:
review+
alecf
:
superreview+
|
Details | Diff | Splinter Review |
Consider the following entry in a .properties file: entry=This is the first line.\n\ \n\ This is the third line. Java handles these .properties files properly. The result of parsing this file in Java is the following: entry=This is the first line. This is the third line. Mozilla handles these files incorrectly: entry=This is the first line. \nThis is the third line. From the following in nsPersistentProperties.cpp, you can see where the error lies: 184 if (c == '\\') { 185 c = Read(); 186 switch(c) { 187 case '\r': 188 case '\n': 189 c = SkipWhiteSpace(c); 190 value.Append((PRUnichar) c); 191 break; This will read until the first '\', which occurs at the end of a line, so this is a multiline properties entry. Then it encounters the end of the first line, and skips until the next non-whitespace character, which is another '\'. It then appends the literal '\' character, and continues on until the 'n'. It then appends the 'n' character, and so we get the literal string '\n' appended instead of a newline.
Assignee | ||
Comment 1•21 years ago
|
||
This is the Java test program I used to test.
Assignee | ||
Comment 2•21 years ago
|
||
Patch which fixes the problem.
Assignee | ||
Updated•21 years ago
|
Attachment #131621 -
Flags: superreview?(alecf)
Attachment #131621 -
Flags: review?(darin)
Comment 3•21 years ago
|
||
Comment on attachment 131621 [details] [diff] [review] Patch v1 looks good, but do LOTS of testing, and be prepared to back out - we've had problems with this code before, and it affects a lot of people - I doubt that many folks are using multi-line .properties files in ENGLISH, but in other languages they may... i.e. make sure to test with cases beyond just \n i.e. test to make sure foo=alpha\ beta\ gamma and make sure you get alphabetagamma. Also make sure that special characters like 'u' and 'n' don't trigger something wierd, like foo=I did it all for the \ nookie
Attachment #131621 -
Flags: superreview?(alecf) → superreview+
Assignee | ||
Comment 5•21 years ago
|
||
We actually found this while testing Mozilla in other languages. The tools our translators are using expected .properties files to behave similarly to Java .properties files, so this may be why this was not caught before. I have tested the examples you have shown and they are working correctly. I have come up with one more incompatibility between the Java .properties parser and the Mozilla .properties parser (both before and after this patch): foo=alpha\ beta\ gamma\ <-- Single space here <-- Single space here foo2=I did it all for the \ nookie With Java's parser, you get the following: foo=alphabetagamma foo2=I did it all for the nookie With Mozilla's parser, you get: Key: foo Value: alphabetagammafoo2=I did it all for the nookie
Status: NEW → ASSIGNED
Comment 6•21 years ago
|
||
thanks for being thorough - I got bit by quirks in this file about 6 months ago...
Comment 7•21 years ago
|
||
Comment on attachment 131621 [details] [diff] [review] Patch v1 r=darin
Attachment #131621 -
Flags: review?(darin) → review+
Assignee | ||
Comment 8•21 years ago
|
||
Better patch. Fixes other inconsistency noted with Mozilla's .properties file parser vs. Java parser. Removes some redundant checks in SkipLine and SkipWhiteSpace.
Assignee | ||
Updated•21 years ago
|
Attachment #131621 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #131774 -
Flags: superreview?(alecf)
Attachment #131774 -
Flags: review?(darin)
Comment 9•21 years ago
|
||
Comment on attachment 131774 [details] [diff] [review] Patch v2 hmm.. while I'm sure these are the right changes, how do you feel about landing this stuff in stages? I know it seems like you're not changing much, but I've been bitten by some wierd uses of .properties files - people rely on wierd quirks of parsing, etc! I spent a good hour testing some changes I had made before landing changes like this, and it took two weeks and 3 fixer-upper patches before everything settled down again.. :( why don't you land the other patch, wait a day, and land this one. that will make it much easier for QA folks to determine which build (and thus which patch) causes a problem... sr=alecf on the changes though
Attachment #131774 -
Flags: superreview?(alecf) → superreview+
Assignee | ||
Comment 10•21 years ago
|
||
I have checked in the first patch (as alecf has suggested) to ensure there are no major regressions. Checking in nsPersistentProperties.cpp; /cvsroot/mozilla/xpcom/ds/nsPersistentProperties.cpp,v <-- nsPersistentProperties.cpp new revision: 1.43; previous revision: 1.42 done
Comment 11•21 years ago
|
||
Comment on attachment 131774 [details] [diff] [review] Patch v2 >Index: xpcom/ds/nsPersistentProperties.cpp > PRInt32 > nsPersistentProperties::SkipWhiteSpace(PRInt32 c) > { >- while ((c >= 0) && IS_WHITE_SPACE(c)) { >+ while (IS_WHITE_SPACE(c)) { > c = Read(); > } here, (c >= 0) is redundant, and it clearly makes sense to remove it. > PRInt32 > nsPersistentProperties::SkipLine(PRInt32 c) > { >- while ((c >= 0) && (c != '\r') && (c != '\n')) { >+ while (c != '\r' && c != '\n') { > c = Read(); > } however, here... by removing (c >= 0) you are now being more accepting. i do not know enough about the context of this function to be able to say off hand whether or not that matters, but it does seem suspect to me. convince me that you can remove this (c >= 0) check or put it back, and r=me ;-)
Assignee | ||
Comment 12•21 years ago
|
||
Darin - you are correct, this was a careless mistake. I will check in the second patch without this change. Thanks.
Comment 13•21 years ago
|
||
Comment on attachment 131774 [details] [diff] [review] Patch v2 r=darin with that change
Attachment #131774 -
Flags: review?(darin) → review+
Assignee | ||
Comment 14•21 years ago
|
||
Checked in second patch. Checking in nsPersistentProperties.cpp; /cvsroot/mozilla/xpcom/ds/nsPersistentProperties.cpp,v <-- nsPersistentProperties.cpp new revision: 1.44; previous revision: 1.43 done
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•