1451731 - Synchronize access to various stream classes' async wait callback reference

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Honza Bambas (:mayhemer)]

nsBufferedInputStream::AsyncWait and OnInputStreamReady can be called on different threads.

Comment 1

[Honza Bambas (:mayhemer)]

please, next time ask a necko peer to review such changes.

This applies to the following classes:

- nsBufferedInputStream
- nsMIMEInputStream

didn't find more, but there could be.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - nsBufferedInputStream

If you don't mind, I would like to propose this patch. I wrote it because of bug 1434553.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 2 - nsMIMEInputStream

Here the nsIMIMEInputStream part.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 3 - nsMIMEInputStream::AsyncWait to cancel the previous call

AsyncWait() can receive a nullptr callback to abort the previous operation. This is not supported by nsMIMEInputStream.

Comment 5

[Andrea Marchesini [:baku]]

To be honest, I think the problem is a bit bigger than AsyncWait()'s callback. An nsIInputStream is supposed to be thread-safe, but our implementations are not. If 2 threads do a ::Read() at the same time, nsBufferedInputStream, and as many other streams, goes in a wrong state because mCursor, mFillPoint and so on, are not protected by mutex.

At the same time, it's not true that we use inputStreams on multiple threads. We move them cross threads, but when the reading starts, usually we don't change threads.

Probably we should "bind" the use of the inputStreams to a particular thread. When the reading starts, we should not allow any other operation on any other thread.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Andrea Marchesini [:baku] from comment #5)
> To be honest, I think the problem is a bit bigger than AsyncWait()'s
> callback. An nsIInputStream is supposed to be thread-safe, but our
> implementations are not. If 2 threads do a ::Read() at the same time,
> nsBufferedInputStream, and as many other streams, goes in a wrong state
> because mCursor, mFillPoint and so on, are not protected by mutex.
> 
> At the same time, it's not true that we use inputStreams on multiple
> threads. We move them cross threads, but when the reading starts, usually we
> don't change threads.
> 
> Probably we should "bind" the use of the inputStreams to a particular
> thread. When the reading starts, we should not allow any other operation on
> any other thread.

In this bug I just wanted to fix the recent regression by adding a code that is obviously multi-threaded and not correctly locked.

Stream concurrency (read/write) is definitely for a different bug and is a way larger thing.  I don't think there is an immediate problem with that, though.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8966149 [details] [diff] [review]
part 1 - nsBufferedInputStream

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

thanks!

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8966153 [details] [diff] [review]
part 3 - nsMIMEInputStream::AsyncWait to cancel the previous call

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

::: netwerk/base/nsMIMEInputStream.cpp
@@ +285,5 @@
>          mAsyncWaitCallback = aCallback;
> +
> +        if (!mAsyncWaitCallback) {
> +           return NS_OK;
> +        }

After thinking about this more, I believe you should call AsyncWait(null) on the inner stream too.  This should apply to all implementations of wrapping asyncWait():

nsMIMEInputStream::AsyncWait
nsBufferedInputStream::AsyncWait
StreamWrapper::AsyncWait
PartiallySeekableInputStream::AsyncWait
SlicedInputStream::AsyncWait

The main reason is to release references and prevent leaks and also ensure the state to "not wait anymore" of the inner stream is passed on it.

if you have a strong reason against, please let me know.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 1 - nsBufferedInputStream

I agree with you. Let's start from scratch.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 2 - nsMIMEInputStream

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 3 - PartialSeekableInputStream

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 4 - SlicedInputStream

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 5 - IDB StreamWrapper

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 6 - IPCBlobInputStream

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 8966469 [details] [diff] [review]
part 2 - nsMIMEInputStream

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

r+ with one suggestion that probably applies to all these patches here.

::: netwerk/base/nsMIMEInputStream.cpp
@@ +280,1 @@
>      }

hmm... what about just:

nsCOMPtr<nsIInputStreamCallback> callback = aCallback ? this : nullptr;

lock..

if (mAsyncWaitCallback && aCallback) return ERROR; // eventually* removed
mAsyncWaitCallback = aCallback;

..unlock

asyncStream->AsyncWait(callback...);


that ensures that every (eventually*) call to this stream asyncWait will be passed on the inner stream asyncWait

@@ +305,1 @@
>    return callback->OnInputStreamReady(this);

indention again

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8966471 [details] [diff] [review]
part 4 - SlicedInputStream

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

r+ but here with the comments addressed

::: xpcom/io/SlicedInputStream.cpp
@@ +377,5 @@
>        uint64_t bufCount = XPCOM_MIN(mStart - mCurPos, (uint64_t)sizeof(buf));
>        nsresult rv = mInputStream->Read(buf, bufCount, &bytesRead);
>        if (NS_SUCCEEDED(rv) && bytesRead == 0) {
>          mClosed = true;
> +        return RunAsyncWaitCallback(lock);

hmm... would be nice to refactor this somehow so that we don't have to reacquire/release the lock one more time for nothing before return.

potentially mozilla::ScopeExit would help?

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 8966475 [details] [diff] [review]
part 6 - IPCBlobInputStream

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

IPCBlobInputStream will definitely need some more (followup) adjustments...

r+ but with some hesitation...

::: dom/file/ipc/IPCBlobInputStream.cpp
@@ +446,5 @@
> +
> +    if (inputStreamCallback) {
> +      MaybeExecuteInputStreamCallback(inputStreamCallback,
> +                                      inputStreamCallbackEventTarget,
> +                                      lock);

again would be good not do the additional purpose-less acq/rel here, same as in the SlicedInputStream patch.

@@ +488,1 @@
>    nsresult rv = EnsureAsyncRemoteStream();

!!! 

another place that likely needs some locking.  this method plays with members (refptrs) of this class.  hence, it must either assert single thread call (likely not an option, class is thread-safe and apparently this can be called on any thread concurrently) or properly locked as well.

another followup needs to be filed for this...

@@ +521,2 @@
>    // We have been closed in the meantime.
>    if (mState == eClosed) {

should we sync these too?

something tells me this class would use an overall threading audit...

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24404f2f582b
Synchronize access to various stream classes' async wait callback reference - part 1 - nsBufferedInputStream, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc8215fe458b
Synchronize access to various stream classes' async wait callback reference - part 2 - nsMIMEInputStream, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f3f2ae6110e
Synchronize access to various stream classes' async wait callback reference - part 3 - SlicedInputStream, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/97276cd45114
Synchronize access to various stream classes' async wait callback reference - part 4 - PartiallySeekableInputStream, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/14872b286b03
Synchronize access to various stream classes' async wait callback reference - part 5 - FileSnapshot StreamWrapper, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4200189c8ae
Synchronize access to various stream classes' async wait callback reference - part 6 - IPCBlobInputStream, r=mayhemer

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to Pulsebot from comment #18)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/24404f2f582b
> Synchronize access to various stream classes' async wait callback reference
> - part 1 - nsBufferedInputStream, r=mayhemer
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fc8215fe458b
> Synchronize access to various stream classes' async wait callback reference
> - part 2 - nsMIMEInputStream, r=mayhemer
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1f3f2ae6110e
> Synchronize access to various stream classes' async wait callback reference
> - part 3 - SlicedInputStream, r=mayhemer
> https://hg.mozilla.org/integration/mozilla-inbound/rev/97276cd45114
> Synchronize access to various stream classes' async wait callback reference
> - part 4 - PartiallySeekableInputStream, r=mayhemer
> https://hg.mozilla.org/integration/mozilla-inbound/rev/14872b286b03
> Synchronize access to various stream classes' async wait callback reference
> - part 5 - FileSnapshot StreamWrapper, r=mayhemer
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e4200189c8ae
> Synchronize access to various stream classes' async wait callback reference
> - part 6 - IPCBlobInputStream, r=mayhemer

How did this happen?  I did require some updates to the patches before landing ("r+ with comments")

OK, seems like one needs to r- even for a trailing space.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 7 - SlicedInputStream - no extra lock

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part 8 - IPCBlobInputStream - no extra lock

Comment 22

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/24404f2f582b
https://hg.mozilla.org/mozilla-central/rev/fc8215fe458b
https://hg.mozilla.org/mozilla-central/rev/1f3f2ae6110e
https://hg.mozilla.org/mozilla-central/rev/97276cd45114
https://hg.mozilla.org/mozilla-central/rev/14872b286b03
https://hg.mozilla.org/mozilla-central/rev/e4200189c8ae

Comment 23

[Andrea Marchesini [:baku]]

There are still a couple of patches to land. I'll move them in a separate bug if the reviewer wants, and before landing.

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 8966653 [details] [diff] [review]
part 7 - SlicedInputStream - no extra lock

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

looks good, thanks.

::: xpcom/io/SlicedInputStream.cpp
@@ +368,5 @@
> +      return NS_OK;
> +    }
> +
> +    auto raii = MakeScopeExit([&] {
> +      mAsyncWaitCallback = nullptr;

maybe add MOZ_ASSERT(mMutex.CurrentThreadOwns()) to make it more clear why this is here?

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 8966662 [details] [diff] [review]
part 8 - IPCBlobInputStream - no extra lock

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

::: dom/file/ipc/IPCBlobInputStream.cpp
@@ +401,5 @@
> +      return rv;
> +    }
> +
> +    MOZ_ASSERT(mAsyncRemoteStream);
> +    return mAsyncRemoteStream->AsyncWait(isNullCallback ? nullptr : this,

any reason to not use aCallback instead of isNullCallback?

@@ +478,1 @@
>    }

are you missing `else { rv = mAsyncRemoteStream->AsyncWait(nullptr, ...); }`?

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 8 - IPCBlobInputStream - no extra lock

Comment 27

[Andrea Marchesini [:baku]]

Attachment: part 8 - IPCBlobInputStream - no extra lock

We don't need to call AsyncWait() passing nullptr when the stream is received.

Comment 28

[Honza Bambas (:mayhemer)]

Comment on attachment 8967409 [details] [diff] [review]
part 8 - IPCBlobInputStream - no extra lock

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

thanks!