1357678 - Streamconverters (including decompressing) block off-main thread delivery?

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Jason Duell]

Dragana tells me that using streamconverters (including gunzip) blocks off-main-thread delivery.

Yet sworkman told me in bug 882996 that unzipping at least does work with off-main-thread.

We should figure out what's going on here--if we are skipping off-main for all gzipped content, that would be worth fixing ASAP.  And if it's easy for us to fix other streamconverters, that would be good too (fine if we split that into separate bugs--less urgent).

Patrick also points out that streamconverters are an XPCOM API, so could be implemented in JS. Let's make sure addons aren't using it (and post-57, no internal JS code).

Comment 1

[Jason Duell]

We should look into the gzip case soon.

Comment 2

[Dragana Damjanovic [:dragana]]

Did not want to clean my neadinfo

Comment 3

[Dragana Damjanovic [:dragana]]

nsHTTPCompressConv.h does not implement nsIRetargetableStreamListener so it will not od off-main thread.


I will check the code to see if it is safe or I will make it safe to do retargeting.

Comment 4

[Dragana Damjanovic [:dragana]]

I need one explanation, maybe it is a stupid question:

OnDataAvailable will be delivered off-the main thread and OnStart/StopRequest on the main thread. We guarantee that  OnDataAvailable will be dispatched before OnStopRequest, or not? OnDataAvailable will start before OnStopRequest? The can run in parallel?

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Dragana Damjanovic [:dragana] from comment #4)
> I need one explanation, maybe it is a stupid question:
> 
> OnDataAvailable will be delivered off-the main thread and
> OnStart/StopRequest on the main thread. We guarantee that  OnDataAvailable
> will be dispatched before OnStopRequest, or not? OnDataAvailable will start
> before OnStopRequest? The can run in parallel?

It will be ensured for them to be fully sequenced.  A followup callback is not called until the current exits.

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

As comment #5, OnStopRequest is guaranteed to be called after OnDataAvailable is finished even if OMT is enabled.

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: bug_1357678.patch

Comment 8

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a12194138f2b7276277542fab873ebc0ce2b334

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8869989 [details] [diff] [review]
bug_1357678.patch

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

I'll look over this patch in more detail.  I believe you are overlocking.

::: netwerk/streamconv/converters/nsHTTPCompressConv.cpp
@@ +126,1 @@
>    return mListener->OnStartRequest(request, aContext);

oh!!  very bad! never call an unknown code while holding your lock.  if there is a reentrancy path, you are gonna blow.

if you need to protect mListener access, then swap to local comptr (something you _MUST_ do anyway!)

@@ +659,5 @@
> +  if (!listener) {
> +    return NS_ERROR_NO_INTERFACE;
> +  }
> +
> +  return listener->CheckListenerChain();

as well here

Comment 10

[Dragana Damjanovic [:dragana]]

nsHTTPCompressConv is very isolated code. I do not expect reentrance any where. Actually if we guarantee taht we do not call OnStop before OnData returns and OnData before OnStart, I think I can skip locking all together.

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8869989 [details] [diff] [review]
bug_1357678.patch

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

::: netwerk/streamconv/converters/nsHTTPCompressConv.cpp
@@ +132,5 @@
>  {
>    nsresult status = aStatus;
>    LOG(("nsHttpCompresssConv %p onstop %" PRIx32 "\n", this, static_cast<uint32_t>(aStatus)));
>    
> +  MutexAutoLock lock(mMutex);

external code must not be called outside the lock as well.  all members you call on has to be swapped to local comptrs and called on from those ptrs outside the lock.

@@ +260,5 @@
>      NS_ERROR("count of zero passed to OnDataAvailable");
>      return NS_ERROR_UNEXPECTED;
>    }
>  
> +  MutexAutoLock lock(mMutex);

this one, as is, is also definitely wrong.  I don't expect OnData being called concurrently, so I believe only thing you need is to protected the members' access.  hence, localy comptrs again, as in the preview case.  and I believe calls on iStr should be outside the lock as well.

@@ +659,5 @@
> +  if (!listener) {
> +    return NS_ERROR_NO_INTERFACE;
> +  }
> +
> +  return listener->CheckListenerChain();

lock only over |listener = do_QueryInterface(mListener);|

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Dragana Damjanovic [:dragana] from comment #10)
> nsHTTPCompressConv is very isolated code. I do not expect reentrance any
> where. Actually if we guarantee taht we do not call OnStop before OnData
> returns and OnData before OnStart, I think I can skip locking all together.

That would be the best option, yes.  But we also must make TSan happy...  hence, up to you.

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #11)
> external code must not be called outside the lock as well.

external code *_must be_ called outside the lock* as well.

Comment 14

[Dragana Damjanovic [:dragana]]

Attachment: bug_1357678.patch

Comment 15

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f78c184ae57e4e0f053c8f2c4e933f5db38305e8

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8870063 [details] [diff] [review]
bug_1357678.patch

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

This all seems a bit strange.  We ensure that OnDataAvail can't be called on whatever thread before OnStart is finished.  Also, we ensure that OnDataAvail is always called (in sequence) on a singe thread.  And we also ensure OnStop is called on the main thread after the last OnDataAvial has finished (no sooner).

Hence, I'm not sure what you are trying to protect here.  Should we rather just assert threading and sequencing with some atomic flags and to make TSan happy just turn some of the status members to be atomic too?

(In reply to Dragana Damjanovic [:dragana] from comment #10)
> nsHTTPCompressConv is very isolated code. I do not expect reentrance any
> where. 

Yes, if it were there we already knew.

> Actually if we guarantee taht we do not call OnStop before OnData
> returns and OnData before OnStart, I think I can skip locking all together.

Me too, maybe really just make some of the important members atomic and we are done...

::: netwerk/streamconv/converters/nsHTTPCompressConv.cpp
@@ +118,2 @@
>  
>    mAsyncConvContext = aCtxt;

this is OK unlocked?

@@ +161,1 @@
>    if (NS_SUCCEEDED(status) && mMode == HTTP_COMPRESS_BROTLI) {

here mMode is OK unlocked?  I know it's r/o here (right?) but TSan won't like it.  Would Atomic for mMode do better here?

@@ +219,5 @@
> +      MutexAutoLock lock(self->mMutex);
> +
> +      res = ::BrotliDecompressStream(
> +        &avail, reinterpret_cast<const unsigned char **>(&dataIn),
> +        &outSize, &outPtr, &self->mBrotli->mTotalOut, &self->mBrotli->mState);

hmm.. how long can this block?  is there a chance we want to enter the lock on a different thread (main thread!) while this is running?  are we protecting the data here?  or state of the brotli decoder?  is this a real protection or is it just TSan-happiness thing?

can this really ever be called on more than one thread over the same data/state?

@@ +289,5 @@
> +  {
> +    MutexAutoLock lock(mMutex);
> +    streamEnded = mStreamEnded;
> +    mode = mMode;
> +  }

so, when some other thread changes this right after we exit this lock, what all may happen?  if that may happen at all?

Comment 17

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #16)
> Comment on attachment 8870063 [details] [diff] [review]
> bug_1357678.patch
> 
> Review of attachment 8870063 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This all seems a bit strange.  We ensure that OnDataAvail can't be called on
> whatever thread before OnStart is finished.  Also, we ensure that
> OnDataAvail is always called (in sequence) on a singe thread.  And we also
> ensure OnStop is called on the main thread after the last OnDataAvial has
> finished (no sooner).
> 
> Hence, I'm not sure what you are trying to protect here.  Should we rather
> just assert threading and sequencing with some atomic flags and to make TSan
> happy just turn some of the status members to be atomic too?
> 

I had a problem with brotli code that is isolated, and I did not want to change it. and I need to change it to make tsan happy.

I think I have an idea how to do that without changing brotli code.

> (In reply to Dragana Damjanovic [:dragana] from comment #10)
> > nsHTTPCompressConv is very isolated code. I do not expect reentrance any
> > where. 
> 
> Yes, if it were there we already knew.
> 
> > Actually if we guarantee taht we do not call OnStop before OnData
> > returns and OnData before OnStart, I think I can skip locking all together.
> 
> Me too, maybe really just make some of the important members atomic and we
> are done...

I did not want to change brotli code therefore I decided to do it this way.

I think I have an idea how to do it without changing Brotli code.

> 
> ::: netwerk/streamconv/converters/nsHTTPCompressConv.cpp
> @@ +118,2 @@
> >  
> >    mAsyncConvContext = aCtxt;
> 
> this is OK unlocked?

This is never used! I could deleted as well.
> 
> @@ +161,1 @@
> >    if (NS_SUCCEEDED(status) && mMode == HTTP_COMPRESS_BROTLI) {
> 
> here mMode is OK unlocked?  I know it's r/o here (right?) but TSan won't
> like it.  Would Atomic for mMode do better here?
> 

It is ok unlocked. I can change it to atomic as well.

> @@ +219,5 @@
> > +      MutexAutoLock lock(self->mMutex);
> > +
> > +      res = ::BrotliDecompressStream(
> > +        &avail, reinterpret_cast<const unsigned char **>(&dataIn),
> > +        &outSize, &outPtr, &self->mBrotli->mTotalOut, &self->mBrotli->mState);
> 
> hmm.. how long can this block?  is there a chance we want to enter the lock
> on a different thread (main thread!) while this is running?  are we
> protecting the data here?  or state of the brotli decoder?  is this a real
> protection or is it just TSan-happiness thing?
> 
> can this really ever be called on more than one thread over the same
> data/state?

Only mBrotli->mTotalOut and mBrotli->mState are access from more than one thread, i.e. in OnDataAvailable (here) and OnStopRequest. So this is only to make tsan happy.

I was think about changing this to atomic but that will mean to change brotli code that I did not want to do! 

> 
> @@ +289,5 @@
> > +  {
> > +    MutexAutoLock lock(mMutex);
> > +    streamEnded = mStreamEnded;
> > +    mode = mMode;
> > +  }
> 
> so, when some other thread changes this right after we exit this lock, what
> all may happen?  if that may happen at all?

mMode is set in AsyncConvertData and it is never changed. AsyncConvertData is called before OnDataAvailable is ever called. It is called right after OnStartRequest on the main thread. So mMode will not change after this lock!!! This is only to make tsan happy.

mStreamEnded cannot change as well. This is only to make tsan happy. mStreamEnded changes in constructor and in this function later on but that will not influence this call.

Comment 18

[Dragana Damjanovic [:dragana]]

Attachment: bug_1357678_v1.patch

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 8871666 [details] [diff] [review]
bug_1357678_v1.patch

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

Thanks!

::: netwerk/streamconv/converters/nsHTTPCompressConv.cpp
@@ +109,2 @@
>    LOG(("nsHttpCompresssConv %p AsyncConvertData %s %s mode %d\n",
> +       this, aFromType, aToType, mode));

nit: you can direct cast mMode to CompressMode for the log arg (it triggers the cast operator on Atomic<>)

::: netwerk/streamconv/converters/nsHTTPCompressConv.h
@@ +68,5 @@
>    {
>      BrotliStateCleanup(&mState);
>    }
>  
> +  BrotliState    mState;

I *think* mState should be atomic too

@@ +71,5 @@
>  
> +  BrotliState    mState;
> +  Atomic<size_t> mTotalOut;
> +  nsresult       mStatus;
> +  Atomic<bool>   mBrotliStateIsStreamEnd;

and I think all can be just Relaxed:

https://dxr.mozilla.org/mozilla-central/source/mfbt/Atomics.h#60

@@ +100,5 @@
>  private:
>      virtual ~nsHTTPCompressConv ();
>  
>      nsCOMPtr<nsIStreamListener> mListener; // this guy gets the converted data via his OnDataAvailable ()
> +    Atomic<CompressMode>        mMode;

I believe this one can be Relaxed

@@ +131,5 @@
>      unsigned mLen, hMode, mSkipCount, mFlags;
>  
>      uint32_t check_header (nsIInputStream *iStr, uint32_t streamLen, nsresult *rv);
>  
> +    Atomic<uint32_t> mDecodedDataLength;

can probably be Relaxed

Comment 20

[Dragana Damjanovic [:dragana]]

I get a crash for some reason... investigating.

Comment 21

[Dragana Damjanovic [:dragana]]

InterceptFailedOnStop in HttpBaseChannel does not implement thread safe nsISupports therefore it was crashing.

InterceptFailedOnStop is introduce because of brotli. I will update the patch and ask Patrick to take a look. I am not sure we still need InterceptFailedOnStop, having a very short look at this - I think that we do not need it.

Comment 22

[Dragana Damjanovic [:dragana]]

> InterceptFailedOnStop is introduce because of brotli. I will update the
> patch and ask Patrick to take a look. I am not sure we still need
> InterceptFailedOnStop, having a very short look at this - I think that we do
> not need it.



short explanation:
brotli used to call BrotliHandler one more time during OnStopRequest, but the brotli code has change and it does not call it any more.

Comment 23

[Dragana Damjanovic [:dragana]]

Attachment: bug_1357678_v1.patch

Patrick, can I change InterceptFailedOnStop to implement NS_DECL_THREADSAFE_ISUPPORTS?

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 8871707 [details] [diff] [review]
bug_1357678_v1.patch

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

thanks

::: netwerk/streamconv/converters/nsHTTPCompressConv.h
@@ +68,5 @@
>    {
>      BrotliStateCleanup(&mState);
>    }
>  
> +  BrotliState             mState;

note that mState is accessed in ondata and onstop as well, if I'm not mistaken.

Comment 25

[Patrick McManus [:mcmanus]]

(In reply to Dragana Damjanovic [:dragana] from comment #23)
> Created attachment 8871707 [details] [diff] [review]
> bug_1357678_v1.patch
> 
> Patrick, can I change InterceptFailedOnStop to implement
> NS_DECL_THREADSAFE_ISUPPORTS?

probably.. your exlpanation makes sense to me at least :)

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47e0df5e8bd1
Make stream converters do omt. r=mayhemer

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47e0df5e8bd1