631452 - LazyIdleThread can race with newly dispatched events when shutting down

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Josh Matthews [:jdm]]

I looked into this because of the error messages in bug 631179.  I believe that if the idle timer expires and the thread starts to shutdown, any events dispatched to the lazy thread will be dropped while the underlying thread is waiting for the shutdown ack event to run.

Specifically, I believe the problem lies in LazyIdleThread::ShutdownThread:

>294     rv = mThread->Shutdown();
>295     NS_ENSURE_SUCCESS(rv, rv);
>296 
>297     mThread = nsnull;

EnsureThread relies on mThread being null to create a new thread object, but it isn't cleared until mThread->Shutdown finishes.  Meanwhile in nsThread::Shutdown, we see this:

>490   // Process events on the current thread until we receive a shutdown ACK.
>491   while (!context.shutdownAck)
>492     NS_ProcessNextEvent(context.joiningThread);

where the current thread is the main thread.  In this situation, we can end up processing events like OriginClearRunnable, which try to dispatch themselves to the lazy IO thread that is currently shutting down.

A possible fix might be to do the old |nsCOMPtr<nsIThread> thread; thread.swap(mThread)| trick and clear out mThread before trying to shut down, but I haven't reasoned through the ramifications of that yet.

Comment 1

[Shawn Wilsher :sdwilsh]

Are we seeing an issues in the wild because of this?  (not saying that that is needed to fix this, just trying to understand how serious this could be)

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We should fix it, it's biting tinderbox, it can happen in the wild.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 509824 [details] [diff] [review]
Patch, v1

> NS_IMETHODIMP
> LazyIdleThread::Dispatch(nsIRunnable* aEvent,
>                          PRUint32 aFlags)
> {
>   ASSERT_OWNING_THREAD();
> 
>+  // LazyIdleThread can't always support synchronous dispatch currently.
>+  NS_ENSURE_TRUE(aFlags == NS_DISPATCH_NORMAL, NS_ERROR_NOT_IMPLEMENTED);
>+
>+  // If our thread is shutting down then we can't actually dispatch right now.
>+  // Queue this runnable for later.
>+  if (mQueuedRunnables) {
I would prefer to see a simple method added that looks like this: bool ThreadShuttingDown() { return !!mQueuedRunnables; }.  I don't like depending on state like this if it can be avoided.


Also, it feels like we could write a test for this, right?

r=sdwilsh with comments addressed.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #4)
> I would prefer to see a simple method added that looks like this: bool
> ThreadShuttingDown() { return !!mQueuedRunnables; }.

Hm. That name would probably be very confusing considering that mThreadIsShuttingDown is unrelated to this queue. I can come up with a better name I guess.

> Also, it feels like we could write a test for this, right?

How would you do that?

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> Hm. That name would probably be very confusing considering that
> mThreadIsShuttingDown is unrelated to this queue. I can come up with a better
> name I guess.
Er, good point.  Better name is needed there.

> How would you do that?
Actually, it's not possible.  Don't worry about a test.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/mozilla-central/rev/9414d1eaeeeb