219493 - nsPersistentProperties doesn't handle multi-line properties files correctly

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Philip K. Warren]

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.

Comment 1

[Philip K. Warren]

Attachment: Java test program

This is the Java test program I used to test.

Comment 2

[Philip K. Warren]

Attachment: Patch v1

Patch which fixes the problem.

Comment 3

[Alec Flett]

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

Comment 4

[Philip K. Warren]

Taking ownership of bug.

Comment 5

[Philip K. Warren]

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

Comment 6

[Alec Flett]

thanks for being thorough - I got bit by quirks in this file about 6 months ago...

Comment 7

[Darin Fisher]

Comment on attachment 131621 [details] [diff] [review]
Patch v1

r=darin

Comment 8

[Philip K. Warren]

Attachment: Patch v2

Better patch. Fixes other inconsistency noted with Mozilla's .properties file
parser vs. Java parser. Removes some redundant checks in SkipLine and
SkipWhiteSpace.

Comment 9

[Alec Flett]

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

Comment 10

[Philip K. Warren]

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

[Darin Fisher]

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 ;-)

Comment 12

[Philip K. Warren]

Darin - you are correct, this was a careless mistake. I will check in the second
patch without this change. Thanks.

Comment 13

[Darin Fisher]

Comment on attachment 131774 [details] [diff] [review]
Patch v2

r=darin with that change

Comment 14

[Philip K. Warren]

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