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

We should look into the gzip case soon.

Did not want to clean my neadinfo

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.

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?

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

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

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

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 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);|

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

(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 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?

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

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

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.

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

Patrick, can I change InterceptFailedOnStop to implement NS_DECL_THREADSAFE_ISUPPORTS?

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.

(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 :)

Pushed by dd.mozilla@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/47e0df5e8bd1 Make stream converters do omt. r=mayhemer