This patch improves the way that GuestViewManager tracks the destruction of GuestView embedders.

This patch improves the way that GuestViewManager tracks the destruction of GuestView embedders. A RenderProcessHostObserver is now used instead of a WebContentsObserver.

There were two main problems with the existing method that are now fixed:
1) The WebContentsObserver would only notify if an embedder crashed, and did nothingwhen it closed normally. (this led to the WebView in the chrome sign-in page not getting cleaned up properly when navigating away from that page)
2) The WebContentsObserver was created in GuestViewBase, so it would not even exist if a GuestView's embedder crashed before its GuestViewBase instance was created.

BUG=355360
Committed: https://crrev.com/bc4600abb6ba65953c6cefd139062e21f0b64977
Cr-Commit-Position: refs/heads/master@{#339658}

[paulmeyer, 2015-07-10 15:18:36]

Patchset #1 (id:1) has been deleted

[paulmeyer, 2015-07-10 15:19:08]

paulmeyer@chromium.org changed reviewers:
+ lazyboy@chromium.org

[Fady Samuel, 2015-07-10 15:24:00]

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

[Fady Samuel, 2015-07-10 15:24:01]

https://codereview.chromium.org/1232603002/diff/20001/components/guest_view/b...
File components/guest_view/browser/guest_view_manager.cc (right):

https://codereview.chromium.org/1232603002/diff/20001/components/guest_view/b...
components/guest_view/browser/guest_view_manager.cc:364:
embedder_render_process_host_observers_.push_back(
When do you remove an observer? I would just have an observer manage its own
lifetime. If you never need to iterate over observers then you don't need a
vector.

[paulmeyer, 2015-07-10 15:34:47]

fsamuel@: ptal.

https://codereview.chromium.org/1232603002/diff/20001/components/guest_view/b...
File components/guest_view/browser/guest_view_manager.cc (right):

https://codereview.chromium.org/1232603002/diff/20001/components/guest_view/b...
components/guest_view/browser/guest_view_manager.cc:364:
embedder_render_process_host_observers_.push_back(
On 2015/07/10 15:24:01, Fady Samuel wrote:
> When do you remove an observer? I would just have an observer manage its own
> lifetime. If you never need to iterate over observers then you don't need a
> vector.

Done.

[Fady Samuel, 2015-07-10 15:37:00]

https://codereview.chromium.org/1232603002/diff/40001/components/guest_view/b...
File components/guest_view/browser/guest_view_manager.cc (right):

https://codereview.chromium.org/1232603002/diff/40001/components/guest_view/b...
components/guest_view/browser/guest_view_manager.cc:367: new
EmbedderRenderProcessHostObserver(this, embedder_process_id);
Hmm, what if you add a webview, remove it, and then add a new one? It seems like
you'll end up with multiple observers for the same embedder, am I right?

[paulmeyer, 2015-07-10 20:05:25]

ptal.

https://codereview.chromium.org/1232603002/diff/40001/components/guest_view/b...
File components/guest_view/browser/guest_view_manager.cc (right):

https://codereview.chromium.org/1232603002/diff/40001/components/guest_view/b...
components/guest_view/browser/guest_view_manager.cc:367: new
EmbedderRenderProcessHostObserver(this, embedder_process_id);
On 2015/07/10 15:36:59, Fady Samuel wrote:
> Hmm, what if you add a webview, remove it, and then add a new one? It seems
like
> you'll end up with multiple observers for the same embedder, am I right?

You're right, that could happen. I'll use something else to trigger the creation
of the observers.

[Fady Samuel, 2015-07-14 17:56:59]

https://codereview.chromium.org/1232603002/diff/60001/components/guest_view/b...
File components/guest_view/browser/guest_view_manager.cc (right):

https://codereview.chromium.org/1232603002/diff/60001/components/guest_view/b...
components/guest_view/browser/guest_view_manager.cc:298: context_,
Why does the cleanup function need the browser context?

[paulmeyer, 2015-07-14 18:04:07]

https://codereview.chromium.org/1232603002/diff/60001/components/guest_view/b...
File components/guest_view/browser/guest_view_manager.cc (right):

https://codereview.chromium.org/1232603002/diff/60001/components/guest_view/b...
components/guest_view/browser/guest_view_manager.cc:298: context_,
On 2015/07/14 17:56:59, Fady Samuel wrote:
> Why does the cleanup function need the browser context?

It always needed it to clean up some of the browser-side state. It used to rely
on getting it via the RenderProcessHost, but I don't think that is actually
always safe since cleanup can be triggered by the destruction of the
RenderProcessHost. So, I'm now just passing it in instead.

[Fady Samuel, 2015-07-16 13:37:06]

lgtm

[paulmeyer, 2015-07-16 13:39:15]

The CQ bit was checked by paulmeyer@chromium.org

[commit-bot: I haz the power, 2015-07-16 13:39:28]

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

[commit-bot: I haz the power, 2015-07-16 14:05:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-16 14:05:03]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

(exceeded global retry quota)

[paulmeyer, 2015-07-16 14:13:56]

The CQ bit was checked by paulmeyer@chromium.org

[paulmeyer, 2015-07-16 14:13:56]

The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/1232603002/#ps80001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-07-16 14:14:34]

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

[commit-bot: I haz the power, 2015-07-16 14:52:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-16 14:52:42]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

(exceeded global retry quota)

[paulmeyer, 2015-07-17 13:20:44]

Patchset #5 (id:100001) has been deleted

[paulmeyer, 2015-07-17 19:28:43]

Patchset #5 (id:120001) has been deleted

[paulmeyer, 2015-07-20 17:09:33]

paulmeyer@chromium.org changed reviewers:
+ kalman@chromium.org

[paulmeyer, 2015-07-20 17:09:34]

+kalman@ for rules_registry_service.* and browser_context_keyed_api_factory.h

These changes were made to avoid creating a new RulesRegistryService when all we
want is to clean up an existing one.

[not at google - send to devlin, 2015-07-20 21:17:57]

https://codereview.chromium.org/1232603002/diff/140001/extensions/browser/bro...
File extensions/browser/browser_context_keyed_api_factory.h (right):

https://codereview.chromium.org/1232603002/diff/140001/extensions/browser/bro...
extensions/browser/browser_context_keyed_api_factory.h:79: static T*
Get(content::BrowserContext* context, bool create) {
I'd rather a method like "GetIfExists" in addition to "Get", rather than having
a method which may or may not have a side-effect based on a boolean value it
gets passed.

Other than that, lgtm.

[paulmeyer, 2015-07-21 14:37:43]

Patchset #6 (id:160001) has been deleted

[paulmeyer, 2015-07-21 14:43:49]

The CQ bit was checked by paulmeyer@chromium.org

[paulmeyer, 2015-07-21 14:43:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1232603002/#ps180001
(title: "Addressed comment by kalman@.")

[commit-bot: I haz the power, 2015-07-21 14:43:57]

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

[commit-bot: I haz the power, 2015-07-21 15:41:55]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2015-07-21 15:43:49]

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