Browser Plugin: New Implementation (Browser Side)

Browser Plugin: New Implementation (browser side)

BUG=none
TEST=manually

[Fady Samuel, 2012-08-08 19:18:04]

This is just about ready for review. I'm just waiting for Darin to review
http://codereview.chromium.org/10826189/ then I can do the final finishing
touches on this.

BrowserPluginHost and BrowserPluginHostHelper are more or less ready for review
so please take a look at those when you get the chance.

[Charlie Reis, 2012-08-13 17:50:14]

My main design question is about BrowserPluginHost acting as both embedder and
guest (and being present on all WebContents, when very few will actually use
it).

http://codereview.chromium.org/10560022/diff/56089/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/10560022/diff/56089/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:322: VIEW_TYPE_APP_SHELL))
nit: This if needs braces because the condition doesn't fit on one line.

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host.cc (right):

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.cc:82:
base::KillProcess(process_handle, RESULT_CODE_HUNG, false);
I don't think we want to kill an unresponsive renderer.  This is a very common
event on heavily loaded clients (or cases like session restore), and there's
often a very good chance the renderer will finish what it's doing.  This is
going to result in a lot of kills.

Is there any other way to handle an unresponsive guest?

Also, this is another case where it's not clear whether this applies to just
guests or both embedders and guests.  We certainly wouldn't want to kill an
unresponsive embedder.

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.cc:92: long long frame_id,
Most frame_ids in view_messages.h are int64.  Why is this a long long?

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.cc:99:
static_cast<WebContentsImpl*>(guest->web_contents()): NULL;
This feels awkward.  Why not just set it to null here, and assign it to
guest->web_contents() in the else block of the if below?

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.cc:104:
base::StringPrintf("%s://%s", chrome::kGuestScheme, host.c_str()));
nit: guest_url -> guest_site

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.cc:107: // than the URL that
the guest is being navigated to.
nit: Please move this comment above the host declaration on line 102.

Also, it's unfortunate we don't have any way to enforce that this is in a
platform app.  Are there things that will break if someone tries to create a
BrowserPlugin elsewhere (e.g., in a WebUI page)?

Also, do we need things to enforce that these guest processes won't share across
apps if the process limit is reached?  (Is there already a bug on file for
this?)

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.cc:110: guest_url);
nit: Wrong indent (should be even with the first argument).

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.cc:120: guest =
guest_web_contents->browser_plugin_host();
I find it odd that all WebContents automatically have a BrowserPluginHost.  Only
a very small fraction of them will ever use it.

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.cc:157: guest ?
static_cast<WebContentsImpl*>(guest->web_contents()): NULL;
nit: Space before :

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.cc:158:
guest->SetDamageBuffer(damage_buffer, gfx::Size(width, height), scale_factor);
Won't this crash if guest is null?  (You just null-checked it on the line
before.)

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.cc:319: 
nit: No empty line.

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.cc:342: GuestSet
guests_to_delete;
This looks like it finds all the guests in the frame that navigated.  What about
the subframes of that frame?

e.g., Page A has subframe B, which itself has subframe C.  If B navigates, we
have to destroy any guests present in B or C.

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.cc:346: if (it->second ==
frame_id) {
nit: No braces needed for one-line if.

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host.h (right):

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.h:40: // A BrowserPluginHost
object is used by both embedders and guests.
I still think having the BrowserPluginHost act as both embedder and guest makes
this code hard to follow.  First, it's not clear which parts of the API are for
which role.  Second, there's a 1:1 relationship between BrowserPlugin and
BrowserPluginHost for guests (like most Foo/FooHost classes), but it sounds like
a single embedder is supposed to manage multiple guests.

Is there a clean way to split this?  Or at least very clearly delineate which
parts of the class are for which role?

I still don't understand why it's useful for BrowserPluginHost to serve both
roles (without apparently knowing which role it is in).  This makes its API
harder to understand.

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.h:61: void
NavigateOrCreateGuest(RenderViewHost* render_view_host,
I had asked this to be renamed to NavigateGuest in
https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows....
 The comments make it clear that it creates the guest if necessary.

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.h:98: return
embedder_render_process_host_;
Why do we need to cache this?  Is it present for both embedders and guests?

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.h:100: int instance_id()
const { return instance_id_; }
This is important enough that it needs a comment indicating what it represents.

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.h:120: BrowserPluginHost*
GetGuestByInstanceID(int container_id) const;
Why is it called ByInstanceID if you're passing in a container ID?  How do those
differ?

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host.h:131: void
set_instance_id(int instance_id) { instance_id_ = instance_id; }
Can this change over time?  I would recommend setting it on construction rather
than having a period of time where you might not have one.

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:23: // Watches
for an GuestCrashed notification.
nit: an -> a

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:57: bool ran_;
nit: observed_

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:69:
IN_PROC_BROWSER_TEST_F(BrowserPluginHostTest,
It would really help to describe a bit of what you're going to be testing.  From
first glance, it's not clear that this test is about an infinite loop page that
you expect to be killed.

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:70:
NavigateOrCreateGuest) {
Ditto: NavigateGuest

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:114:
crash_observer.Wait();
I still wonder about the kill behavior here.  A 5 second timeout is going to
lead to a lot of unnecessary kills in practice, but lengthening the timeout is
going to make this test very slow.  Is there any way to let the embedder react
to a notification that the guest has hung?

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_helper.h (right):

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_helper.h:29: // This class
acts as a plumber helper for BrowserPluginHost. A lot
I'm not entirely clear why this is its own class.  Is this an artifact of using
BrowserPluginHost for both embedders and guests?

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_helper.h:40: // Make it
public for sync IPCs.
Which ones are synchronous?

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_helper.h:44: long long
frame_id,
nit: Wrong indent.

http://codereview.chromium.org/10560022/diff/56089/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_helper.h:72: 
nit: Extra space.

http://codereview.chromium.org/10560022/diff/56089/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.h (right):

http://codereview.chromium.org/10560022/diff/56089/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.h:393: void ResetFlags();
Needs a comment.  What flags are we talking about?

http://codereview.chromium.org/10560022/diff/56089/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.h (right):

http://codereview.chromium.org/10560022/diff/56089/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.h:127:
content::RenderWidgetHostView* GetCreatedWidget(int route_id);
As discussed in the other review, we probably don't want to make this public.

http://codereview.chromium.org/10560022/diff/56089/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.h:687: // Manages the browser
plugin instances hosted by this WebContents.
Again, this isn't clear about the embedder vs guest roles.

http://codereview.chromium.org/10560022/diff/56089/content/public/browser/not...
File content/public/browser/notification_types.h (right):

http://codereview.chromium.org/10560022/diff/56089/content/public/browser/not...
content/public/browser/notification_types.h:373:
NOTIFICATION_BROWSER_PLUGIN_GUEST_CRASHED,
Please add comments.

[Fady Samuel, 2012-08-13 21:30:08]

Addressed some comments. Not yet ready for further review. Handing off to
Istiaque as I head off for vacation.

https://chromiumcodereview.appspot.com/10560022/diff/56089/content/browser/br...
File content/browser/browser_plugin/browser_plugin_host.cc (right):

https://chromiumcodereview.appspot.com/10560022/diff/56089/content/browser/br...
content/browser/browser_plugin/browser_plugin_host.cc:82:
base::KillProcess(process_handle, RESULT_CODE_HUNG, false);
On 2012/08/13 17:50:14, creis wrote:
> I don't think we want to kill an unresponsive renderer.  This is a very common
> event on heavily loaded clients (or cases like session restore), and there's
> often a very good chance the renderer will finish what it's doing.  This is
> going to result in a lot of kills.
> 
> Is there any other way to handle an unresponsive guest?
> 
> Also, this is another case where it's not clear whether this applies to just
> guests or both embedders and guests.  We certainly wouldn't want to kill an
> unresponsive embedder.

Discussed offline. For the time being this still needs to happen in the guest
because the guest can potentially hang the embedder on input events.

https://chromiumcodereview.appspot.com/10560022/diff/56089/content/browser/br...
content/browser/browser_plugin/browser_plugin_host.cc:92: long long frame_id,
On 2012/08/13 17:50:14, creis wrote:
> Most frame_ids in view_messages.h are int64.  Why is this a long long?

Done.

https://chromiumcodereview.appspot.com/10560022/diff/56089/content/browser/br...
File content/browser/browser_plugin/browser_plugin_host.h (right):

https://chromiumcodereview.appspot.com/10560022/diff/56089/content/browser/br...
content/browser/browser_plugin/browser_plugin_host.h:61: void
NavigateOrCreateGuest(RenderViewHost* render_view_host,
On 2012/08/13 17:50:14, creis wrote:
> I had asked this to be renamed to NavigateGuest in
>
https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows....
>  The comments make it clear that it creates the guest if necessary.

Ohh sorry, it looks like I missed addressing that particular comment last time.
Sorry about that. Fixing now.

https://chromiumcodereview.appspot.com/10560022/diff/56089/content/browser/br...
content/browser/browser_plugin/browser_plugin_host.h:98: return
embedder_render_process_host_;
On 2012/08/13 17:50:14, creis wrote:
> Why do we need to cache this?  Is it present for both embedders and guests?

This is used to talk back to the embedder.

https://chromiumcodereview.appspot.com/10560022/diff/56089/content/browser/br...
content/browser/browser_plugin/browser_plugin_host.h:120: BrowserPluginHost*
GetGuestByInstanceID(int container_id) const;
On 2012/08/13 17:50:14, creis wrote:
> Why is it called ByInstanceID if you're passing in a container ID?  How do
those
> differ?

I used to call this container_id then I renamed it to instance_id. I fixed this.

[jam, 2012-08-13 21:35:35]

(just saw a change to this file, didn't look at the rest)

http://codereview.chromium.org/10560022/diff/59102/content/public/browser/not...
File content/public/browser/notification_types.h (right):

http://codereview.chromium.org/10560022/diff/59102/content/public/browser/not...
content/public/browser/notification_types.h:375:
NOTIFICATION_BROWSER_PLUGIN_UPDATE_RECT,
please try to find a way to do these tests without notifications. notifications
are evil. although it makes writing the test easier, this can be (ab)used by
other code in content (and worse, chrome, thereby having a side api parallel to
the content api). this makes refactorings hard.

[Fady Samuel, 2012-09-12 19:14:57]

Closing this patch, this has been superseded by Istiaque's patch. Thanks.