Getting rid of more webview memory leaks.

components/guest_view/browser/guest_view_manager.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_GUEST_VIEW_BROWSER_GUEST_VIEW_MANAGER_H_
 #define COMPONENTS_GUEST_VIEW_BROWSER_GUEST_VIEW_MANAGER_H_
 
 #include <map>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/gtest_prod_util.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "content/public/browser/browser_plugin_guest_manager.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
 
 class GURL;
@@ skipping 63 matching lines @@
   template <typename T>
   void RegisterGuestViewType() {
     // If the GuestView type |T| is already registered, then there is nothing
     // more to do. If an existing entry in the registry was created by this
     // function for type |T|, then registering again would have no effect, and
     // if it was registered elsewhere, then we do not want to overwrite it. Note
     // that it is possible for tests to have special test factory methods
     // registered here.
     if (guest_view_registry_.count(T::Type))
       return;
-    guest_view_registry_[T::Type] = base::Bind(&T::Create);
+    auto new_entry = std::make_pair(T::Type, GuestViewData(

[lazyboy, 2015/06/05 23:09:55]

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

[paulmeyer, 2015/06/08 17:53:58]

Done.

+        base::Bind(&T::Create), base::Bind(&T::CleanUp)));
+    guest_view_registry_.insert(new_entry);
   }
 
+  // Registers a callback to be called when the view identified by
+  // |embedder_process_id| and |view_instance_id| is destroyed.
+  void RegisterViewCallback(int embedder_process_id,

[lazyboy, 2015/06/05 23:09:55]

RegisterViewDestructionCallback

[paulmeyer, 2015/06/08 17:53:58]

Done.

+                            int view_instance_id,
+                            const base::Closure& callback);
+
   using WebContentsCreatedCallback =
       base::Callback<void(content::WebContents*)>;
   void CreateGuest(const std::string& view_type,
                    content::WebContents* owner_web_contents,
                    const base::DictionaryValue& create_params,
                    const WebContentsCreatedCallback& callback);
 
   content::WebContents* CreateGuestWithWebContentsParams(
       const std::string& view_type,
       content::WebContents* owner_web_contents,
       const content::WebContents::CreateParams& create_params);
 
   content::SiteInstance* GetGuestSiteInstance(
       const GURL& guest_site);
 
   // BrowserPluginGuestManager implementation.
   content::WebContents* GetGuestByInstanceID(
       int owner_process_id,
       int element_instance_id) override;
   bool ForEachGuest(content::WebContents* owner_web_contents,
                     const GuestCallback& callback) override;
   content::WebContents* GetFullPageGuest(
       content::WebContents* embedder_web_contents) override;
 
  protected:
   friend class GuestViewBase;
   friend class GuestViewEvent;
   friend class GuestViewMessageFilter;
 
-  // Can be overriden in tests.
+  // The virtual methods are virtual so that they can be overriden in tests.
+
   virtual void AddGuest(int guest_instance_id,
                         content::WebContents* guest_web_contents);
   virtual void RemoveGuest(int guest_instance_id);
+
+  // Called when a GuestView has been created in JavaScript.
+  virtual void ViewCreated(int embedder_process_id,
+                           int view_instance_id,
+                           const std::string& view_type);
+
+  // Called when a GuestView has been garbage collected in JavaScript.
   virtual void ViewGarbageCollected(int embedder_process_id,
-                                    int view_instance_id) {}
+                                    int view_instance_id);
 
   // Creates a guest of the provided |view_type|.
   GuestViewBase* CreateGuestInternal(content::WebContents* owner_web_contents,
                                      const std::string& view_type);
 
   // Adds GuestView types to the GuestView registry.
   void RegisterGuestViewTypes();
 
   // Indicates whether the provided |guest| can be used in the context it has
   // been created.
   bool IsGuestAvailableToContext(GuestViewBase* guest);
 
   // Dispatches the event with |name| with the provided |args| to the embedder
   // of the given |guest| with |instance_id| for routing.
   void DispatchEvent(const std::string& event_name,
                      scoped_ptr<base::DictionaryValue> args,
                      GuestViewBase* guest,
                      int instance_id);
 
+  // This method is called when the embedder with ID |embedder_process_id| is
+  // about to be destroyed.
+  void EmbedderWillBeDestroyed(int embedder_process_id);
+
   content::WebContents* GetGuestByInstanceID(int guest_instance_id);
 
   bool CanEmbedderAccessInstanceIDMaybeKill(
       int embedder_render_process_id,
       int guest_instance_id);
 
   bool CanEmbedderAccessInstanceID(int embedder_render_process_id,
                                    int guest_instance_id);
 
   // Returns true if |guest_instance_id| can be used to add a new guest to this
@@ skipping 24 matching lines @@
     bool operator==(const ElementInstanceKey& other) const;
   };
 
   using GuestInstanceIDMap = std::map<ElementInstanceKey, int>;
   GuestInstanceIDMap instance_id_map_;
 
   // The reverse map of GuestInstanceIDMap.
   using GuestInstanceIDReverseMap = std::map<int, ElementInstanceKey>;
   GuestInstanceIDReverseMap reverse_instance_id_map_;
 
-  using GuestCreationCallback =
+  using GuestViewCreateFunction =
       base::Callback<GuestViewBase*(content::WebContents*)>;
-  using GuestViewCreationMap =
-      std::map<std::string, GuestViewManager::GuestCreationCallback>;
-  GuestViewCreationMap guest_view_registry_;
+  using GuestViewCleanUpFunction = base::Callback<void(int, int)>;
+  struct GuestViewData {
+    GuestViewData(const GuestViewCreateFunction& create,
+                  const GuestViewCleanUpFunction& clean_up);
+    ~GuestViewData();
+    const GuestViewCreateFunction create;

[lazyboy, 2015/06/05 23:09:55]

s/create/create_function
s/clean_up/cleanup_function

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

Done.

+    const GuestViewCleanUpFunction clean_up;
+  };
+  using GuestViewMethodMap = std::map<std::string, GuestViewData>;
+  GuestViewMethodMap guest_view_registry_;
 
   int current_instance_id_;
 
   // Any instance ID whose number not greater than this was removed via
   // RemoveGuest.
   // This is used so that we don't have store all removed instance IDs in
   // |removed_instance_ids_|.
   int last_instance_id_removed_;
   // The remaining instance IDs that are greater than
   // |last_instance_id_removed_| are kept here.
   std::set<int> removed_instance_ids_;
 
   content::BrowserContext* context_;
 
   scoped_ptr<GuestViewManagerDelegate> delegate_;
 
+  // |view_callback_map_| maps from embedder process ID to view ID to a
+  // vector of callback functions to be called when that view is destroyed.
+  std::map<int, std::map<int, std::vector<base::Closure>>> view_callback_map_;

[lazyboy, 2015/06/05 23:09:55]

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.

[paulmeyer, 2015/06/08 17:53:58]

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

+
   DISALLOW_COPY_AND_ASSIGN(GuestViewManager);
 };
 
 }  // namespace guest_view
 
 #endif  // COMPONETS_GUEST_VIEW_BROWSER_GUEST_VIEW_MANAGER_H_