Remove the RenderProcessHost observer and attach the WebContentsObserver earlier to the GuestView.

Remove the RenderProcessHost observer and attach the WebContentsObserver earlier to the GuestView.

This fixes an issue where if the embedder WebContents is destroyed during a GuestView creation we may end up leaking the guest WebContents.

BUG=419020
Committed: https://crrev.com/52fce9455ffd688ef3752a816192f9e76555e7d1
Cr-Commit-Position: refs/heads/master@{#298934}

Committed: https://crrev.com/97437544d4891fa80201673a2f64e443fe87065a
Cr-Commit-Position: refs/heads/master@{#299117}

[lfg, 2014-10-08 21:07:42]

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

[lfg, 2014-10-08 21:07:43]

PTAL

[Fady Samuel, 2014-10-08 21:45:32]

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/guest_view_base.cc (left):

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.cc:168: int
embedder_render_process_id,
Looks like we can get rid of this. You can  instead use the
embedder_web_contents to grab the embedder_render_process_id_.

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.cc:491: int
embedder_render_process_id,
Do we need to pass in this field as well? It seems redundant if we have an
embedder_web_contents.

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.cc:60: class
GuestViewBase::EmbedderWebContentsObserver : public WebContentsObserver {
Maybe if we rename this to EmbedderLifetimeObserver, it'll be clearer what this
does?

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.cc:62: explicit
EmbedderWebContentsObserver(
remove explicit

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.cc:483:
embedder_render_process_id,
Remove this.

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/guest_view_base.h (left):

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.h:265: int
embedder_render_process_id,
Remove this.

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.h:174: int
embedder_render_process_id,
Remove this.

[lfg, 2014-10-08 23:25:36]

PTAL.

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/guest_view_base.cc (left):

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.cc:168: int
embedder_render_process_id,
On 2014/10/08 21:45:32, Fady Samuel wrote:
> Looks like we can get rid of this. You can  instead use the
> embedder_web_contents to grab the embedder_render_process_id_.

Done.

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.cc:491: int
embedder_render_process_id,
On 2014/10/08 21:45:32, Fady Samuel wrote:
> Do we need to pass in this field as well? It seems redundant if we have an
> embedder_web_contents.

Done.

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.cc:60: class
GuestViewBase::EmbedderWebContentsObserver : public WebContentsObserver {
On 2014/10/08 21:45:32, Fady Samuel wrote:
> Maybe if we rename this to EmbedderLifetimeObserver, it'll be clearer what
this
> does?

Done.

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.cc:62: explicit
EmbedderWebContentsObserver(
On 2014/10/08 21:45:32, Fady Samuel wrote:
> remove explicit

Done.

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.cc:483:
embedder_render_process_id,
On 2014/10/08 21:45:32, Fady Samuel wrote:
> Remove this.

Done.

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/guest_view_base.h (left):

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.h:265: int
embedder_render_process_id,
On 2014/10/08 21:45:32, Fady Samuel wrote:
> Remove this.

Done.

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/642573002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.h:174: int
embedder_render_process_id,
On 2014/10/08 21:45:32, Fady Samuel wrote:
> Remove this.

Done.

[Fady Samuel, 2014-10-08 23:41:15]

lgtm

[Fady Samuel, 2014-10-08 23:43:27]

https://codereview.chromium.org/642573002/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/guest_view_base.cc (left):

https://codereview.chromium.org/642573002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.cc:379: new
EmbedderWebContentsObserver(this));
Hmm now that I think about it. Maybe we want to create a new
EmbedderLifetimeObserver here in case the embedder WebContents changes.

[lfg, 2014-10-09 15:38:08]

I've added a check on attachment to see if the embedder_web_contents have
changed, if they have, then create a new lifetime observer.

Please, take another look.

https://codereview.chromium.org/642573002/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/guest_view_base.cc (left):

https://codereview.chromium.org/642573002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.cc:379: new
EmbedderWebContentsObserver(this));
On 2014/10/08 23:43:27, Fady Samuel wrote:
> Hmm now that I think about it. Maybe we want to create a new
> EmbedderLifetimeObserver here in case the embedder WebContents changes.

Done.

[Fady Samuel, 2014-10-09 15:58:49]

lgtm + nit.

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

https://codereview.chromium.org/642573002/diff/40001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.cc:363:
embedder_web_contents_observer_.reset(
nit: since this statement occupies multiple lines, please place this in a block
to avoid future confusion.

[lfg, 2014-10-09 17:56:02]

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

https://codereview.chromium.org/642573002/diff/40001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.cc:363:
embedder_web_contents_observer_.reset(
On 2014/10/09 15:58:49, Fady Samuel wrote:
> nit: since this statement occupies multiple lines, please place this in a
block
> to avoid future confusion.

Done.

[lfg, 2014-10-09 17:56:06]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2014-10-09 17:57:36]

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

[commit-bot: I haz the power, 2014-10-09 18:14:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-09 18:15:27]

Patchset 4 (id:??) landed as
https://crrev.com/52fce9455ffd688ef3752a816192f9e76555e7d1
Cr-Commit-Position: refs/heads/master@{#298934}

[danakj, 2014-10-09 20:38:49]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/640293005/ by danakj@chromium.org.

The reason for reverting is: Speculative revert for double frees on mac gpu
bots.

http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...
http://build.chromium.org/p/chromium.gpu/builders/Mac%20Debug%20%28Intel%29/b....

[Fady Samuel, 2014-10-10 01:34:36]

On 2014/10/09 20:38:49, danakj wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/640293005/ by mailto:danakj@chromium.org.
> 
> The reason for reverting is: Speculative revert for double frees on mac gpu
> bots.
> 
>
http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...
>
http://build.chromium.org/p/chromium.gpu/builders/Mac%20Debug%20%28Intel%29/b....

This seems unrelated to me but I'm not sure. Lucas, please sync up with Dana if
need be.

[lfg, 2014-10-10 15:17:06]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2014-10-10 15:18:17]

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

[commit-bot: I haz the power, 2014-10-10 15:19:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-10 15:19:50]

Patchset 4 (id:??) landed as
https://crrev.com/97437544d4891fa80201673a2f64e443fe87065a
Cr-Commit-Position: refs/heads/master@{#299117}