Closed Bug 1024027 Opened 6 years ago Closed 6 years ago

Cannot safely dispatch to nsStreamTransportService from a non-main thread

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bent.mozilla, Assigned: seward.zheng)

Details

Attachments

(3 files, 5 obsolete files)

nsStreamTransportService has a threadsafe refcount but must be created and destroyed on the main thread. Its Dispatch() method doesn't handle racing with the main thread.
Attached patch Phase 1: Add lock to STS (obsolete) — Splinter Review
Assignee: nobody → szheng
Status: NEW → ASSIGNED
Attachment #8439588 - Flags: review?(benjamin)
Benjamin, I am not sure if you are fine to review this. If you know a better reviewer, could you help adding him/her?

I appreciate!
Attachment #8439591 - Flags: review?(benjamin)
Comment on attachment 8439588 [details] [diff] [review]
Phase 1: Add lock to STS

I am not a necko person.
Attachment #8439588 - Flags: review?(benjamin) → review?(mcmanus)
Comment on attachment 8439591 [details] [diff] [review]
Phase 2: Add additional check in PutEvent

Does this need to be NS_WARN_IF or can we use MOZ_ASSERT here? I prefer MOZ_ASSERT unless it turns existing tests orange.
Attachment #8439591 - Flags: review?(benjamin) → review+
Comment on attachment 8439588 [details] [diff] [review]
Phase 1: Add lock to STS

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

lgtm; thanks.
Attachment #8439588 - Flags: review?(mcmanus) → review+
Comment on attachment 8439588 [details] [diff] [review]
Phase 1: Add lock to STS

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

I hate to barge in but I think this patch has some problems. Patrick, do you disagree with these changes?

::: netwerk/base/src/nsStreamTransportService.cpp
@@ +508,1 @@
>      NS_ENSURE_TRUE(mPool, NS_ERROR_NOT_INITIALIZED);

This needs to be changed to 'pool', not 'mPool'. Otherwise the dance above is useless :)

@@ +557,1 @@
>      mPool->Shutdown();

Eek! This will spin the event loop with the lock held. That's a recipe for deadlocks.

I think you should just swap the mPool pointer to a local stack pointer, drop the lock, and then call Shutdown() on the stack pointer.
Attachment #8439588 - Flags: review-
Attachment #8439588 - Flags: feedback?(mcmanus)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Does this need to be NS_WARN_IF or can we use MOZ_ASSERT here? I prefer
> MOZ_ASSERT unless it turns existing tests orange.

From reading the code it looks to me that calling Dispatch() on one thread while another thread calls Shutdown() can actually land you in PutEvent(). The only way to be safe is to check mShutdown with the mutex held in PutEvent(), so I don't think this can be asserted.
Well... my assertion is that clients should not be Dispatch()ing to a thread which might be simultaneously Shutdown(). That sounds like a design error.
Hrm. The STS is using the thread pool here, so if we don't handle this potential race in the threadpool itself then we'll need a different plan for how to handle the race in the STS I guess...
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #7)
> Comment on attachment 8439588 [details] [diff] [review]
> Phase 1: Add lock to STS
> 
> Review of attachment 8439588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I hate to barge in but I think this patch has some problems. Patrick, do you
> disagree with these changes?
> 
> ::: netwerk/base/src/nsStreamTransportService.cpp
> @@ +508,1 @@
> >      NS_ENSURE_TRUE(mPool, NS_ERROR_NOT_INITIALIZED);
> 
> This needs to be changed to 'pool', not 'mPool'. Otherwise the dance above
> is useless :)
> 
> @@ +557,1 @@
> >      mPool->Shutdown();
> 
> Eek! This will spin the event loop with the lock held. That's a recipe for
> deadlocks.
> 
> I think you should just swap the mPool pointer to a local stack pointer,
> drop the lock, and then call Shutdown() on the stack pointer.

I believe you are right. The performance test did show this problem (stuck). I will do further tests to see if it helps.
Comment on attachment 8439588 [details] [diff] [review]
Phase 1: Add lock to STS

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

sorry I misinterpreted this before. ben is right. (generally a safe bet.)

::: netwerk/base/src/nsStreamTransportService.cpp
@@ +508,1 @@
>      NS_ENSURE_TRUE(mPool, NS_ERROR_NOT_INITIALIZED);

yes - both the ensure and the ->dispatch

@@ +557,1 @@
>      mPool->Shutdown();

I agree.. make mpool null under the lock and do the actual shutdown of it after the lock is released.
Attachment #8439588 - Flags: review+
Attachment #8439588 - Flags: feedback?(mcmanus)
Attachment #8439588 - Flags: feedback-
Attached patch Phase 1: Add lock to STS (obsolete) — Splinter Review
These changes have been made.
Attachment #8439588 - Attachment is obsolete: true
Attachment #8441683 - Flags: review?(mcmanus)
Attachment #8441683 - Flags: review?(bent.mozilla)
Attached patch Phase 1: Add lock to STS (obsolete) — Splinter Review
Sry, this is the right one.
Attachment #8441683 - Attachment is obsolete: true
Attachment #8441683 - Flags: review?(mcmanus)
Attachment #8441683 - Flags: review?(bent.mozilla)
Attachment #8441685 - Flags: review?(mcmanus)
Attachment #8441685 - Flags: review?(bent.mozilla)
Comment on attachment 8441685 [details] [diff] [review]
Phase 1: Add lock to STS

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

my only problem here is that the lock ends up in a logical sense locking code paths instead of locking data. That's something I try and avoid - its known to be fragile and mistake prone. Even with your short path I had to spend a chunk of time wondering if dispatch on the main thread needed the lock or not (I agree it technically didn't).

The easy way to fix it is to change the lock to be "mShutdownLock" defined to protect access to a new bool "mIsShutdown"..

hold that lock during dispatch. My preference would be for all threads to hold it for all of dispatch (just put MutexAutoLock lock(mShutdownLock) on the stack) and just do MOZ_ASSERT (!mIsShutdown). Similarly Observe() can just put the autolock on the stack and set mIsShutdown to true.
Attachment #8441685 - Flags: review?(mcmanus)
I think even comment 15 is wrong due to my shorthand.. in the sense that the ASSERT isn't correct and of course the lock can't be held while calling shutdown() as ben mentioned

Dispatch() needs to 
{
 nsCOMPtr pool;
 {
   autolock(shutdownlock);
   if (mIsShutdown) return NS_ERROR_MUMBLE;
   pool = mPool;
 } 
 ensure_true(pool)
 return pool->dispatch()
}

and Observe() needs to
{
 {
   autolock(shutdownlock);
   mIsShutdown = true;
  }
 
 if (mPool) {
   mPool->Shutdown();
   mPool = nullptr; 
  }
}
 

make sense?
Attached patch Phase 1: Add lock to STS (obsolete) — Splinter Review
Attachment #8441685 - Attachment is obsolete: true
Attachment #8441685 - Flags: review?(bent.mozilla)
Attachment #8442955 - Flags: review?(mcmanus)
Comment on attachment 8442955 [details] [diff] [review]
Phase 1: Add lock to STS

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

::: netwerk/base/src/nsStreamTransportService.cpp
@@ +500,5 @@
>  {
> +    nsCOMPtr<nsIThreadPool> pool;
> +    {
> +        mozilla::MutexAutoLock lock(mShutdownLock);
> +        if (mIsShutdown)

all new code does 

if (foo) {
 noteTheBraces();
}

even if it doesn't match the rest of the file

@@ +510,4 @@
>  }
>  
>  NS_IMETHODIMP
>  nsStreamTransportService::IsOnCurrentThread(bool *result)

I'm really not certain how this is defined.. but if it is subject to the same race as Dispatch - then it needs to do the same check of mIsShutdown under lock

@@ +551,5 @@
>                                    const char16_t *data)
>  {
>    NS_ASSERTION(strcmp(topic, "xpcom-shutdown-threads") == 0, "oops");
>  
> +  nsCOMPtr<nsIThreadPool> pool;

you don't need the local pool in this version

@@ +554,5 @@
>  
> +  nsCOMPtr<nsIThreadPool> pool;
> +  {
> +      mozilla::MutexAutoLock lock(mShutdownLock);
> +      mIsShutdown = nullptr;

mIsShutdown = true ... this is the r- driver.

::: netwerk/base/src/nsStreamTransportService.h
@@ +21,5 @@
>      NS_DECL_NSIOBSERVER
>  
>      nsresult Init();
>  
> +    nsStreamTransportService() : mShutdownLock("nsStreamTransportService.mLock"),

("nsStreamTransportService.mShutdownLock")
Attachment #8442955 - Flags: review?(mcmanus) → review-
Just like the race between Dispatch() and Shutdown(), the race might arise when we call IsOnCurrentThread and Shutdown concurrently. Do we need to check mShutdown at the beginning of an IsOnCurrentThread() call to ensure that does not happen?
Flags: needinfo?(benjamin)
Attachment #8442955 - Attachment is obsolete: true
Attachment #8443250 - Flags: review?(mcmanus)
Flags: needinfo?(benjamin)
Since IsOnCurrentThread has the same issue, I think another check is also needed there. Here is the patch.
Attachment #8439591 - Attachment is obsolete: true
Attachment #8443251 - Flags: review?(benjamin)
Attachment #8443250 - Flags: review?(mcmanus) → review+
Attachment #8443251 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/89417b53e239
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
What's the priority of this bent?
Flags: needinfo?(bent.mozilla)
ignore last comment :)
Flags: needinfo?(bent.mozilla)
You need to log in before you can comment on or make changes to this bug.