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

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jduell.mcbugs, Assigned: dragana)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

2 years ago
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).
Reporter

Comment 1

2 years ago
We should look into the gzip case soon.
Flags: needinfo?(dd.mozilla)
Whiteboard: necko-next
Assignee

Updated

2 years ago
Flags: needinfo?(dd.mozilla)
Whiteboard: necko-next → [necko-next]
Assignee

Comment 2

2 years ago
Did not want to clean my neadinfo
Flags: needinfo?(dd.mozilla)
Assignee: nobody → dd.mozilla
Whiteboard: [necko-next] → [necko-active]
Reporter

Updated

2 years ago
Blocks: PBg-HTTP
Assignee

Comment 3

2 years ago
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.
Flags: needinfo?(dd.mozilla)
Assignee

Comment 4

2 years ago
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?
Flags: needinfo?(schien)
(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.
Flags: needinfo?(schien)
Assignee

Comment 7

2 years ago
Posted patch bug_1357678.patch (obsolete) — Splinter Review
Attachment #8869989 - Flags: review?(valentin.gosu)
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
Attachment #8869989 - Flags: review?(honzab.moz)
Assignee

Comment 10

2 years ago
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.
Assignee

Updated

2 years ago
Attachment #8869989 - Attachment is obsolete: true
Attachment #8869989 - Flags: review?(valentin.gosu)
Attachment #8869989 - Flags: review?(honzab.moz)
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);|
Attachment #8869989 - Attachment is obsolete: false
(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.
Assignee

Comment 14

2 years ago
Posted patch bug_1357678.patch (obsolete) — Splinter Review
Attachment #8869989 - Attachment is obsolete: true
Attachment #8870063 - Flags: review?(honzab.moz)
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?
Attachment #8870063 - Flags: feedback-
Assignee

Comment 17

2 years ago
(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.
Assignee

Updated

2 years ago
Attachment #8870063 - Attachment is obsolete: true
Attachment #8870063 - Flags: review?(honzab.moz)
Assignee

Comment 18

2 years ago
Posted patch bug_1357678_v1.patch (obsolete) — Splinter Review
Attachment #8871666 - Flags: review?(honzab.moz)
Assignee

Updated

2 years ago
Attachment #8871666 - Flags: review?(honzab.moz)
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
Assignee

Comment 20

2 years ago
I get a crash for some reason... investigating.
Assignee

Comment 21

2 years ago
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.
Assignee

Comment 22

2 years ago
> 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.
Assignee

Comment 23

2 years ago
Patrick, can I change InterceptFailedOnStop to implement NS_DECL_THREADSAFE_ISUPPORTS?
Attachment #8871666 - Attachment is obsolete: true
Attachment #8871707 - Flags: review?(honzab.moz)
Attachment #8871707 - Flags: feedback?(mcmanus)
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.
Attachment #8871707 - Flags: review?(honzab.moz) → review+
(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 :)
Assignee

Updated

2 years ago
Attachment #8871707 - Flags: feedback?(mcmanus)

Comment 26

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

Comment 27

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/47e0df5e8bd1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.