Fix a bug where the JavaBridgeDispatcherHost is prematurely deleted

Fix a bug where the JavaBridgeDispatcherHost is prematurely deleted

JavaBridgeDispatcherHost must be ref-counted because it is used with
base::Bind. http://codereview.chromium.org/8834013 changed
JavaBridgeDispatcherHost to also inherit from RenderViewHostObserver. The base
implementation of RenderViewHostObserver::RenderViewHostDestroyed() deletes the
object. This is unwanted in this class, so we provide an empy implementation of
the method.


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

[joth, 2011-12-13 11:50:43]

lgtm other than a couple nits.

http://codereview.chromium.org/8929005/diff/1/content/browser/renderer_host/j...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

http://codereview.chromium.org/8929005/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:51: Send(new
JavaBridgeMsg_Init(routing_id()));
note the RVH may have gone away (due to ref-count we can out live it) so this
Send may return false. I'm not sure what it really means in this case anyway.

http://codereview.chromium.org/8929005/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:65: bool
JavaBridgeDispatcherHost::Send(IPC::Message* msg) {
aside (not for this CL) - is this Send() override needed? AFAICS it's only
called from within this class.

http://codereview.chromium.org/8929005/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:71: // refs
held by the JavaBridgeDispatcherHostManager and base::Bind, so this
nit: this behavior -> that behavior.

http://codereview.chromium.org/8929005/diff/1/content/browser/renderer_host/j...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.h (right):

http://codereview.chromium.org/8929005/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_bridge_dispatcher_host.h:25: public
RenderViewHostObserver {
multiple behavior inheritance: this is not strictly allowed, although it is
somewhat common in chrome.
Besides being banned it's the root of the problems here as both base classes
define incompatible protocols for object lifetime.

fortunately overriding RenderViewHostDestroyed() with an empty impl is pretty
common so presumably a safe enough fix. (We could add an extra ref and remove it
in RenderViewHostDestroyed() but doesn't look like that's needed)

[Steve Block, 2011-12-13 12:40:20]

http://codereview.chromium.org/8929005/diff/1/content/browser/renderer_host/j...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

http://codereview.chromium.org/8929005/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:51: Send(new
JavaBridgeMsg_Init(routing_id()));
I don't think this is a problem, as the JavaBridgeDispatcherHostManager should
never call these methods on this object once the corresponding RenderViewHost
has been deleted.

http://codereview.chromium.org/8929005/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:65: bool
JavaBridgeDispatcherHost::Send(IPC::Message* msg) {
Yes, I think it is, as we need to make Send() public for use with the delayed
response message macros. RenderViewHostObserver::Send() is protected.

http://codereview.chromium.org/8929005/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:71: // refs
held by the JavaBridgeDispatcherHostManager and base::Bind, so this
On 2011/12/13 11:50:43, joth wrote:
> nit: this behavior -> that behavior.

Done.

[joth, 2011-12-13 12:45:03]

lgtm

[jam, 2011-12-13 14:50:37]

lgtm

[commit-bot: I haz the power, 2011-12-13 14:53:21]

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

[commit-bot: I haz the power, 2011-12-13 16:16:30]

Change committed as 114218