GuestView: Move lifetime management out of content

GuestView: Move lifetime management out of content

This CL introduces an internal destroyGuest API, and enables preservation of
guests when display:none is set.

This patch is also a big step towards guest teleportation.

BUG=419020, 330264
TBR=kenrb@chromium.org for browser_plugin_messages.h IPC removal, asvitkine@chromium.org for histograms.xml

Committed: https://crrev.com/a8484dd476ef402fc235a3e9c19b1d52ac0d0f6e
Cr-Commit-Position: refs/heads/master@{#297751}

[Fady Samuel, 2014-09-30 18:05:57]

fsamuel@chromium.org changed reviewers:
+ lfg@chromium.org

[lfg, 2014-09-30 18:59:27]

Just a few comments and/or questions.

https://codereview.chromium.org/618823002/diff/60001/extensions/browser/api/g...
File extensions/browser/api/guest_view/guest_view_internal_api.cc (right):

https://codereview.chromium.org/618823002/diff/60001/extensions/browser/api/g...
extensions/browser/api/guest_view/guest_view_internal_api.cc:83: return false;
Maybe should provide feedback to the javascript API that there was an error
destroying the guest.

https://codereview.chromium.org/618823002/diff/60001/extensions/browser/guest...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/618823002/diff/60001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.cc:84: Destroy();
Why do we need this?

Is it safe? This call will destroy the WebContents, which is the object that
called this function in the first place.

https://codereview.chromium.org/618823002/diff/60001/extensions/renderer/reso...
File extensions/renderer/resources/web_view.js (right):

https://codereview.chromium.org/618823002/diff/60001/extensions/renderer/reso...
extensions/renderer/resources/web_view.js:145:
GuestViewInternal.destroyGuest(this.guestInstanceId);
If a webview element is attached and immediatelly detached, wouldn't it be
possible that the guestInstanceId wasn't allocated yet, and we could leak the
guest? I see that guestInstanceId is only assigned on a callback from
createGuest.

[Fady Samuel, 2014-09-30 22:50:04]

PTAL

https://codereview.chromium.org/618823002/diff/60001/extensions/browser/api/g...
File extensions/browser/api/guest_view/guest_view_internal_api.cc (right):

https://codereview.chromium.org/618823002/diff/60001/extensions/browser/api/g...
extensions/browser/api/guest_view/guest_view_internal_api.cc:83: return false;
On 2014/09/30 18:59:27, lfg wrote:
> Maybe should provide feedback to the javascript API that there was an error
> destroying the guest.

This code is likely racy at tear down. The embedder process is getting rid of
its elements, and the browser process sees that the embedder is doing that and
tries to kill the guest as well. I'm hesitant to add an error.

https://codereview.chromium.org/618823002/diff/60001/extensions/browser/guest...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/618823002/diff/60001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.cc:84: Destroy();
On 2014/09/30 18:59:27, lfg wrote:
> Why do we need this?
> 
> Is it safe? This call will destroy the WebContents, which is the object that
> called this function in the first place.

Add comments throughout to explain lifetime management.

https://codereview.chromium.org/618823002/diff/60001/extensions/renderer/reso...
File extensions/renderer/resources/web_view.js (right):

https://codereview.chromium.org/618823002/diff/60001/extensions/renderer/reso...
extensions/renderer/resources/web_view.js:145:
GuestViewInternal.destroyGuest(this.guestInstanceId);
On 2014/09/30 18:59:27, lfg wrote:
> If a webview element is attached and immediatelly detached, wouldn't it be
> possible that the guestInstanceId wasn't allocated yet, and we could leak the
> guest? I see that guestInstanceId is only assigned on a callback from
> createGuest.

Fixed..I think but this code is VERY brittle now. Paul will take care of making
this code cleaner and make more sense.

[lfg, 2014-09-30 23:29:42]

Just one typo in extension_options.js:371, otherwise LGTM.

https://codereview.chromium.org/618823002/diff/60001/extensions/browser/api/g...
File extensions/browser/api/guest_view/guest_view_internal_api.cc (right):

https://codereview.chromium.org/618823002/diff/60001/extensions/browser/api/g...
extensions/browser/api/guest_view/guest_view_internal_api.cc:83: return false;
On 2014/09/30 22:50:03, Fady Samuel wrote:
> On 2014/09/30 18:59:27, lfg wrote:
> > Maybe should provide feedback to the javascript API that there was an error
> > destroying the guest.
> 
> This code is likely racy at tear down. The embedder process is getting rid of
> its elements, and the browser process sees that the embedder is doing that and
> tries to kill the guest as well. I'm hesitant to add an error.

Acknowledged.

https://codereview.chromium.org/618823002/diff/60001/extensions/browser/guest...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/618823002/diff/60001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.cc:84: Destroy();
On 2014/09/30 22:50:03, Fady Samuel wrote:
> On 2014/09/30 18:59:27, lfg wrote:
> > Why do we need this?
> > 
> > Is it safe? This call will destroy the WebContents, which is the object that
> > called this function in the first place.
> 
> Add comments throughout to explain lifetime management.

I see. Makes sense now :)

https://codereview.chromium.org/618823002/diff/60001/extensions/renderer/reso...
File extensions/renderer/resources/web_view.js (right):

https://codereview.chromium.org/618823002/diff/60001/extensions/renderer/reso...
extensions/renderer/resources/web_view.js:145:
GuestViewInternal.destroyGuest(this.guestInstanceId);
On 2014/09/30 22:50:03, Fady Samuel wrote:
> On 2014/09/30 18:59:27, lfg wrote:
> > If a webview element is attached and immediatelly detached, wouldn't it be
> > possible that the guestInstanceId wasn't allocated yet, and we could leak
the
> > guest? I see that guestInstanceId is only assigned on a callback from
> > createGuest.
> 
> Fixed..I think but this code is VERY brittle now. Paul will take care of
making
> this code cleaner and make more sense. 

Acknowledged.

https://codereview.chromium.org/618823002/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/extension_options.js (right):

https://codereview.chromium.org/618823002/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:371:
intenral.elementAttached = false;
typo.

[Fady Samuel, 2014-10-01 15:40:15]

fsamuel@chromium.org changed reviewers:
+ creis@chromium.org, rockot@chromium.org

[Fady Samuel, 2014-10-01 15:40:16]

Hi all, this CL fixes a process leak (scary!) in <webview> and other elements
and FINALLY allows guest preservation on display:none (a major <webview> pain
point that I've been hearing from a lot of people).

+creis@ please review:

content/browser/web_contents/web_contents_view_guest.h
content/browser/web_contents/web_contents_view_guest.cc
content/public/browser/browser_plugin_guest_delegate.h

+rockot@ please review:

extensions/common/api/guest_view_internal.json
extensions/browser/api/web_view/web_view_internal_api.cc
extensions/browser/api/guest_view/guest_view_internal_api.cc
extensions/browser/api/guest_view/guest_view_internal_api.h

https://codereview.chromium.org/618823002/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/extension_options.js (right):

https://codereview.chromium.org/618823002/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:371:
intenral.elementAttached = false;
On 2014/09/30 23:29:42, lfg wrote:
> typo.

Done.

[Ken Rockot(use gerrit already), 2014-10-01 16:51:52]

On 2014/10/01 15:40:16, Fady Samuel wrote:
> +rockot@ please review:
> 
> extensions/common/api/guest_view_internal.json
> extensions/browser/api/web_view/web_view_internal_api.cc
> extensions/browser/api/guest_view/guest_view_internal_api.cc
> extensions/browser/api/guest_view/guest_view_internal_api.h
> 

said files lgtm

[Charlie Reis, 2014-10-01 17:11:00]

content/ LGTM.

https://codereview.chromium.org/618823002/diff/120001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/618823002/diff/120001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.h:363: int
guest_proxy_routing_id_;
nit: Please document what this is.

[Fady Samuel, 2014-10-01 21:40:52]

fsamuel@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Fady Samuel, 2014-10-01 21:40:53]

+asvitkine@ for tools/metrics/histograms/histograms.xml

Thanks!

https://codereview.chromium.org/618823002/diff/120001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/618823002/diff/120001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.h:363: int
guest_proxy_routing_id_;
On 2014/10/01 17:11:00, Charlie Reis wrote:
> nit: Please document what this is.

Done.

[Fady Samuel, 2014-10-01 22:25:30]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-10-01 22:27:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/618823002/170001

[Alexei Svitkine (slow), 2014-10-01 23:47:27]

lgtm

[commit-bot: I haz the power, 2014-10-02 00:51:43]

Committed patchset #10 (id:170001) as 9352f18b3d458c020b5a7af8f7835da2746974f7

[commit-bot: I haz the power, 2014-10-02 00:52:19]

Patchset 10 (id:??) landed as
https://crrev.com/a8484dd476ef402fc235a3e9c19b1d52ac0d0f6e
Cr-Commit-Position: refs/heads/master@{#297751}