412440 - head_storage_legacy_1.js depends on bug in readLine

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

head_storage_legacy_1.js depends on bug in readLine.

Landing bug 397850 caused the "test_storage_legacy_3.js" unit test to fail,
the reason was that countLinesInFile() in "head_storage_legacy_1.js" depended
on that bug.  I have already checked in the patch to fix orange so this is
to get a proper review after the fact.
(sorry for not detecting this before landing bug 397850)

Comment 1

[Justin Dolske [:Dolske]]

Comment on attachment 297160 [details] [diff] [review]
Patch rev. 1

>     var line = { value : null };
>-    var lineCount = 0;
>+    var lineCount = 1;
>     while (lineStream.readLine(line)) 
>         lineCount++;

Hmm, I don't think this is right. Consider the case of a 0-byte file.

Perhaps something like:

do {
  var more = lineStream.readLine(line);
  if (line.value != null)
    lineCount++;
} while (more);

I'm assuming that reading from a 0-byte file returns line.value == null, and not empty-string.

I guess the interface signature is just sucky here, because the retval doesn't tell you if the current call succeeded. Also implies that if the source stream sends "line1\n", it can't emit a line to the caller until either another byte appears from the stream or it closes. :-/

Comment 2

[Mats Palmgren (inactive)]

(In reply to comment #1)
> I'm assuming that reading from a 0-byte file returns line.value == null, and
> not empty-string.

Unfortunately, your assumption is wrong.  It returns an empty string, so
there really is no way differentiate between an empty file and a file with
just a single newline.  This is the correct behaviour though, according to:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/io/nsILineInputStream.idl&rev=3.6&root=/cvsroot&mark=47-49

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Use nsIFile.fileSize to determine if it's an empty file.

Comment 4

[Justin Dolske [:Dolske]]

(Since this is just a test, I don't believe any further review is required by a Real Module Peer.)

Comment 5

[:Gavin Sharp [email: [e-mail]]]

I am Gavin Sharp, and I approve this message.

Comment 6

[Mats Palmgren (inactive)]

toolkit/components/passwordmgr/test/unit/head_storage_legacy_1.js 	1.9

(qm-winxp01 is orange at the moment, but it's unrelated)

-> FIXED