1424183 - merged Blob from sliced one produce corrupted Blob - follow up

Status: RESOLVED FIXED

(Core :: DOM: File, defect, P1)

Description

[Andrea Marchesini [:baku]]

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.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - too many file descriptors

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.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - closed stream and AsyncWait

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

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - currentStream is closed

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.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 1 - too many file descriptors

gtest was already there. Better to merge the patch.

Comment 5

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

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 6

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

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 7

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

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 8

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

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

Comment 9

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

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."

Comment 10

[Andrea Marchesini [:baku]]

> 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.

Comment 11

[Andrea Marchesini [:baku]]

> 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.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 2 - closed stream and AsyncWait

Comments improved

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 2 - closed stream and AsyncWait

Comment 14

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

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

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 1 - too many file descriptors

Comment 16

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

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.

Comment 17

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

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

Comment 18

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

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 19

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

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

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 4 - be sure to return data

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.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Natalia Csoregi [:nataliaCs]]

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

Comment 23

[Kaiido]

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?

Comment 24

[Andrea Marchesini [:baku]]

Yes, please. Thanks for your test.

Comment 25

[Gerry Chang [:gchang]]

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