417623 - ###!!! ASSERTION: nsPluginHostImpl not thread-safe

Status: RESOLVED FIXED

(Plugins Graveyard :: Java (Java Embedding Plugin), defect)

Description

[Carsten Book [:Tomcat]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008021417 Firefox/3.0b4pre

Steps to reproduce:
Load http://www.whitehouse.gov/history/whtour/blueroom-ipix.html

-> Assertion

###!!! ASSERTION: nsPluginHostImpl not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /debug/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp, line 2687

Comment 1

[Carsten Book [:Tomcat]]

also reproducible with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008032701 Firefox/3.0pre ID:2008032701 when i play the game at http://www.arcadetown.com/needformadness/game.asp

Comment 2

[Carsten Book [:Tomcat]]

Attachment: stack

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I hit this assertion at http://ma7room.com/ along with bug 417624 and bug 425300 at the same site.

Comment 4

[Peter Van der Beken [:peterv]]

This looks like it's coming from MRJ/JEP, Java_JEPCookieHandler_setCookie is in the stack (hopefully that part of the stack is correct). nsPluginHostImpl doesn't look threadsafe, should MRJ/JEP ensure that it only calls functions on nsPluginHostImpl from the main thread?

Comment 10

[timeless]

we have some code in certain parts of plugins to force fail access from other threads. we can add more.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

timeless, the JEP has its own component, so unless this bug is about the OJI interface it talks to--which seems not to be the case, given comment 5--this bug should live there.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

I'll get to this as soon as I can.

But I lately I haven't had much time for the JEP, so that may be a
while.

On the face of it, the JEP seems to be calling
nsIServiceManager::GetService() (from MRJPlugin::GetService()) on
something else than the main thread.  But note that (if this is
confirmed) the JEP may have no choice but to do this.  And also note
that (as far as I can tell) there aren't any other problems with doing
this than the assertion itself.

Comment 13

[Peter Van der Beken [:peterv]]

(In reply to comment #12)
> But note that (if this is
> confirmed) the JEP may have no choice but to do this.  And also note
> that (as far as I can tell) there aren't any other problems with doing
> this than the assertion itself.

In some of the duplicated bugs we end up in the cycle collector, which can definitely lead to serious problems if it happens from a non-main thread.

Comment 14

[timeless]

we now provide:
https://developer.mozilla.org/en/NPN_PluginThreadAsyncCall

which lets you schedule time on the main thread.

there's also

http://mxr-test.konigsberg.mozilla.org/mozilla-central/ident?i=NS_WITH_PROXIED_SERVICE

although i'm not sure how well that works. if it's broken, we need to fix it. note that in general, if you need anything proxied, you need to get it in advance, not on demand.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #13 and comment #14)

OK.  Presuming I can confirm this bug, I'll see what I can do to
change the JEP's behavior.

By the way, do you think it'd be reasonable to cache the result of
calling nsIServiceManager::GetService() on the main thread, then call
that service on a secondary thread?

Comment 16

[timeless]

no. you have two choices:

1. getservice on the main thread and create a proxy.
2. create a proxy for yourself on the main thread.

either way, from another thread you have to call a method on a proxy object (or use NPN_PluginThreadAsyncCall) to perform your operation on the main thread.

very very very little of gecko is threadsafe. it isn't sufficient to just do the get on the main thread.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

nsIServiceManager::GetService() is thread-safe (see the various
implementations of nsComponentManagerImpl::GetService() in
nsComponentManager.cpp).

But most "services" (in fact all XPCOM objects) seem not to be
thread-safe.  Or at least they throw assertions (in debug builds) if
you try to access one from a different thread from the one on which
the service/object was created.

This works as follows:

Every XPCOM object that uses the NS_IMPL_ISUPPORTS and NS_IMPL_ADDREF
(and some other macros) includes another macro
(NS_ASSERT_OWNINGTHREAD) that gets called whenever AddRef() is called,
which throws an assertion whenever it's run on a different thread from
the one on which the object was created.  There are some "services"
that are clearly designed to be thread-safe (an example is
nsIJVMThreadManager, which can be used to post events on different
threads).  But (as timeless says) the default assumption seems to be
that every XPCOM object is non-thread-safe (unless for some reason it
needs to be thread-safe).

Comment 18

[Steven Michaud [:smichaud] (Retired)]

The problem here is that the JEP (actually the MRJ Plugin JEP)
accesses the nsICookieStorage service (one of the interfaces of an
nsPluginHostImpl object) on a secondary thread, while the
nsPluginHostImpl object was created on the main thread.

Turns out this problem has a very easy solution.  The JEP has an API
(JEPRunOnMainThread()) that I've long used to do Java-to-JavaScript
LiveConnect (where calls are made on secondary threads that need to be
run on the main thread).  It's very simple, and in my experience works
a lot better than nsIJVMManager::PostEvent() (what the old MRJPlugin
used).

I've changed MRJGetCookie() and MRJSetCookie() to use
JEPRunOnMainThread().  In brief testing they seem to work perfectly,
and to get rid of all the "not thread safe" assertions.

I need to do more testing.  And there are some other changes I want to
make to the new JEP before I release it (including (if I'm lucky)
changes needed to make it work on SnowLeopard, and adding mime-type
info to the MRJ Plugin JEP's Info.plist (see bug 469510)).  But if all
goes well, I should be able to release the new JEP in a month or so.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

I've just released a new version of the JEP (0.9.7) that contains the
fix I talk about in comment #18.  See bug 488746.