Getting rid of more webview memory leaks.

This patch implements a way to register callbacks for when a GuestView is destroyed in GuestViewManager.

This new callback system is used to do some memory cleanup in WebViewGuest when each view is destroyed, instead of only when the embedder is destroyed.

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

[paulmeyer, 2015-06-05 20:05:01]

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

[lazyboy, 2015-06-05 23:09:56]

Some comments.

High level issues found so far:
1) The map of map of vector of items.
2) Too much usage of auto in some cases.

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
File components/guest_view/browser/guest_view_base.cc (right):

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
components/guest_view/browser/guest_view_base.cc:113:
GuestViewManager::FromBrowserContext(web_contents()->GetBrowserContext())->
-> goes next line, can you run git cl format on your CL?

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
File components/guest_view/browser/guest_view_base.h (right):

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
components/guest_view/browser/guest_view_base.h:63: // Clean up state when this
GuestView is being destroyed. Note that this
nits: Cleans up ...
"Note that .. " goes to next line.

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
components/guest_view/browser/guest_view_base.h:66: // instance. This method can
be hidden in a derived class, in which case the
I'm not sure I understand the hidden part immediately. Can you reword this a
bit.

You mean derived classes can provide their own T::CleanUp()?

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
File components/guest_view/browser/guest_view_manager.cc (right):

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
components/guest_view/browser/guest_view_manager.cc:255: "Invalid GuestView
created of type \"" << view_type << "\"";
Duplicate GuestView created of type ...

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
components/guest_view/browser/guest_view_manager.cc:272: auto&
callbacks_for_embedder = embedder_it->second;
I'm already lost here with auto-s :)
I'd highly encourage you to type these ones.

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
File components/guest_view/browser/guest_view_manager.h (right):

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
components/guest_view/browser/guest_view_manager.h:93: auto new_entry =
std::make_pair(T::Type, GuestViewData(
nit: Can we break lines a bit differently?

std::make_pair(
    T::Type,
    GuestViewData(base::Bind(&T::Create),
                  base::Bind(&T::CleanUp)));

It's concise if each param has their own indentation for this kind of
situation, much easier to read.

nit: s/new_entry/registry_entry

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
components/guest_view/browser/guest_view_manager.h:100: void
RegisterViewCallback(int embedder_process_id,
RegisterViewDestructionCallback

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
components/guest_view/browser/guest_view_manager.h:221: const
GuestViewCreateFunction create;
s/create/create_function
s/clean_up/cleanup_function

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
components/guest_view/browser/guest_view_manager.h:244: std::map<int,
std::map<int, std::vector<base::Closure>>> view_callback_map_;
1)
It's not obvious from the usage of this map why the callback
functions need to be vector and not just one base::Closure.
In which case does that happen? i.e. we get more than one
destruction callback registered for one view?

You should add comment if that's the case.

2)
view_callback_map_ -> view_destruction_callback_map_

3)
Did you check /w Fady if there's a better way that this...
A way to grab all the view instance ids of an embedder,
given a render process id.

Maybe with
GuestManager::ForEachGuest can help? Altho that doesn't give
you <webview>s that doesn't have GuestView created for them.

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
File components/guest_view/browser/guest_view_message_filter.h (right):

https://chromiumcodereview.appspot.com/1143333008/diff/1/components/guest_vie...
components/guest_view/browser/guest_view_message_filter.h:57: void
OnViewCreated(int view_instance_id, const std::string& view_type);
You should make this one and the next one private.

Move it below OnAttachGuest().

https://chromiumcodereview.appspot.com/1143333008/diff/1/extensions/browser/g...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://chromiumcodereview.appspot.com/1143333008/diff/1/extensions/browser/g...
extensions/browser/guest_view/web_view/web_view_guest.cc:190: static
base::LazyInstance<WebViewKeyToIDMap> web_view_key_to_id_map =
should we put these inside anonymous namespace?

https://chromiumcodereview.appspot.com/1143333008/diff/1/extensions/browser/g...
extensions/browser/guest_view/web_view/web_view_guest.cc:198: auto
browser_context = rph->GetBrowserContext();
If I remember correctly, we were using auto only for
SomeClass::From* when the return type is SomeClass* or SomeClass&

Also this makes the code hard to follow imo.

https://chromiumcodereview.appspot.com/1143333008/diff/1/extensions/renderer/...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://chromiumcodereview.appspot.com/1143333008/diff/1/extensions/renderer/...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:270: //
There are two parameters.
three

https://chromiumcodereview.appspot.com/1143333008/diff/1/extensions/renderer/...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:287: //
The view_instance_id is given to the SetWeak callback so that that view's
nit: |view_instance_id|

[Fady Samuel, 2015-06-06 01:22:53]

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

[Fady Samuel, 2015-06-06 01:22:54]

I'll let Istiaque do most of the reviewing. Please speak to Xi and make sure to
replace her code here:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

WebViewContentScriptManager is an area I'm particularly concerned about. We
could leak content scripts for an indefinite period of time.

[paulmeyer, 2015-06-08 17:53:43]

On 2015/06/06 01:22:54, Fady Samuel wrote:
> I'll let Istiaque do most of the reviewing. Please speak to Xi and make sure
to
> replace her code here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> 
> WebViewContentScriptManager is an area I'm particularly concerned about. We
> could leak content scripts for an indefinite period of time.

I'll address the content scripts in a separate patch.

[paulmeyer, 2015-06-08 17:53:59]

ptal.

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
File components/guest_view/browser/guest_view_base.cc (right):

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_base.cc:113:
GuestViewManager::FromBrowserContext(web_contents()->GetBrowserContext())->
On 2015/06/05 23:09:55, lazyboy wrote:
> -> goes next line, can you run git cl format on your CL?

Done.

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
File components/guest_view/browser/guest_view_base.h (right):

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_base.h:63: // Clean up state when this
GuestView is being destroyed. Note that this
On 2015/06/05 23:09:55, lazyboy wrote:
> nits: Cleans up ...
> "Note that .. " goes to next line.

Done.

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_base.h:66: // instance. This method can
be hidden in a derived class, in which case the
On 2015/06/05 23:09:55, lazyboy wrote:
> I'm not sure I understand the hidden part immediately. Can you reword this a
> bit.
> 
> You mean derived classes can provide their own T::CleanUp()?

Done.

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
File components/guest_view/browser/guest_view_manager.cc (right):

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_manager.cc:255: "Invalid GuestView
created of type \"" << view_type << "\"";
On 2015/06/05 23:09:55, lazyboy wrote:
> Duplicate GuestView created of type ... 

This CHECK isn't checking for duplicates. It checks to make sure that the type
view that has just been created is one of the registered types in
|guest_view_registry_|. If it is not in the registry, then it is not a valid
GuestView type.

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_manager.cc:272: auto&
callbacks_for_embedder = embedder_it->second;
On 2015/06/05 23:09:55, lazyboy wrote:
> I'm already lost here with auto-s :)
> I'd highly encourage you to type these ones.

Hahaha, Good call. I am using quite a lot of autos in a row here. I'll keep the
autos for the iterators, since those are more obvious, but I'll change the
others to explicit types.

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
File components/guest_view/browser/guest_view_manager.h (right):

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_manager.h:93: auto new_entry =
std::make_pair(T::Type, GuestViewData(
On 2015/06/05 23:09:55, lazyboy wrote:
> nit: Can we break lines a bit differently?
> 
> std::make_pair(
>     T::Type,
>     GuestViewData(base::Bind(&T::Create),
>                   base::Bind(&T::CleanUp)));
> 
> It's concise if each param has their own indentation for this kind of
> situation, much easier to read.
> 
> nit: s/new_entry/registry_entry

Done.

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_manager.h:100: void
RegisterViewCallback(int embedder_process_id,
On 2015/06/05 23:09:55, lazyboy wrote:
> RegisterViewDestructionCallback

Done.

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_manager.h:221: const
GuestViewCreateFunction create;
On 2015/06/05 23:09:55, lazyboy wrote:
> s/create/create_function
> s/clean_up/cleanup_function

Done.

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_manager.h:244: std::map<int,
std::map<int, std::vector<base::Closure>>> view_callback_map_;
On 2015/06/05 23:09:55, lazyboy wrote:
> 1)
> It's not obvious from the usage of this map why the callback
> functions need to be vector and not just one base::Closure.
> In which case does that happen? i.e. we get more than one
> destruction callback registered for one view?
> 
> You should add comment if that's the case.
> 
> 2)
> view_callback_map_ -> view_destruction_callback_map_
> 
> 3)
> Did you check /w Fady if there's a better way that this...
> A way to grab all the view instance ids of an embedder,
> given a render process id.
> 
> Maybe with
> GuestManager::ForEachGuest can help? Altho that doesn't give
> you <webview>s that doesn't have GuestView created for them.

I addressed the first two. For the third, as per our offline conversation, I'll
make some typedefs to make this more readable.

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
File components/guest_view/browser/guest_view_message_filter.h (right):

https://codereview.chromium.org/1143333008/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_message_filter.h:57: void
OnViewCreated(int view_instance_id, const std::string& view_type);
On 2015/06/05 23:09:55, lazyboy wrote:
> You should make this one and the next one private.
> 
> Move it below OnAttachGuest().

Done.

https://codereview.chromium.org/1143333008/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1143333008/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.cc:190: static
base::LazyInstance<WebViewKeyToIDMap> web_view_key_to_id_map =
On 2015/06/05 23:09:55, lazyboy wrote:
> should we put these inside anonymous namespace?

Not sure. I didn't actually write this one, I just moved it from below. I'll put
it into the namespace above.

https://codereview.chromium.org/1143333008/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.cc:198: auto
browser_context = rph->GetBrowserContext();
On 2015/06/05 23:09:55, lazyboy wrote:
> If I remember correctly, we were using auto only for
> SomeClass::From* when the return type is SomeClass* or SomeClass&
> 
> Also this makes the code hard to follow imo.

I was under the impression that auto should/can be used whenever the type is
clear from the right-hand-side, like in this case where I am using
GetBrowserContext(), and the type is BrowserContext. I think that that is clear
enough.

https://codereview.chromium.org/1143333008/diff/1/extensions/renderer/guest_v...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://codereview.chromium.org/1143333008/diff/1/extensions/renderer/guest_v...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:270: //
There are two parameters.
On 2015/06/05 23:09:56, lazyboy wrote:
> three

Done.

https://codereview.chromium.org/1143333008/diff/1/extensions/renderer/guest_v...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:287: //
The view_instance_id is given to the SetWeak callback so that that view's
On 2015/06/05 23:09:55, lazyboy wrote:
> nit: |view_instance_id|

Done.

[paulmeyer, 2015-06-08 19:57:49]

Patchset #2 (id:20001) has been deleted

[Fady Samuel, 2015-06-08 20:25:57]

One nit + Please look at cleaning up the WebViewContentScriptManager state!

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

https://codereview.chromium.org/1143333008/diff/40001/components/guest_view/b...
components/guest_view/browser/guest_view_manager.h:246: typedef
std::vector<base::Closure> Callbacks;
nit: use using instead of typedef.

[lazyboy, 2015-06-09 14:19:32]

Looks good, I have one confusion.

https://chromiumcodereview.appspot.com/1143333008/diff/40001/components/guest...
File components/guest_view/browser/guest_view_base.cc (right):

https://chromiumcodereview.appspot.com/1143333008/diff/40001/components/guest...
components/guest_view/browser/guest_view_base.cc:122: // This observer ensures
that the GuestViewBase destroys itself if its opening
s/opening/opener

https://chromiumcodereview.appspot.com/1143333008/diff/40001/components/guest...
File components/guest_view/browser/guest_view_manager.h (right):

https://chromiumcodereview.appspot.com/1143333008/diff/40001/components/guest...
components/guest_view/browser/guest_view_manager.h:135: // The virtual methods
are virtual so that they can be overriden in tests.
nit: These methods are virtual so that ...

https://chromiumcodereview.appspot.com/1143333008/diff/40001/extensions/brows...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://chromiumcodereview.appspot.com/1143333008/diff/40001/extensions/brows...
extensions/browser/api/web_request/web_request_api.cc:1372:
listener.extension_id,
This isn't equivalent to what it used to be, as I'm not too familiar
/w web request api's listeners, I'm not too comfortable with this
change.
Could this potentially clear more listeners that we intended to?

[paulmeyer, 2015-06-09 14:47:16]

On 2015/06/09 14:19:32, lazyboy wrote:
> Looks good, I have one confusion.
> 
>
https://chromiumcodereview.appspot.com/1143333008/diff/40001/components/guest...
> File components/guest_view/browser/guest_view_base.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1143333008/diff/40001/components/guest...
> components/guest_view/browser/guest_view_base.cc:122: // This observer ensures
> that the GuestViewBase destroys itself if its opening
> s/opening/opener
> 
>
https://chromiumcodereview.appspot.com/1143333008/diff/40001/components/guest...
> File components/guest_view/browser/guest_view_manager.h (right):
> 
>
https://chromiumcodereview.appspot.com/1143333008/diff/40001/components/guest...
> components/guest_view/browser/guest_view_manager.h:135: // The virtual methods
> are virtual so that they can be overriden in tests.
> nit: These methods are virtual so that ...
> 
>
https://chromiumcodereview.appspot.com/1143333008/diff/40001/extensions/brows...
> File extensions/browser/api/web_request/web_request_api.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1143333008/diff/40001/extensions/brows...
> extensions/browser/api/web_request/web_request_api.cc:1372:
> listener.extension_id,
> This isn't equivalent to what it used to be, as I'm not too familiar
> /w web request api's listeners, I'm not too comfortable with this
> change.
> Could this potentially clear more listeners that we intended to?

We definitely want to clear all listeners regardless of extension ID, so we
won't be clearing too many.

[paulmeyer, 2015-06-09 14:49:42]

ptal.

https://chromiumcodereview.appspot.com/1143333008/diff/40001/components/guest...
File components/guest_view/browser/guest_view_base.cc (right):

https://chromiumcodereview.appspot.com/1143333008/diff/40001/components/guest...
components/guest_view/browser/guest_view_base.cc:122: // This observer ensures
that the GuestViewBase destroys itself if its opening
On 2015/06/09 14:19:32, lazyboy wrote:
> s/opening/opener

Done.

https://chromiumcodereview.appspot.com/1143333008/diff/40001/components/guest...
File components/guest_view/browser/guest_view_manager.h (right):

https://chromiumcodereview.appspot.com/1143333008/diff/40001/components/guest...
components/guest_view/browser/guest_view_manager.h:135: // The virtual methods
are virtual so that they can be overriden in tests.
On 2015/06/09 14:19:32, lazyboy wrote:
> nit: These methods are virtual so that ...

Done.

https://chromiumcodereview.appspot.com/1143333008/diff/40001/components/guest...
components/guest_view/browser/guest_view_manager.h:246: typedef
std::vector<base::Closure> Callbacks;
On 2015/06/08 20:25:56, Fady Samuel wrote:
> nit: use using instead of typedef.

Done.

https://chromiumcodereview.appspot.com/1143333008/diff/40001/extensions/brows...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://chromiumcodereview.appspot.com/1143333008/diff/40001/extensions/brows...
extensions/browser/api/web_request/web_request_api.cc:1372:
listener.extension_id,
On 2015/06/09 14:19:32, lazyboy wrote:
> This isn't equivalent to what it used to be, as I'm not too familiar
> /w web request api's listeners, I'm not too comfortable with this
> change.
> Could this potentially clear more listeners that we intended to?

As per our offline discussion, I'll leave this change in since I want to remove
all listeners with the matching embedder/view IDs, regardless of extension ID.

[lazyboy, 2015-06-09 14:50:42]

LGTM

[paulmeyer, 2015-06-09 14:52:06]

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

[paulmeyer, 2015-06-09 14:52:07]

+kalman@ for extensions/

[not at google - send to devlin, 2015-06-09 20:30:19]

rubberstamp lgtm, unless you want me to do more than rubberstamp it.

[paulmeyer, 2015-06-09 20:33:18]

On 2015/06/09 20:30:19, kalman wrote:
> rubberstamp lgtm, unless you want me to do more than rubberstamp it.

I think rubberstamp is fine in this case since lazyboy@ reviewed it all already.
Thanks.

[paulmeyer, 2015-06-09 20:33:44]

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

[paulmeyer, 2015-06-09 20:33:44]

+kenrb@ for guest_view_messages.h

[paulmeyer, 2015-06-10 12:51:17]

The CQ bit was checked by paulmeyer@chromium.org

[commit-bot: I haz the power, 2015-06-10 12:51:28]

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

[paulmeyer, 2015-06-10 12:51:59]

The CQ bit was unchecked by paulmeyer@chromium.org

[paulmeyer, 2015-06-10 12:52:19]

The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-10 12:52:31]

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

[commit-bot: I haz the power, 2015-06-10 14:12:36]

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

[commit-bot: I haz the power, 2015-06-10 14:12:40]

Dry run: This issue passed the CQ dry run.

[kenrb, 2015-06-10 20:04:09]

ipc lgtm

[paulmeyer, 2015-06-10 20:06:02]

The CQ bit was checked by paulmeyer@chromium.org

[commit-bot: I haz the power, 2015-06-10 20:06:26]

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

[commit-bot: I haz the power, 2015-06-10 20:12:04]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-06-10 20:13:10]

Patchset 3 (id:??) landed as
https://crrev.com/598af4c2aa61d865e831aeeea47d0cc126d622fe
Cr-Commit-Position: refs/heads/master@{#333782}