397850 - Make NS_ReadLine() not lie about having more data when the stream ends in \r\n

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Mats Palmgren (inactive)]

Make NS_ReadLine() not lie about having more data when the stream ends in \r\n.

Follow up from bug 397768 comment 8.

Patch coming up...

Comment 1

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This is hopefully easier to understand than the old code. I removed
"aBuffer->empty" and replaced it with "aBuffer->start == aBuffer->end" as
the empty buffer condition and "aBuffer->current" isn't needed between calls
so I made it local, and "eolStarted" is redundant since we have "eolchar".

It's 2-22% faster than the old code (the lower figure for "normal"
content and the higher figure for the edge case of a file filled with \n).

It's possible to make the function slightly shorter, by just having
one for-loop with the "if (eolchar ...)" tests inside it, but by
splitting the loop in two "eolchar" became a loop invariant so it's
faster.  It's also clearer I think.

Comment 2

[Mats Palmgren (inactive)]

Attachment: xpcshell regression tests

Comment 3

[Mike Schroepfer]

We want this for bug 410264 ?

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 282972 [details] [diff] [review]
xpcshell regression tests

should probably make the entries in the test_array strings instead of numbers, because that's what readLine returns

can you put your txt files into a data subdirectory of test/unit, so that they don't clutter the directory with the tests?

Comment 5

[Mike Schroepfer]

Did this get resolved?

Comment 6

[[e-mail]]

if there are no objections i'll just land this sometime in the next few days.

Comment 7

[Mats Palmgren (inactive)]

Mike, Dan, sorry for the delay.  I'm landing some other stuff today so I'll
fix Christian's nits and land this today also...

Comment 8

[Mats Palmgren (inactive)]

Attachment: xpcshell regression tests

'kLineBufferSize' has been changed from 1024 to 4096 since the last time,
so I had to rewrite a few of the tests to trigger the end-of-buffer
condition as intended.

(I will add a code comment next to the #define to point out that the
 tests should be updated if the value is changed.)

(In reply to comment #4)
> should probably make the entries in the test_array strings ...

Did so.

> can you put your txt files into a data subdirectory of test/unit,

That too.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Additional patch

There are a couple of uses of the old line buffer fields that I missed,
so we need this patch too.  Sorry for missing this the first time.

Comment 10

[Mats Palmgren (inactive)]

I had to do a minor tweak in one of the unit tests for passwordmgr
that depended on this bug.  Filed bug 412440 on that.

mozilla/netwerk/test/unit/test_readline.js 	1.1
mozilla/netwerk/test/unit/data/test_readline1.txt 	1.1
mozilla/netwerk/test/unit/data/test_readline2.txt 	1.1
mozilla/netwerk/test/unit/data/test_readline3.txt 	1.1
mozilla/netwerk/test/unit/data/test_readline4.txt 	1.1
mozilla/netwerk/test/unit/data/test_readline5.txt 	1.1
mozilla/netwerk/test/unit/data/test_readline6.txt 	1.1
mozilla/netwerk/test/unit/data/test_readline7.txt 	1.1
mozilla/netwerk/test/unit/data/test_readline8.txt 	1.1
mozilla/netwerk/base/public/nsReadLine.h 	1.12
mozilla/mailnews/compose/src/nsMsgSendPart.cpp 	1.64
mozilla/mailnews/local/src/nsLocalMailFolder.cpp 	1.568 

-> FIXED