assumptions about Singleton destruction thread

[Marshall Greenblatt]

Hi All,

Objects created using Singleton and DefaultSingletonTraits are destroyed by AtExitManager. AtExitManager is owned by ContentMainRunner and is destroyed on the main application thread. This may be a problem for Singletons that make assumptions about the destruction thread. For example, the BrowserAccessibilityStateImpl class (content/browser/accessibility/browser_accessibility_state_impl.h) is a Singleton created on the UI thread that uses base::OneShotTimer to update a histogram. The Timer::AbandonScheduledTask method (called from the OneShotTimer destructor) DCHECKs that the Timer is destroyed on the same thread that created it. While the UI thread and the main application thread are currently the same for Chromium this may not always be a safe assumption. For example, the Chromium Embedded Framework supports a run mode where the UI thread is separate from the main application thread and is consequently hitting this DCHECK.

What kinds of assumptions should Singleton objects make about the threads used for creation and destruction? Is there currently a way to indicate that a Singleton should be destroyed on a particular thread? If not, should we add this capability?

Thanks,
Marshall

[Peter Kasting]

On Tue, Apr 10, 2012 at 1:01 PM, Marshall Greenblatt <magree...@gmail.com> wrote:

the Chromium Embedded Framework supports a run mode where the UI thread is separate from the main application thread and is consequently hitting this DCHECK.

Honestly I'm stunned if hitting this DCHECK is the only bad thing happening to you.  I'd have expected this sort of a setup to break about 10,000 assumptions in the code.

PK

[Marshall Greenblatt]

Most code is tied to either the creation thread or BrowserThread::UI specifically. The multi-threaded UI mode supported by CEF is convenient when using frameworks like MFC that hide the application message loop implementation. It works reasonably well on Windows but is not possible on all platforms (specifically, Mac).

PK

[John Bates]

On Tue, Apr 10, 2012 at 1:01 PM, Marshall Greenblatt <magree...@gmail.com> wrote:

Hi All,

Objects created using Singleton and DefaultSingletonTraits are destroyed by AtExitManager. AtExitManager is owned by ContentMainRunner and is destroyed on the main application thread. This may be a problem for Singletons that make assumptions about the destruction thread. For example, the BrowserAccessibilityStateImpl class (content/browser/accessibility/browser_accessibility_state_impl.h) is a Singleton created on the UI thread that uses base::OneShotTimer to update a histogram. The Timer::AbandonScheduledTask method (called from the OneShotTimer destructor) DCHECKs that the Timer is destroyed on the same thread that created it.

Minor correction: ... DCHECKs that the Timer is destroyed on the same thread that *started* it. That's to catch potential use-after-free races.

 

While the UI thread and the main application thread are currently the same for Chromium this may not always be a safe assumption. For example, the Chromium Embedded Framework supports a run mode where the UI thread is separate from the main application thread and is consequently hitting this DCHECK.

What kinds of assumptions should Singleton objects make about the threads used for creation and destruction?

As long as other threads are joined by the time Singletons are deleted on the main thread, there should be no fundamental problem with Singletons.

 

Is there currently a way to indicate that a Singleton should be destroyed on a particular thread? If not, should we add this capability?

In most cases regarding the Timer DCHECK, there is a shutdown code path that occurs on the Timer's thread which can delete the Timer (by making it a scoped_ptr, for example and resetting it on the proper thread before the singleton is destroyed on the main thread). Is there a natural shutdown point in the UI thread that could tell BrowserAccessibilityStateImpl to delete it's timer?

 

Thanks,
Marshall

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Marshall Greenblatt]

Evan Martin suggested using LeakySingletonTraits instead of DefaultSingletonTraits, which seems an easy fix. Any reason why it wouldn't be OK to leak the Timer?
 

[John Bates]

No, LeakySingletonTraits SGTM.

[William Chan (陈智昌)]

The only reasons why it wouldn't be OK in general are:

* You're in some code which is shared by users that don't want leaks (ie: something that is dlopen()'d and then dlclose()'d expects that the shared object is not leaking memory in its address space). I suspect this isn't the case here, unless CEF is designed to allow that.

* There is important behavior in the destructor (I doubt this is the case here) that needs to run on shutdown.

In general, I'm a fan of leaking on shutdown (and Singleton and LazyInstance should have valgrind suppressions already so they don't whine about this) since it's easier and doesn't slow down shutdown.