Pass RenderFrameHost to WebContentObservers' message handlers

Pass RenderFrameHost to WebContentObservers' message handlers

This patch enables WebContentsObserver::OnMessageReceived to receive
a reference to RenderFrameHost that received the message.

This allows passing of JavaBridgeDispatcherHostManager ownership from
WebContentsImpl to ContentViewCore and removing of some redundant
initialization code in JavaBridgeDispatcherHostManager, fixing
a long-standing TODO.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268939

[mnaganov (inactive), 2014-04-28 19:44:34]

Hi Jochen,

Can you please take a look, whether this change to WebContentsObserver makes
sense to you.

Although, in this patch, I'm changing the old Java Bridge implementation, in
fact I'm facing the same problem in the new implementation. It is even more
acute, as I now have 4 messages originating from renderer to browser instead of
just 1 in the old implementation.

[Charlie Reis, 2014-04-28 19:48:30]

Please have jam@ review this.  We've had an extensive thread on this topic on
site-isolation-dev@chromium.org.

[mnaganov (inactive), 2014-04-28 19:55:12]

Thanks, Charlie! I'll have a look at the list.

John, does this change makes sense, or do you have a better solution in mind?

[Charlie Reis, 2014-04-28 21:37:39]

On 2014/04/28 19:55:12, Mikhail Naganov (Cr) wrote:
> Thanks, Charlie! I'll have a look at the list.

Sure.  For reference, here's the (lengthy) thread:
https://groups.google.com/a/chromium.org/forum/#!topic/site-isolation-dev/7u1...

The last two messages from Nick and John are where we're starting to reach a
conclusion.  The IPC_MESSAGE_HANDLER and IPC_MESSAGE_FORWARD macros are the
remaining thing to resolve, if I understand correctly.

> 
> John, does this change makes sense, or do you have a better solution in mind?

[ncarter (slow), 2014-04-28 22:04:50]

On 2014/04/28 21:37:39, Charlie Reis wrote:
> On 2014/04/28 19:55:12, Mikhail Naganov (Cr) wrote:
> > Thanks, Charlie! I'll have a look at the list.
> 
> Sure.  For reference, here's the (lengthy) thread:
>
https://groups.google.com/a/chromium.org/forum/#!topic/site-isolation-dev/7u1...
> 
> The last two messages from Nick and John are where we're starting to reach a
> conclusion.  The IPC_MESSAGE_HANDLER and IPC_MESSAGE_FORWARD macros are the
> remaining thing to resolve, if I understand correctly.
> 
> > 
> > John, does this change makes sense, or do you have a better solution in
mind?

This change LGTM and implements the approach agreed to in that email thread. So
I'm excited to see it. I think what Mikhail did here, shoving the RFH in a
temporary member, is the right thing to do until we get a better pattern, maybe
a macro that supports the RFH parameter.

FYI, be aware of http://crbug.com/364974 which means that the observer currently
won't be tracking every RFH. I'm trying to land a fix for that (
https://codereview.chromium.org/246293009/ ) but it's actually running up
against a DCHECK failure in java_bridge_dispatcher_host_manager.cc's
WebContentsObserver methods and I'm not sure why.

[jam, 2014-04-29 00:33:50]

I would prefer that this lands in one change which does the following:
-does not forward IPCs from RenderFrameHostImpl to
WebContentsObserver::OnMessageReceived(message)
  -this involves auditing the places that call RenderFrame::Send or
RenderFrameObserver::Send; there aren't many yet so it shouldn't be too
burdensome
-use macros which give a message handler the frame
  -note today you can add "const IPC::Message& msg" as the first parameter to a
message handler, and it automagically passes the IPC to the method. I think this
could be even simpler as you can pass the parameter into the macro, something
like:

bool Foo::OnMessageReceived(const IPC::Message& message, RenderFrameHost* rfh) {
...
    IPC_MESSAGE_HANDLER(FooHostMsg_Bar, rfh, OnBar)
...
}

void Foo::OnBar(RenderFrameHost* rfh, int param1) {
...
}

[mnaganov (inactive), 2014-04-29 15:11:15]

On 2014/04/29 00:33:50, jam wrote:
> I would prefer that this lands in one change which does the following:
> -does not forward IPCs from RenderFrameHostImpl to
> WebContentsObserver::OnMessageReceived(message)
>   -this involves auditing the places that call RenderFrame::Send or
> RenderFrameObserver::Send; there aren't many yet so it shouldn't be too
> burdensome
> -use macros which give a message handler the frame
>   -note today you can add "const IPC::Message& msg" as the first parameter to
a
> message handler, and it automagically passes the IPC to the method. I think
this
> could be even simpler as you can pass the parameter into the macro, something
> like:
> 
> bool Foo::OnMessageReceived(const IPC::Message& message, RenderFrameHost* rfh)
{
> ...
>     IPC_MESSAGE_HANDLER(FooHostMsg_Bar, rfh, OnBar)
> ...
> }
> 
> void Foo::OnBar(RenderFrameHost* rfh, int param1) {
> ...
> }

Thanks everyone for replies! I will try this approach.

Nick, JavaBridgeDispatcherHostManager does its own tracking of RenderFrameHost
instances via RenderFrameCreated / RenderFrameDeleted (I think, you were talking
about such possibility in your discussion of the issue). So it looks like, the
manager doesn't receive all RenderFrameDeleted notifications prior to
destruction. JavaBridgeDispatcherHostManager is currently owned by
WebContentsImpl.

JavaBridgeDispatcherHostManager is Android-only, so my advice is for debugging
this is to build / debug on a local Android device instead of messing with try
bots. This is how you can build and run these tests:

GYP_DEFINES="OS=android use_goma=1 component=shared_library" gclient sync
ninja -j150 -l30 -C out/Debug content_unittests_apk
build/android/test_runner.py gtest -s content_unittests --gtest-filter
"RenderFrameHostManagerTest*" --verbose

[ncarter (slow), 2014-04-29 20:50:44]

https://codereview.chromium.org/253013002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/java/java_bridge_dispatcher_host_manager.cc
(right):

https://codereview.chromium.org/253013002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_dispatcher_host_manager.cc:124:
render_frame_host_message_source_ = render_frame_host;
Here's an idea that might mean we don't need to reinvent the macros, at least
for this cl. What if this code used the FORWARD version of the macros? Something
like:

  JavaBridgeDispatcherHost* instance = instances_[render_frame_host];

  // TODO: handle null |instance|
  IPC_BEGIN_MESSAGE_MAP(JavaBridgeDispatcherHostManager, message)
    IPC_MESSAGE_HANDLER_FORWARD_DELAY_REPLY(
        JavaBridgeHostMsg_GetChannelHandle,
        instance,
        OnGetChannelHandle);
    IPC_MESSAGE_UNHANDLED(handled = false)
  IPC_END_MESSAGE_MAP()

[mnaganov (inactive), 2014-05-01 13:57:38]

This is what I have done:

Now we have

virtual bool WebContentsObserver::OnMessageReceived(const IPC::Message& message,
RenderFrameHost* render_frame_host);

which is only called for messages received via RenderFrameHostDelegate, and good
old IPC::Listener's

virtual bool WebContentsObserver::OnMessageReceived(const IPC::Message& message)
OVERRIDE;

which is called for all other messages.

I have looked through all usages of RenderFrame::Send and
RenderFrameObserver::Send and have found that in most of the cases, messages are
handled by WebContentsImpl itself, in RenderFrameHost or in
RenderFrameMessageFilter descendants. Only two WebContentObservers seems to
receive RFH-specific messages currently: Java Bridge and shady
UIThreadExtensionFunction::RenderHostTracker, which are both fixed in this
patch.


There is one caveat with adding OnMessageReceived(message, rfh) to
WebContentsObserver -- InterstitialPageImpl class subclasses both
WebContentsObserver and RenderFrameHostDelegate, so it has got two variants of
OnMessageReceived: (message, rfh) and (rfh, message), which doesn't look great.

So WDYT about the change?

https://codereview.chromium.org/253013002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/java/java_bridge_dispatcher_host_manager.cc
(right):

https://codereview.chromium.org/253013002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bridge_dispatcher_host_manager.cc:124:
render_frame_host_message_source_ = render_frame_host;
On 2014/04/29 20:50:45, ncarter wrote:
> Here's an idea that might mean we don't need to reinvent the macros, at least
> for this cl. What if this code used the FORWARD version of the macros?
Something
> like:
> 
>   JavaBridgeDispatcherHost* instance = instances_[render_frame_host];
> 
>   // TODO: handle null |instance|
>   IPC_BEGIN_MESSAGE_MAP(JavaBridgeDispatcherHostManager, message)
>     IPC_MESSAGE_HANDLER_FORWARD_DELAY_REPLY(
>         JavaBridgeHostMsg_GetChannelHandle,
>         instance,
>         OnGetChannelHandle);
>     IPC_MESSAGE_UNHANDLED(handled = false)
>   IPC_END_MESSAGE_MAP()

Thanks for the suggestion, Nick!

[jam, 2014-05-02 19:41:21]

lgtm, thanks!

we can do the macros in a followup when we need them

and re interstitial object, i think it's minor so no need to worry about it.

https://codereview.chromium.org/253013002/diff/30014/content/browser/renderer...
File content/browser/renderer_host/java/java_bridge_dispatcher_host_manager.h
(right):

https://codereview.chromium.org/253013002/diff/30014/content/browser/renderer...
content/browser/renderer_host/java/java_bridge_dispatcher_host_manager.h:57: 
nit: without the blank line was conforming to the style guide

https://codereview.chromium.org/253013002/diff/30014/content/public/browser/w...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/253013002/diff/30014/content/public/browser/w...
content/public/browser/web_contents_observer.h:339: // possibilities for
cross-site interaction.
nit: remove "or RenderFrameObserver::Send" since there are various wayts to get
to RF::Send and we don't want to enumerate them all here.

"cross-site interaction" is going to be a bit cryptic for most devs. i would
just remove this last sentance.

[mnaganov (inactive), 2014-05-06 09:06:46]

Thank you, John!

https://codereview.chromium.org/253013002/diff/30014/content/browser/renderer...
File content/browser/renderer_host/java/java_bridge_dispatcher_host_manager.h
(right):

https://codereview.chromium.org/253013002/diff/30014/content/browser/renderer...
content/browser/renderer_host/java/java_bridge_dispatcher_host_manager.h:57: 
On 2014/05/02 19:41:22, jam wrote:
> nit: without the blank line was conforming to the style guide

Sorry, restored.

https://codereview.chromium.org/253013002/diff/30014/content/public/browser/w...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/253013002/diff/30014/content/public/browser/w...
content/public/browser/web_contents_observer.h:339: // possibilities for
cross-site interaction.
On 2014/05/02 19:41:22, jam wrote:
> nit: remove "or RenderFrameObserver::Send" since there are various wayts to
get
> to RF::Send and we don't want to enumerate them all here.
> 
> "cross-site interaction" is going to be a bit cryptic for most devs. i would
> just remove this last sentance.

Done.

[mnaganov (inactive), 2014-05-06 09:08:42]

benm@: Can you please take a look at the changes for Java Bridge? I'm addressing
a TODO assigned to you.

yoz@: Please take a look at the extention_function.cc change

[Yoyo Zhou, 2014-05-06 18:23:20]

extension_function.cc LGTM

[mnaganov (inactive), 2014-05-07 10:53:19]

Some tests were failing due to usage of RenderFrame-specific messages. Moved the
respective handlers into the RFH-specific OnMessageHandler.

Need more OWNER reviews:

bauerb@: chrome/browser/plugins
fsamuel@: chrome/browser/guest_view

benm@: Ping?

[Bernhard Bauer, 2014-05-07 12:50:48]

c/b/p LGTM

[Fady Samuel, 2014-05-07 12:53:35]

guest_view lgtm

[benm (inactive), 2014-05-07 13:51:42]

lgtm thanks Mikhail!

https://codereview.chromium.org/253013002/diff/70001/content/browser/renderer...
File content/browser/renderer_host/java/java_bridge_dispatcher_host_manager.cc
(right):

https://codereview.chromium.org/253013002/diff/70001/content/browser/renderer...
content/browser/renderer_host/java/java_bridge_dispatcher_host_manager.cc:30:
DCHECK(retained_object_set);
should this be retained_object_set_ (i.e. the member, not the parameter?)

[mnaganov (inactive), 2014-05-07 15:12:34]

https://codereview.chromium.org/253013002/diff/70001/content/browser/renderer...
File content/browser/renderer_host/java/java_bridge_dispatcher_host_manager.cc
(right):

https://codereview.chromium.org/253013002/diff/70001/content/browser/renderer...
content/browser/renderer_host/java/java_bridge_dispatcher_host_manager.cc:30:
DCHECK(retained_object_set);
On 2014/05/07 13:51:43, benm wrote:
> should this be retained_object_set_ (i.e. the member, not the parameter?)

Here we are checking that we have received an actual object, not a Null. And the
member is JavaObjectWeakGlobalRef, thus it's a "processed" argument, and it can
be null for some other reason.

[mnaganov (inactive), 2014-05-07 15:13:43]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-05-07 15:19:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mnaganov@chromium.org/253013002/90001

[commit-bot: I haz the power, 2014-05-07 20:52:26]

Change committed as 268939