getting resources from ResourceBundle thread-safe?

[Dmitry Polukhin]

Hi Team,

Is getting resources from ResourceBundle thread-safe or not? It looks like POSIX implementation is thread-safe if ResourceBundle is initialized. POSIX implementation of ResourceBundle::GetRawDataResource is read-only operation. Windows implementation also looks thread-safe because FindResource/LoadResource/LockResource are thread-safe. But there is comment near ReloadSharedInstance that "all string access is expected to happen on the UI thread". So my question is why it is expected? If it is really expected, we should enforce it by inserting DCHECK that current thread is UI thread. But I see no reason for doing this. Having thread-safe load resources is significant advantage and it looks like we already have such implementations. Therefore we should remove comment and make it clear in class comment that ResourceBundle is thread safe after you have initialized it on UI thread. Resources reload happens only on Chrome OS during OOBE/login when there is no browser windows so it is exception case when it is safe.

     Dmitry

[Peter Kasting]

On Wed, Apr 6, 2011 at 11:48 PM, Dmitry Polukhin <dpol...@chromium.org> wrote:

But there is comment near ReloadSharedInstance that "all string access is expected to happen on the UI thread". So my question is why it is expected? If it is really expected, we should enforce it by inserting DCHECK that current thread is UI thread. But I see no reason for doing this.

I suspect no one had a case where resources (which, ultimately, are going to be used on the UI thread) should be loaded on a background thread, and thus, as is common practice in Chrome code, the code was written under the assumption that it could be non-threadsafe.  As you note, initializing the ResourceBundle itself is non-threadsafe.

Having thread-safe load resources is significant advantage

Why is it an advantage?

Resources reload happens only on Chrome OS during OOBE/login when there is no browser windows so it is exception case when it is safe.

If you're showing UI, aren't you still running a UI thread, whether or not there are browser windows around?

PK

[Dmitry Polukhin]

Strings and UI elements can be built/processed not only on UI thread. But UI thread is used to show them. Also resources can be used to keep some file that may be needed on other threads. Locally I inserted assert about named threads only and found use case in bookmarks (it looks like it is general issue for all component extensions):

[31861:31871:863259965706:FATAL:resource_bundle_posix.cc(58)] Check failed: id == BrowserThread::UI (6 vs. 0)

Backtrace:

base::debug::StackTrace::StackTrace() [0x8d90f54]

logging::LogMessage::~LogMessage() [0x8da6fa1]

ui::ResourceBundle::GetRawDataResource() [0x8e584a3]

(anonymous namespace)::URLRequestResourceBundleJob::GetData() [0x825b57c]

net::URLRequestSimpleJob::StartAsync() [0x91c448d]

DispatchToMethod<>() [0x91c48a1]

ScopedRunnableMethodFactory<>::RunnableMethod<>::Run() [0x91c4809]

MessageLoop::RunTask() [0x8dab9d7]

MessageLoop::DeferOrRunPendingTask() [0x8daba9b]

MessageLoop::DoWork() [0x8dac32b]

base::MessagePumpLibevent::Run() [0x8d80219]

MessageLoop::RunInternal() [0x8dab823]

MessageLoop::RunHandler() [0x8dab6fd]

MessageLoop::Run() [0x8dab151]

base::Thread::Run() [0x8dec14f]

base::Thread::ThreadMain() [0x8dec31f]

base::(anonymous namespace)::ThreadFunc() [0x8deb786]

start_thread [0xf6c7396e]

As for reload in Chrome OS, it happens on UI thread but we control all windows visible that time and they process locale change request accordingly. Switching language when there are browser windows is much harder.

In general if we make some assumption, we should add check otherwise it will be broken at some point.

[Peter Kasting]

On Fri, Apr 8, 2011 at 2:07 AM, Dmitry Polukhin <dpol...@chromium.org> wrote:

In general if we make some assumption, we should add check otherwise it will be broken at some point.

I agree.  Someone familiar with the bookmark code on POSIX should probably look at the checkfailure you were able to cause, and we should probably then go ahead and insert such a check.

PK 

[Evan Martin]

I believe there are some resources that are used in networking, from
the IO thread. For example, error messages related to networking
failures, or images used in some error page.

I think documenting the expected behavior of this code (whatever it is
-- UI thread only or thread-safe) in the form of assertions like the
one you added is always a good idea.

On Fri, Apr 8, 2011 at 2:07 AM, Dmitry Polukhin <dpol...@chromium.org> wrote:

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