This is the continuation of bug 1421176. I have to admit that when I was testing that issue I was using 4096 as a buffer size to make the test faster. This was hiding bugs in nsMultiplexInputStream when using the testcase from bug 1421176.

nsMultiplexInputStream::AsyncWait() calls asyncWait for any substream. This ends up creating too many file descriptors in this scenario: DOM File pointing to a underlying OS File, split in more than 1024 parts, and reassembled. Initially, the multiple asyncWait() calls were needed because ::Available() was counting the available size for each substream. This is not the case anymore after bug 1421176. This means that we can call asyncWait() only on the current stream. Bonus goal, we don't have to use the main-thread if the eventTarget is not passed. I have a separate patch with a gtest.

We must call the nsIInputStreamCallback callback also when the stream is already closed. This is correct following nsIAsyncInputStream.idl and it makes nsInputStreamPump happy.

Last part is about ::Available(). If the current stream is closed, we must move to the following one, otherwise, if ::AsyncWait() is called, we end up calling ::AsyncWait() on the current stream where there is nothing else to read.

gtest was already there. Better to merge the patch.

Comment on attachment 8935685 [details] [diff] [review] part 2 - closed stream and AsyncWait ok, I don't yet understand the later part of the patch.

Comment on attachment 8935687 [details] [diff] [review] part 3 - currentStream is closed ok, looks like also elsewhere mCurrentStream may point outside mStreams. A bit worrisome, but since that is the case elsewhere too, fine.

Comment on attachment 8935693 [details] [diff] [review] part 1 - too many file descriptors >@@ -179,20 +175,16 @@ TEST(TestMultiplexInputStream, AsyncWait > RefPtr<testing::InputStreamCallback> cb = > new testing::InputStreamCallback(); > > ASSERT_EQ(NS_OK, ais->AsyncWait(cb, nsIAsyncInputStream::WAIT_CLOSURE_ONLY, > 0, nullptr)); > ASSERT_FALSE(cb->Called()); > > ais->CloseWithStatus(NS_ERROR_FAILURE); >- ASSERT_FALSE(cb->Called()); >- >- // Eventually it is called. >- MOZ_ALWAYS_TRUE(mozilla::SpinEventLoopUntil([&]() { return cb->Called(); })); > ASSERT_TRUE(cb->Called()); Why SpinEventLoopUntil can be removed? AsyncWait says that the callback should be called async. That hints aboud some unexpected behavior here.

Comment on attachment 8935685 [details] [diff] [review] part 2 - closed stream and AsyncWait I tried and failed to understand - if (mCurrentStream < mStreams.Length() && + if (NS_SUCCEEDED(mStatus) && + mCurrentStream < mStreams.Length() && Please add a comment there explaining why we don't use the stream in NS_FAILED case

FWIW, I think the documentation is unclear here. https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/xpcom/io/nsIAsyncInputStream.idl#56-57 What does "event will be dispatched immediately" means? OnInputStreamReady is not any kind of event or runnable. And the first sentence about the method says "Asynchronously wait for the stream to be readable or closed."

> AsyncWait says that the callback should be called async. > That hints aboud some unexpected behavior here. Absolutely not. This is the behavior described in nsIAsyncInputStream::asyncWait and it is the same behavior implemented elsewhere. IPCBlobInputStream, pipe, SlicedInputStream and so on.

> And the first sentence about the method says > "Asynchronously wait for the stream to be readable or closed." https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/xpcom/io/nsIAsyncInputStream.idl#71-74 * @param aEventTarget * Specify NULL to receive notification on ANY thread (possibly even * recursively on the calling thread -- i.e., synchronously), or * specify that the notification be delivered to a specific event * target.

Comments improved

Please ask then re-review for part 1. I have not reviewed it. At least the documentation needs to be fixed.

BTW, since asyncWait isn't then really async, I'm pretty sure we have bugs where code expects it to be async. We should either rename it or fix it to be always async. The current setup is totally broken.

But that isn't really about this bug. But just very very error prone setup.

Comment on attachment 8936228 [details] [diff] [review] part 1 - too many file descriptors I don't understand why we need to inject the broken AsyncWait handling here too. Why we can't keep async handling always? Either I'm missing something, or removing that asynchronousness has nothing to do with this bug. Please explain.

Comment on attachment 8936228 [details] [diff] [review] part 1 - too many file descriptors > nsMultiplexInputStream::Close() > { >- MutexAutoLock lock(mLock); >- mStatus = NS_BASE_STREAM_CLOSED; >+ nsTArray<nsCOMPtr<nsIInputStream>> streams; >+ >+ // Let's take a copy of the streams becuase, calling close() it could trigger because

This is the last part: nsIAsyncInputStream must always have data (if not closed) after a OnInputStreamReady(). We must be sure that nsMultiplexInputStream behaves in this way. This patch expands an existing test to be sure that all the possible combinations are covered. If you want, I can add a gtest as well.

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/26cc9645024a nsMultiplexInputStream must not call AsyncOpen on more than the current stream, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0f2d6cd33d nsMultiplexInputStream must call OnInputStreamReady when closed, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/c5fd38f7fea7 nsMultiplexInputStream must move to the following stream if the current one doesn't have data to read, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/dfeab081202f nsMultiplexInputStream must call OnInputStreamReady only when there is data available, r=smaug

https://hg.mozilla.org/mozilla-central/rev/26cc9645024a https://hg.mozilla.org/mozilla-central/rev/4d0f2d6cd33d https://hg.mozilla.org/mozilla-central/rev/c5fd38f7fea7 https://hg.mozilla.org/mozilla-central/rev/dfeab081202f

You will hate me... It seems to have fixed something, but I guess there is still something wrong: Have a look at https://jsfiddle.net/bbnh7L7n/ Choose a big File (at least a few MB) On my computer, after clicking on "download", the download speed is around 30ko/s. This can also be seen when trying to play the merged video blob in a video element, somehow, the video element takes a long time to load it. And worse, after cancelling the downloading, FF eventually crashes. What is really weird though, is that this doesn't happens with Blob loaded through network (e.g fetch#blob()). In this case, there is absolutely no problem. Should I open a new issue?

Yes, please. Thanks for your test.

Too late for Beta58. Mark 58 won't fix.