Update Java Bridge to use its own thread in the browser process

Update Java Bridge to use its own thread in the browser process

Currently the Java Bridge uses the WEBKIT thread to manipulate injected objects
in the browser process. However, the WEBKIT thread is not set up when the
renderer is run in-process, and it may be deprecated.

This change adds a new background thread in JavaBridgeDispatcherHost, and updates the JavaBridge to use it in place of the WEBKIT thread.

BUG=106838
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114011

[Steve Block, 2011-12-08 16:53:26]

Note that this is based on top of http://codereview.chromium.org/8834013

[joth, 2011-12-08 18:25:54]

couple quick questions.

http://codereview.chromium.org/8890018/diff/1/content/browser/browser_main_lo...
File content/browser/browser_main_loop.h (right):

http://codereview.chromium.org/8890018/diff/1/content/browser/browser_main_lo...
content/browser/browser_main_loop.h:89: scoped_ptr<BrowserProcessSubThread>
java_bridge_thread_;
I was wondering whether having a base::Thread owned by
JavaBridgeDispatcherHostManager would be cleaner? This would avoid spilling
knowledge of this fairly constrained use case all over the code, and mean it
could be lazy-created when the JavaBridgeDispatcherHost is created (passing the
MessageLoopProxy in as an extra param) but would mean each tab would get its own
thread, unless there's a sane way to share them. (another LazyInstance....)

Thoughts?

http://codereview.chromium.org/8890018/diff/1/content/public/browser/browser_...
File content/public/browser/browser_thread.h (right):

http://codereview.chromium.org/8890018/diff/1/content/public/browser/browser_...
content/public/browser/browser_thread.h:84: JAVA_BRIDGE,
could this go in #if defined, like WEB_SOCKET_PROXY below?

[Steve Block, 2011-12-09 12:39:10]

http://codereview.chromium.org/8890018/diff/1/content/browser/browser_main_lo...
File content/browser/browser_main_loop.h (right):

http://codereview.chromium.org/8890018/diff/1/content/browser/browser_main_lo...
content/browser/browser_main_loop.h:89: scoped_ptr<BrowserProcessSubThread>
java_bridge_thread_;
Yeah, I thought about this, but if we want it to be a BrowserThread, it needs to
be created and destroyed with the other BrowserThreads. I'll have a look at
making it a plain Thread, internal to JavaBridgeDispatcherHost.

http://codereview.chromium.org/8890018/diff/1/content/public/browser/browser_...
File content/public/browser/browser_thread.h (right):

http://codereview.chromium.org/8890018/diff/1/content/public/browser/browser_...
content/public/browser/browser_thread.h:84: JAVA_BRIDGE,
It could, but I thought the policy was to minimize use of ifdefs?

[joth, 2011-12-09 16:26:08]

This lgtm with some nits for the purpose of moving forward, but one comment
about thread ownership we may want to think about some more for a follow up...

http://codereview.chromium.org/8890018/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

http://codereview.chromium.org/8890018/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:25:
base::Thread* thread = new (instance) base::Thread("Java Bridge");
nit: "JavaBridge"
some tools may not be so smart with whitespace; anyway we generally seem to
avoid it in thread names

http://codereview.chromium.org/8890018/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:29: };
my own preference would be to make your own Thread subclass that calls Start
from the c'tor.
This way it's easy to add Init() & CleanUp() overrides if needed in future.

http://codereview.chromium.org/8890018/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:31:
LAZY_INSTANCE_INITIALIZER;
I wasn't clear in my previous comment, but lazy instance might not be ideal for
this as it is only destroyed when the AtExitManager runs, which is in general a
bit hairy as the thread may still be running using other things that are
destroyed before it in the AtExitManager queue (e.g. MessageLoop::current()
depends on AtExitManager not having run yet but may get cleaned up before this
thread is stopped.)

afaict this "shouldn't" be the case here as by the time we exit we should have
disconnected the renderers and so have no pending sync calls into the bridge,
right?

(conceptually, what happens if the java-side is in an inf. loop when the
renderer is shutdown? somewhere along the line either some one will block on a
thread join that never returns or we need to send an interrupt into the VM for
that thread. either way that's certainly not something to fully handle in this
CL.. just trying to get a model on the relationship between the parts)



another thing occurs to me I have no idea the answer to: this will mean multiple
pages accessing the bridge will end up serialized on this one thread - is that
OK? I guess the alternative - one thread per dispatcher host - requires the
java-side to be fully thread safe. That may well not be the case, in general, I
guess.

http://codereview.chromium.org/8890018/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:129: new
NPObjectStub(object, channel_, route_id, 0, GURL());
comment why the return value from new is not stored or deleted?

[Steve Block, 2011-12-09 17:36:26]

http://codereview.chromium.org/8890018/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

http://codereview.chromium.org/8890018/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:25:
base::Thread* thread = new (instance) base::Thread("Java Bridge");
On 2011/12/09 16:26:09, joth wrote:
> nit: "JavaBridge"
> some tools may not be so smart with whitespace; anyway we generally seem to
> avoid it in thread names

Done.

http://codereview.chromium.org/8890018/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:29: };
On 2011/12/09 16:26:09, joth wrote:
> my own preference would be to make your own Thread subclass that calls Start
> from the c'tor.
> This way it's easy to add Init() & CleanUp() overrides if needed in future.

Done.

http://codereview.chromium.org/8890018/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:31:
LAZY_INSTANCE_INITIALIZER;
re AtExitManager - yes, the JavaBridge should be gracefully shutdown when the
renderer is disconnected, so this won't be a problem.

re serialization onto a single thread - note that this isn't changed by this CL
and must remain the case for backwards compatibility.

http://codereview.chromium.org/8890018/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:129: new
NPObjectStub(object, channel_, route_id, 0, GURL());
On 2011/12/09 16:26:09, joth wrote:
> comment why the return value from new is not stored or deleted?

Done.

[jam, 2011-12-10 00:08:42]

lgtm

[commit-bot: I haz the power, 2011-12-12 11:18:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/steveblock@chromium.org/8890018/10002

[commit-bot: I haz the power, 2011-12-12 12:34:58]

Change committed as 114011