nsBufferedOutputStream::WriteSegments doesn't handle non-blocking streams well

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

unspecified
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

5 years ago
nsBufferedOutputStream::WriteSegments can return NS_ERROR_FAILURE even after having transferred some data to the underlying stream, causing the partial write count to be unavailable to JS callers, which makes it extremely difficult to use correctly from JS.

The general convention for non-blocking I/O is that, if the operation was able to transfer any bytes at all, even if not as many as was requested, it returns the count and NS_OK. Only if the operation was unable to transfer any bytes whatsoever does it return NS_BASE_STREAM_WOULD_BLOCK.

We can see this rule implemented correctly in nsBufferedOutputStream::Write: if a call to Flush fails, then we reach this exit path:

    *result = written;
    return (written > 0) ? NS_OK : rv;

However, nsBOS::WriteSegments isn't so civil:

            rv = Flush();
            if (NS_FAILED(rv))
              return rv;

As a consequence, if we go around the WriteSegments loop successfully once, and then a call to Flush fails, WriteSegments will return an error code, and the caller has no idea how many bytes were written. (I guess they can compare 'tell' results before and after, but that's horrible.)
Assignee

Comment 1

5 years ago
Fix, including tests. (This patch also fixes a memcpy vs. memmove issue.)
Assignee: nobody → jimb
Status: NEW → ASSIGNED
Attachment #8415679 - Flags: review?(mcmanus)
Assignee

Updated

5 years ago
Blocks: 797639
Comment on attachment 8415679 [details] [diff] [review]
Handle partial flushes correctly in nsBufferedOutputStream::WriteSegments.

Review of attachment 8415679 [details] [diff] [review]:
-----------------------------------------------------------------

thanks
Attachment #8415679 - Flags: review?(mcmanus) → review+
Assignee

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e0c131fc557
Flags: in-testsuite+
Target Milestone: --- → mozilla32
https://hg.mozilla.org/mozilla-central/rev/4e0c131fc557
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.