1778565 - HTMLEditor::InsertObject incorrectly handles reading input streams from blobs

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Nika Layzell [:nika] (ni? for response)]

The code in HTMLEditor::InsertObject will attempt to read the input stream from the associated type-erased object if it is being used to insert an image into the document. Unfortunately, this behaviour is performed with some incorrect assumptions.

As noted in the documentation for NS_ConsumeStream, this method will only consume the data from a stream up until it returns an error, including the NS_BASE_STREAM_WOULD_BLOCK error which is returned from non-blocking async streams. This means that the method is only an appropriate way to consume an entire stream synchronously if the passed-in stream is fully available and synchronous.

However, the stream which is passed in is frequently not fully-available and synchronous when the blob path is taken. A blob received from the parent process, such as a file blob, will generally use a RemoteLazyInputStream as the returned stream type. This stream is always async, and will stream in the response data asynchronously when it is requested, meaning that the call to NS_ConsumeStream will always return NS_BASE_STREAM_WILL_BLOCK and return nothing when used with a remote blob in this way.

This may also be the case with the streams which are passed in directly, however it is harder to follow the origins of those input streams, so there may be some other invariant somehow ensuring that they're always synchronous.

Given that this code is running on the main thread, and may need to do some I/O to read in the stream, it probably should be refactored to be able to read the data asynchronously. This would also allow removing the call to SlurpFileToString which when called would do blocking file I/O on the main thread to read in a file from disk.

It seems likely that it would be possible to handle this situation asynchronously given that the code earlier operating on BlobImpl will consume the blob's data asynchronously through SlurpBlob.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you for explaining the detail! It sounds like the pasting file/image is completely broken, however, we don't get such bug reports as far as I know. Perhaps I misunderstand somethimg or the path is a dead path actually, e.g., JS libraries for creating editor or web apps themselves handle it instead for better and consistent behavior between browsers.

I need to keep investigating more for considering the severity.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:kmag, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Tom Schuster (MoCo)]

Attachment: Bug 1778565 - Editor: Handle nsIFile transferable as blob. r?nika,masayuki

Comment 4

[Tom S [:evilpie]]

Attachment: Bug 1778565 - Editor: Handle nsIFile transferable as blob. r?nika,masayuki

Comment 5

[Tom S [:evilpie]]

Attachment: Bug 1778565 - Remove IsFileImage and SlurpFileToString. r?nika

Depends on D155753

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/420e02d37505
Editor: Handle nsIFile transferable as blob. r=nika,masayuki
https://hg.mozilla.org/integration/autoland/rev/5f6350cfab61
Remove IsFileImage and SlurpFileToString. r=nika

Comment 7

[Sandor Molnar[:smolnar]]

Backed out for causing mochitest plain failures in editor/libeditor/tests/test_bug490879.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/472635603967535a4fabdb805f16955e638c7525

Push with failures

Failure log

INFO - TEST-INFO | screenshot: exit 0
[task 2022-10-18T19:03:12.663Z] 19:03:12     INFO - TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug490879.html | uncaught exception - TypeError: can't access property "src", doc.getElementsByTagName(...)[0] is undefined at verifyContent@http://mochi.test:8888/tests/editor/libeditor/tests/test_bug490879.html:15:12
[task 2022-10-18T19:03:12.664Z] 19:03:12     INFO - runTest@http://mochi.test:8888/tests/editor/libeditor/tests/test_bug490879.html:38:3

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6c4fdcf6a69
Editor: Handle nsIFile transferable as blob. r=nika,masayuki
https://hg.mozilla.org/integration/autoland/rev/47dfb29abf33
Remove IsFileImage and SlurpFileToString. r=nika

Comment 9

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/d6c4fdcf6a69
https://hg.mozilla.org/mozilla-central/rev/47dfb29abf33

Comment 10

[Alessandro Castellani [:aleca]]

Unfortunately this busted Thunderbird as we are using SlurpFileToString here: https://searchfox.org/comm-central/rev/809c0b1305c77e3d9b4b90eefed6c1f27c47439c/mailnews/compose/src/nsMsgCompose.cpp#3743

Could I kindly ask for some help in porting this changes?