nsConverterOutputStream logs "WARNING: Flush() lost data!" at close if underlying stream is already closed

RESOLVED FIXED in mozilla15

Status

()

Core
Internationalization
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Irving, Assigned: Irving)

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

The warning is frightening but harmless; the sequence of events that brought this on are:

JS code creates an nsISafeOutputStream to write safely to a file, then wraps that stream in a nsIConverterOutputStream

Once all the bytes are written through the converter, the JS code calls ConverterOutputStream.flush() and then SafeOutputStream.finish() to close and rename the underlying file to its final name. The JS code *cannot* just call ConverterOutputStream.close(), because calling close() on the SafeOutputStream aborts the file save operation; the temporary is only moved into its final position if finish() is called.

So, the underlying OutputStream is closed but the ConverterOutputStream is not. Then, when JS GC destroys the ConverterOutputStream, the destructor calls close(), which calls flush(), which always tries to write() to the underlying OutputStream even if it doesn't have anything to write.

I'll attach a patch for that issue, but it would also be worth making nsIConverterOutputStream implement the nsISafeOutputStream API so that calling code could just finish() the ConverterOutputStream and have the right things happen all the way down.
Created attachment 626541 [details] [diff] [review]
Don't write to the underlying stream if flush has nothing to write
Attachment #626541 - Flags: review?(smontagu)
Attachment #626541 - Flags: review?(smontagu) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea68a0638211
Keywords: checkin-needed
Target Milestone: --- → mozilla15

Comment 3

5 years ago
https://hg.mozilla.org/mozilla-central/rev/ea68a0638211
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.