Closed Bug 1778565 Opened 2 years ago Closed 2 years ago

HTMLEditor::InsertObject incorrectly handles reading input streams from blobs

Categories

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

defect

Tracking

()

RESOLVED FIXED
108 Branch
Tracking Status
firefox108 --- fixed

People

(Reporter: nika, Assigned: evilpie)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.

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.

OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo) → needinfo?(masayuki)
Attachment #9291847 - Attachment is obsolete: true
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Blocks: 1699743

Depends on D155753

Flags: needinfo?(masayuki)
Severity: -- → S3
Priority: P3 → P2
Pushed by evilpies@gmail.com: 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

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
Flags: needinfo?(evilpies)
Pushed by evilpies@gmail.com: 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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 108 Branch

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?

Flags: needinfo?(nika)
Flags: needinfo?(nika)
Flags: needinfo?(evilpies)
Regressions: 1808177
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: