347420 - networks.txt ignored if last line doesn't end in \n

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[Kenneth Herron]

The code which reads networks.txt (see bug 27807) silently ignores any text in the file following the last \r or \n. As a result, if the last END tag in the file isn't followed by a newline, the entire file is silently rejected as invalid.

Apparently there's some plan to add network editing to the chatzilla UI, in which case I presume this file would be automatically generated, but currently the only way to get this file is for the user to construct one with a text editor. It's pretty easy to create a file with this flaw in notepad for example. Silently rejecting the file because of a single missing trailing whitespace character is pretty unfriendly behavior.

Based on a conversation in #chatzilla I expect this to be closed without action, but I wanted to get it on record.

Comment 1

[Kenneth Herron]

Attachment: Sample networks.txt which displays the problem

This defines a network called "nosenet". Note there is no newline following the last "END" tag.

Comment 2

[:Gijs (he/him)]

Attachment: Patch

This seems to work jolly well :-)

Comment 3

[:Gijs (he/him)]

Attachment: Patch v2

OK, so perhaps this is the less evil solution.

Comment 4

[James Ross]

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

>-            this._buffer += this._fileStream.read();
>+            newBytes = this._fileStream.read();
>+            // If we got something, parse stuff
>+            // Otherwise, bail if there's no data at all.
>+            if (newBytes != null)
>+            {
>+                this._buffer += newBytes;
>+                bytesRead += newBytes.length;
>+            }
>+            else if (bytesRead < this._file.fileSize)
>+            {
>+                continue;
>+            }

This should be rearranged to have the continue first, and perhaps a comment to indicate that we've been told to expect more data but that it hasn't been given to us.

>             // Got more data in the buffer, so split into lines.
>-            // The last one doesn't count - the rest get added to the full list.
>+            // The last one doesn't count if it isn't really the last line.
>             var lines = this._buffer.split(/[\r\n]+/);
>-            this._buffer = lines.pop();
>+            if (bytesRead < this._file.fileSize)
>+                this._buffer = lines.pop();

This will leave this._buffer with the old data if bytesRead >= this._file.fileSize. That doesn't sound right to me.

>             this._lines = this._lines.concat(lines);

Comment 5

[:Gijs (he/him)]

Attachment: Patch v3

<Silver_work>	Also, we have another issue I've just realised.
<Silver_work>	You're only counting data read /that call/, when the file is shared over all deserialize calls.
<Hannibal>	ok, so bytesRead should be a member variable?
<Silver_work>	A private one, yeah.

That and comments above fixed, or so I hope.

Comment 6

[James Ross]

Comment on attachment 244425 [details] [diff] [review]
Patch v3

>+            if (!newBytes && this._bytesRead < this._file.fileSize)

Please use an extra set of parentheses here, for clarity.

r=silver with that fixed.

Comment 7

[:Gijs (he/him)]

Attachment: Patch v4

Patch which fixes the nsLocalFile.read method to:
- return null at EOF
- return "" ONLY if the underlying filestream throws while trying to get more data.
- return more data otherwise.

and fixes the users of said method to cope with this change.

Comment 8

[James Ross]

Comment on attachment 244833 [details] [diff] [review]
Patch v4

>+// Will return null if there is no more data in the file.
>+// Will block as long as it thinks there's data left before |max| .

Not quite right; the stream read method will return > 0 but <= max bytes. It may not return == max always. I think the correct notion here is that it will block until it has /some/ data to return.

>+// Will return an empty string if there is data, but it couldn't be read.


>-            this._buffer += this._fileStream.read();
>-            // Got more data in the buffer, so split into lines.
>-            // The last one doesn't count - the rest get added to the full list.
>+            var newData = this._fileStream.read();
>+            if (newData)
>+                this._buffer += newData;
>+            else if (this._buffer.length == 0)
>+                break;

Nit: Insert a blank line here for clarity.

>+            // Got more data in the buffer, so split into lines. Unless we're
>+            // done, the last one might not be complete yet, so save that one.


>-        state.loadPendingData += state.loadFile.read(state.loadChunk);
>+
>+        var newChunk = state.loadFile.read(state.loadChunk);
>+        if (newChunk != null)
>+            state.loadPendingData += newChunk;

Arguably, that could just be |if (newChunk)| as well.

r=silver with changes.

Comment 9

[:Gijs (he/him)]

Fixed with nits changed:

Checking in mozilla/extensions/irc/js/lib/file-utils.js;
/cvsroot/mozilla/extensions/irc/js/lib/file-utils.js,v  <--  file-utils.js
new revision: 1.11; previous revision: 1.10
done
Checking in mozilla/extensions/irc/js/lib/text-serializer.js;
/cvsroot/mozilla/extensions/irc/js/lib/text-serializer.js,v  <--  text-serializer.js
new revision: 1.6; previous revision: 1.5
done
Checking in mozilla/extensions/irc/xul/content/channels.js;
/cvsroot/mozilla/extensions/irc/xul/content/channels.js,v  <--  channels.js
new revision: 1.3; previous revision: 1.2
done