Closed Bug 1451731 Opened 3 years ago Closed 3 years ago

Synchronize access to various stream classes' async wait callback reference

Categories

(Core :: Networking, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mayhemer, Assigned: baku)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(8 files, 5 obsolete files)

4.17 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
3.79 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
4.76 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
7.67 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
3.46 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
8.34 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
8.04 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
4.78 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
nsBufferedInputStream::AsyncWait and OnInputStreamReady can be called on different threads.
Whiteboard: [necko-triaged]
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.
Summary: Synchronize access to nsBufferedInputStream.mAsyncWaitCallback → Synchronize access to various stream classes' async wait callback reference
Whiteboard: [necko-triaged]
Whiteboard: [necko-triaged]
Attached patch part 1 - nsBufferedInputStream (obsolete) — Splinter Review
If you don't mind, I would like to propose this patch. I wrote it because of bug 1434553.
Assignee: honzab.moz → amarchesini
Attachment #8966149 - Flags: review?(honzab.moz)
Attached patch part 2 - nsMIMEInputStream (obsolete) — Splinter Review
Here the nsIMIMEInputStream part.
Attachment #8966152 - Flags: review?(honzab.moz)
Attachment #8966149 - Attachment description: fs_buffer_fix.patch → part 1 - nsBufferedInputStream
AsyncWait() can receive a nullptr callback to abort the previous operation. This is not supported by nsMIMEInputStream.
Attachment #8966153 - Flags: review?(honzab.moz)
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.
Flags: needinfo?(honzab.moz)
(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.
Flags: needinfo?(honzab.moz)
Comment on attachment 8966149 [details] [diff] [review]
part 1 - nsBufferedInputStream

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

thanks!
Attachment #8966149 - Flags: review?(honzab.moz) → review+
Attachment #8966152 - Flags: review?(honzab.moz) → review+
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.
Attachment #8966153 - Flags: review?(honzab.moz) → review-
I agree with you. Let's start from scratch.
Attachment #8966149 - Attachment is obsolete: true
Attachment #8966468 - Flags: review?(honzab.moz)
Attachment #8966469 - Flags: review?(honzab.moz)
Attachment #8966152 - Attachment is obsolete: true
Attachment #8966153 - Attachment is obsolete: true
Attachment #8966470 - Flags: review?(honzab.moz)
Attachment #8966471 - Flags: review?(honzab.moz)
Attachment #8966472 - Flags: review?(honzab.moz)
Attachment #8966475 - Flags: review?(honzab.moz)
Attachment #8966468 - Flags: review?(honzab.moz) → review+
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
Attachment #8966469 - Flags: review?(honzab.moz) → review+
Attachment #8966470 - Flags: review?(honzab.moz) → review+
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?
Attachment #8966471 - Flags: review?(honzab.moz) → review+
Attachment #8966472 - Flags: review?(honzab.moz) → review+
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...
Attachment #8966475 - Flags: review?(honzab.moz) → review+
Pushed by amarchesini@mozilla.com:
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
(In reply to Pulsebot from comment #18)
> Pushed by amarchesini@mozilla.com:
> 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.
Attachment #8966653 - Flags: review?(honzab.moz)
Attachment #8966662 - Flags: review?(honzab.moz)
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 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?
Attachment #8966653 - Flags: review?(honzab.moz) → review+
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, ...); }`?
Attachment #8966662 - Flags: review?(honzab.moz) → review-
Attachment #8966662 - Attachment is obsolete: true
Attachment #8967130 - Flags: review?(honzab.moz)
We don't need to call AsyncWait() passing nullptr when the stream is received.
Attachment #8967130 - Attachment is obsolete: true
Attachment #8967130 - Flags: review?(honzab.moz)
Attachment #8967409 - Flags: review?(honzab.moz)
Comment on attachment 8967409 [details] [diff] [review]
part 8 - IPCBlobInputStream - no extra lock

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

thanks!
Attachment #8967409 - Flags: review?(honzab.moz) → review+
Blocks: 1453955
You need to log in before you can comment on or make changes to this bug.