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

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Posted patch transport.patch (obsolete) — Splinter Review
Attachment #8882719 - Flags: review?(bugs)
Comment on attachment 8882719 [details] [diff] [review]
transport.patch

I'm not completely happy with this patch. Removing the review request.
Attachment #8882719 - Flags: review?(bugs)
Attachment #8882719 - Attachment is obsolete: true
Attachment #8884921 - Flags: review?(bugs)
Attachment #8884923 - Flags: review?(bugs)
Posted patch part 4 - nsIAsyncFileMetadata (obsolete) — Splinter Review
Attachment #8884929 - Flags: review?(bugmail)
Comment on attachment 8884921 [details] [diff] [review]
part 1 - blocking stream and FileReader

Why this patch is needed?
Attachment #8884921 - Flags: review?(bugs)
Comment on attachment 8884923 [details] [diff] [review]
part 2 - no seekable IPCBlobInputStream

Also this, why? Please explain this all.
Attachment #8884923 - Flags: review?(bugs)
Attachment #8884927 - Flags: review?(bugs) → review+
(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.
(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 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.
Attachment #8884929 - Flags: review?(bugmail) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d830efaed244
FileReader should create a pipe if the stream is blocking, r=smaug
https://hg.mozilla.org/mozilla-central/rev/d830efaed244
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Attachment #8884923 - Attachment is obsolete: true
Duplicate of this bug: 1380618
Attachment #8884929 - Attachment is obsolete: true
Attachment #8884927 - Attachment is obsolete: true
Should we uplift this to beta?
Flags: needinfo?(amarchesini)
Together with bug 1381465 when reviewed.
Flags: needinfo?(amarchesini)
Blocks: 1381465
Blocks: 1397627
No longer blocks: 1381465
You need to log in before you can comment on or make changes to this bug.