Fix a race condition in the Java Bridge when adding objects

Fix a race condition in the Java Bridge when adding objects

JavaBridgeDispatcherHost::AddNamedObject() currently has a race condition, as
it posts to the WEBKIT thread before sending the injected object to the
renderer. See bug for details.

To fix this, JavaBridgeDispatcherHost::AddNamedObject() now creates the
NPVariant_Param synchronously, and immediately sends this to the renderer. This
requires use of a route ID generator which is shared with the
JavaBridgeChannelHost. Creation of the JavaBridgeChannelHost and the
corresponding NPObjectStub is done asynchronously on the WEBKIT thread.

This means that the channel handle is not available when the Java Bridge is
first initialized in the renderer. To overcome this, the renderer obtains it
from the browser with a new sync IPC call.

- RenderViewImpl - OnJavaBridgeInit() no longer supplies a channel handle.
- JavaBridgeDispatcher - Lazily gets channel handle from browser.
- JavaBridgeDispatcherHost - Now a RVH obsever to provide channel handle.
  Uses shared route ID generator to synchronously create the NPVariant_Param
  when a new object is injected. Creates the JavaBridgeChannelHost and the
  corresponding NPObjectStub asynchronously on the WEBKIT thread.
- JavaBridgeChannelHost - Shares a route ID generator with the
  JavaBridgeDispatcherHost.

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

[joth, 2011-12-07 14:49:07]

couple quick comments. I don't think I quite understood the comment in the CL
description for injecting the ID generator.

http://codereview.chromium.org/8834013/diff/1001/content/browser/renderer_hos...
File content/browser/renderer_host/java/java_bridge_channel_host.cc (right):

http://codereview.chromium.org/8834013/diff/1001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_channel_host.cc:28: static
base::subtle::AtomicWord last_id = 0;
strictly, this is not thread-safe, as we disable gcc's thread-safe local static
support.
putting this at file scope would fix it.

http://codereview.chromium.org/8834013/diff/1001/content/browser/renderer_hos...
File content/browser/renderer_host/java/java_bridge_channel_host.h (right):

http://codereview.chromium.org/8834013/diff/1001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_channel_host.h:11: // Threadsafe
route ID generator.
could you expand this a little:
- allows injection of the logic that will be used to create the route ID
- GenerateRouteID() must be [re-entrant | safe to call on any thread]

http://codereview.chromium.org/8834013/diff/1001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_channel_host.h:15: private:
\n before

http://codereview.chromium.org/8834013/diff/1001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_channel_host.h:17:
~RouteIDGenerator() {}
d'tor body always in .cc

http://codereview.chromium.org/8834013/diff/1001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_channel_host.h:18: };
as this does not have any state (other than in a static) would it be OK to pass
this as a plain-old function pointer, rather than create thread safe ref-counted
object for this?
Or even, just a plain function which both consumers can call directly (no need
for a function pointer?)

I'm not quite sure what the state is that this object instance captures.

http://codereview.chromium.org/8834013/diff/1001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_channel_host.h:23: int
renderer_id, base::MessageLoopProxy* ipc_message_loop,
one param per line in declarations.

[Steve Block, 2011-12-07 16:47:44]

http://codereview.chromium.org/8834013/diff/1001/content/browser/renderer_hos...
File content/browser/renderer_host/java/java_bridge_channel_host.cc (right):

http://codereview.chromium.org/8834013/diff/1001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_channel_host.cc:28: static
base::subtle::AtomicWord last_id = 0;
On 2011/12/07 14:49:09, joth wrote:
> strictly, this is not thread-safe, as we disable gcc's thread-safe local
static
> support.
> putting this at file scope would fix it.

Done.

http://codereview.chromium.org/8834013/diff/1001/content/browser/renderer_hos...
File content/browser/renderer_host/java/java_bridge_channel_host.h (right):

http://codereview.chromium.org/8834013/diff/1001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_channel_host.h:11: // Threadsafe
route ID generator.
On 2011/12/07 14:49:09, joth wrote:
> could you expand this a little:
> - allows injection of the logic that will be used to create the route ID
> - GenerateRouteID() must be [re-entrant | safe to call on any thread]

Done.

http://codereview.chromium.org/8834013/diff/1001/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_channel_host.h:18: };
The only state this captures is ensuring that all calls (for a given
JavaBridgeDispatcherHost) get a unique ID. I was considering using a different
series of IDs per JavaBridgeDispatcherHost, but you're right that if we use a
global series, this can be a static function.

[jam, 2011-12-07 22:07:03]

don't have the java code in my head atm, I defer to joth for that. just nits,
lgtm with them

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

http://codereview.chromium.org/8834013/diff/9003/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:23: ,
is_renderer_initialized_(false) {
nit: , on previous line per Google C++ style guide

http://codereview.chromium.org/8834013/diff/9003/content/common/java_bridge_m...
File content/common/java_bridge_messages.h (right):

http://codereview.chromium.org/8834013/diff/9003/content/common/java_bridge_m...
content/common/java_bridge_messages.h:21:
IPC_SYNC_MESSAGE_ROUTED0_1(JavaBridgeMsg_GetChannelHandle,
nit: renderer-> browser messages should have Host in them, i.e.
JavaBridgeHostMsg_..., and it should be listed at the bottom of the file (see
other files for example)

http://codereview.chromium.org/8834013/diff/9003/content/renderer/java/java_b...
File content/renderer/java/java_bridge_dispatcher.cc (right):

http://codereview.chromium.org/8834013/diff/9003/content/renderer/java/java_b...
content/renderer/java/java_bridge_dispatcher.cc:29: render_view()->Send(new
JavaBridgeMsg_GetChannelHandle(
nit: "render_view()->" isn't needed, and render_view()->GetRoutingId() can just
be "routing_id(). Send() and routing_id() are methods on RenderViewObserver.

[Steve Block, 2011-12-08 11:17:56]

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

http://codereview.chromium.org/8834013/diff/9003/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:23: ,
is_renderer_initialized_(false) {
On 2011/12/07 22:07:03, John Abd-El-Malek wrote:
> nit: , on previous line per Google C++ style guide

Done.

http://codereview.chromium.org/8834013/diff/9003/content/common/java_bridge_m...
File content/common/java_bridge_messages.h (right):

http://codereview.chromium.org/8834013/diff/9003/content/common/java_bridge_m...
content/common/java_bridge_messages.h:21:
IPC_SYNC_MESSAGE_ROUTED0_1(JavaBridgeMsg_GetChannelHandle,
On 2011/12/07 22:07:03, John Abd-El-Malek wrote:
> nit: renderer-> browser messages should have Host in them, i.e.
> JavaBridgeHostMsg_..., and it should be listed at the bottom of the file (see
> other files for example)

Done.

http://codereview.chromium.org/8834013/diff/9003/content/renderer/java/java_b...
File content/renderer/java/java_bridge_dispatcher.cc (right):

http://codereview.chromium.org/8834013/diff/9003/content/renderer/java/java_b...
content/renderer/java/java_bridge_dispatcher.cc:29: render_view()->Send(new
JavaBridgeMsg_GetChannelHandle(
On 2011/12/07 22:07:03, John Abd-El-Malek wrote:
> nit: "render_view()->" isn't needed, and render_view()->GetRoutingId() can
just
> be "routing_id(). Send() and routing_id() are methods on RenderViewObserver.

Done.

[Steve Block, 2011-12-08 13:46:03]

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:67: if
(RenderProcessHost::run_renderer_in_process()) {
Given the ongoing discussion about renderer-in-process mode in
http://code.google.com/p/chromium/issues/detail?id=106839, I could remove this
and replace with a TODO referencing
http://code.google.com/p/chromium/issues/detail?id=106838. This would allow this
patch to be landed without waiting for resolution on renderer-in-process mode.

[joth, 2011-12-08 18:15:45]

LGTM

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
File content/browser/renderer_host/java/java_bridge_channel_host.cc (right):

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
content/browser/renderer_host/java/java_bridge_channel_host.cc:46: return
base::subtle::Barrier_AtomicIncrement(&g_last_id, 1);
I'll stick my head out and say NoBarrier is fine here. (It generally is for
sequence numbers; see AtomicSequenceNumber)

you _could_ use AtomicSequenceNumber. But then you'd need a LazyInstance. Which
would then re-insert the (half) memory barrier on each access. :-)

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:67: if
(RenderProcessHost::run_renderer_in_process()) {
On 2011/12/08 13:46:04, Steve Block wrote:
> Given the ongoing discussion about renderer-in-process mode in
> http://code.google.com/p/chromium/issues/detail?id=106839, I could remove this
> and replace with a TODO referencing
> http://code.google.com/p/chromium/issues/detail?id=106838. This would allow
this
> patch to be landed without waiting for resolution on renderer-in-process mode.

I don't mind either way, given
http://codereview.chromium.org/8890018/diff/1/content/browser/renderer_host/j...
will land next anyway

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.h (right):

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
content/browser/renderer_host/java/java_bridge_dispatcher_host.h:19: typedef
_NPVariant NPVariant;
these two not needed? (_NPVariant & NPVariant)

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
content/browser/renderer_host/java/java_bridge_dispatcher_host.h:27: public
base::RefCountedThreadSafe<JavaBridgeDispatcherHost> {
nit: I *think* we tend to put ABCs before interfaces. (not that it's always that
clear)

[Steve Block, 2011-12-09 12:27:31]

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
File content/browser/renderer_host/java/java_bridge_channel_host.cc (right):

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
content/browser/renderer_host/java/java_bridge_channel_host.cc:46: return
base::subtle::Barrier_AtomicIncrement(&g_last_id, 1);
On 2011/12/08 18:15:46, joth wrote:
> I'll stick my head out and say NoBarrier is fine here. (It generally is for
> sequence numbers; see AtomicSequenceNumber)
> 
> you _could_ use AtomicSequenceNumber. But then you'd need a LazyInstance.
Which
> would then re-insert the (half) memory barrier on each access. :-)

Done.

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:67: if
(RenderProcessHost::run_renderer_in_process()) {
Added a TODO.

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.h (right):

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
content/browser/renderer_host/java/java_bridge_dispatcher_host.h:19: typedef
_NPVariant NPVariant;
On 2011/12/08 18:15:46, joth wrote:
> these two not needed? (_NPVariant & NPVariant)

Done.

http://codereview.chromium.org/8834013/diff/16001/content/browser/renderer_ho...
content/browser/renderer_host/java/java_bridge_dispatcher_host.h:27: public
base::RefCountedThreadSafe<JavaBridgeDispatcherHost> {
On 2011/12/08 18:15:46, joth wrote:
> nit: I *think* we tend to put ABCs before interfaces. (not that it's always
that
> clear)

Done.