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

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[:Irving Reid (No longer working on Firefox)]

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.

Comment 1

[:Irving Reid (No longer working on Firefox)]

Attachment: Don't write to the underlying stream if flush has nothing to write

Comment 2

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea68a0638211

Comment 3

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ea68a0638211