Closed Bug 631452 Opened 9 years ago Closed 9 years ago

LazyIdleThread can race with newly dispatched events when shutting down

Categories

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

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: jdm, Assigned: bent.mozilla)

References

Details

(Whiteboard: [softblocker])

Attachments

(1 file)

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.
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)
We should fix it, it's biting tinderbox, it can happen in the wild.
Assignee: nobody → bent.mozilla
blocking2.0: --- → final+
Whiteboard: [softblocker]
Attached patch Patch, v1Splinter Review
Attachment #509824 - Flags: review?(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.
Attachment #509824 - Flags: review?(sdwilsh) → review+
(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?
blocking2.0: final+ → ---
(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.
blocking2.0: --- → final+
http://hg.mozilla.org/mozilla-central/rev/9414d1eaeeeb
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.