1377589 - IPCBlobInputStream should always work with nsIAsyncStream in order to avoid I/O on main-thread

Status: RESOLVED FIXED

(Core :: DOM: File, enhancement)

Description

[Andrea Marchesini [:baku]]

IPCBlobInputStream receives a remote inputStream from the parent process when AsyncWait() is called. The remote inputStream could be a non-nsIAsyncInputStream File stream in case we are dealing with real files.

Any additional read() will end up calling the remote inputStream->read() on the current thread and this is bad because it could involve doing I/O.

The solution is to transform the remote inputStream to a nsIAsyncInputStream if it does not already implement such interface. In my patch this is done using nsITransport.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: transport.patch

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8882719 [details] [diff] [review]
transport.patch

I'm not completely happy with this patch. Removing the review request.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - blocking stream and FileReader

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - no seekable IPCBlobInputStream

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 3 - IPCBlobInputStream and non-blocking remote streams

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 4 - nsIAsyncFileMetadata

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8884921 [details] [diff] [review]
part 1 - blocking stream and FileReader

Why this patch is needed?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8884923 [details] [diff] [review]
part 2 - no seekable IPCBlobInputStream

Also this, why? Please explain this all.

Comment 9

[Andrea Marchesini [:baku]]

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8884921 [details] [diff] [review]
> part 1 - blocking stream and FileReader
> 
> Why this patch is needed?

This can be a separate bug. I just found that FileReader doesn't check if the stream is blocking or not blocking before using it.

Comment 10

[Andrea Marchesini [:baku]]

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8884923 [details] [diff] [review]
> part 2 - no seekable IPCBlobInputStream
> 
> Also this, why? Please explain this all.

Pipe streams are not seekable. With this new approach, rarely a IPCBlobInputStream would be a nsISeekableStream and, btw, nothing is using it as seekable. Better to remove non-used code.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8884929 [details] [diff] [review]
part 4 - nsIAsyncFileMetadata

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

== Restating

How things work before the patch:
- PreprocessHelper::ProcessCurrentStreamPair wants to get PRFileDesc*'s to the bytecode and compiled files because JS::DeserializeWasmModule wants to mmap() the files.
- It has a simple state machine that considers the bytecode and compiled files in sequence, first synchronously trying GetFileDescriptorFromStream and going async with WaitForStreamReady if that fails.  This was largely introduced in bug 1360185 part 5's prior IPCBlob changes.
- GetFileDescriptorFromStream extracts the file descriptor via the (sync) nsIFileMetadata interface.
- WaitForStreamReady uses nsIAsyncInputStream::AsyncWait.  We will only AsyncWait for one stream at a time so there is no ambiguity about what stream/fd the callback corresponds to.

This (part 4) patch is necessary because:
- nsFileInputStream IPC-serializes so it's still an nsFileInputStream in the child.  It is a blocking stream.
- For sanity in the face of diverse blob sources, we don't want IPCBlob* consumers to have to worry that read calls might be blocking (or screw up and do blocking reads), so part 3 normalizes streams that are non-async or non-non-blocking to be non-blocking via nsIStreamTransportService::CreateInputTransport(aCloseWhenDone=true)/OpenInputStream which ends up being NS_NewPipe2 and NS_AsyncCopy run on the STS threadpool.
- IPCBlobInputStream's nsIFileMetadata::getFileDescriptor implementation operates on mRemoteStream.  Part 3 does not change that we hold onto mRemoteStream and use it for this.
- When the mAsyncRemoteStream NS_AsyncCopy completes, it will close mRemoteStream.  This will cause either cause nsFileStreamBase::GetFileDescriptor (via DoPendingOpen) to return NS_BASE_STREAM_CLOSED or nsFileStreamBase::Close() to potentially PR_Close() the fd out from under the reference held by PreprocessHelper (which may not have invoked JS::DeserializeWasmModule logic yet because it is asynchronously waiting on the compiled stream).  Either way this breaks WASM deserialization.
- The NS_AsyncCopy is by definition a race.

The patch makes things work by:
- Avoid the mRemoteStream-closing race by creating a new nsIAsyncFileMetadata::asyncWait() method that will not trigger part 3's new EnsureAsyncRemoteStream() method that creates the pipe and racing NS_AsyncCopy.
- Altering PreprocessHelper::WaitForStreamReady to use that new asyncWait() instead of the race-triggering one.  Its state machine is otherwise unchanged.

::: dom/file/ipc/IPCBlobInputStream.cpp
@@ +117,5 @@
>    NS_INTERFACE_MAP_ENTRY(nsIInputStreamCallback)
>    NS_INTERFACE_MAP_ENTRY(nsICloneableInputStream)
>    NS_INTERFACE_MAP_ENTRY(nsIIPCSerializableInputStream)
> +  NS_INTERFACE_MAP_ENTRY(nsIAsyncFileMetadata)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIFileMetadata, nsIAsyncFileMetadata)

Does this need to be ambiguous?  nsIFileMetadata is only present via nsIAsyncFileMetadata.

::: dom/indexedDB/ActorsChild.cpp
@@ +3607,5 @@
> +BackgroundRequestChild::
> +PreprocessHelper::OnFileMetadataReady(nsIAsyncFileMetadata* aObject)
> +{
> +  nsCOMPtr<nsIInputStream> stream = do_QueryInterface(aObject);
> +  MOZ_ASSERT(stream, "It was a stream before!");

This is a delightfully amusing assertion error string.

::: netwerk/base/nsIFileStreams.idl
@@ +185,5 @@
>  /**
>   * An interface that allows you to get some metadata like file size and
> + * file last modified time. These methods and attributes can throw
> + * NS_BASE_STREAM_WOULD_BLOCK in case the informations are not available yet.
> + * If this happens, consider the use of nsIAsyncFileMetadata.

I would suggest also adding a comment like the following that captures that IPCBlobInputStream's EnsureAsyncRemoteStream() method breaks this interface even if you use nsIAsyncFileMetadata::asyncWait():

If using nsIAsyncFileMetadata, you should retrieve any data via this interface before taking any action that might consume the underlying stream.  For example, once Available(), Read(), or nsIAsyncInputStream::AsyncWait() are invoked, these methods may return NS_BASE_STREAM_CLOSED.  This will happen when using IPCBlobInputStream with an underlying file stream, for example.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d830efaed244
FileReader should create a pipe if the stream is blocking, r=smaug

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[Andrew Overholt [:overholt]]

Should we uplift this to beta?

Comment 16

[Andrea Marchesini [:baku]]

Together with bug 1381465 when reviewed.