Convert renderer JS memory usage reporting to use Mojo.

chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc

Index: chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc
diff --git a/chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc b/chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc
index abed85c140e5ad6026f0a663998565808277b96f..60bc147bb9cfd8d704a79925e526b8ab373cfaa0 100644
--- a/chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc
+++ b/chrome/browser/ui/webui/memory_internals/memory_internals_proxy.cc
@@ -4,11 +4,15 @@
 
 #include "chrome/browser/ui/webui/memory_internals/memory_internals_proxy.h"
 
+#include <map>
 #include <set>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/bind.h"
+#include "base/memory/linked_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
 #include "base/sys_info.h"
 #include "base/values.h"
@@ -17,6 +21,7 @@
 #include "chrome/browser/memory_details.h"
 #include "chrome/browser/prerender/prerender_manager.h"
 #include "chrome/browser/prerender/prerender_manager_factory.h"
+#include "chrome/browser/process_resource_usage.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/renderer_host/chrome_render_message_filter.h"
@@ -29,13 +34,11 @@
 #include "chrome/common/render_messages.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
-#include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
+#include "content/public/common/service_registry.h"
 
 #if defined(ENABLE_PRINT_PREVIEW)
 #include "chrome/browser/printing/background_printing_manager.h"
@@ -118,37 +121,52 @@ void GetAllWebContents(std::set<content::WebContents*>* web_contents) {
 
 }  // namespace
 
-class RendererDetails : public content::NotificationObserver {
+class RendererDetails {
  public:
   typedef base::Callback<void(const base::ProcessId pid,
                               const size_t v8_allocated,
                               const size_t v8_used)> V8DataCallback;
 
   explicit RendererDetails(const V8DataCallback& callback)
-      : callback_(callback) {
-    registrar_.Add(this, chrome::NOTIFICATION_RENDERER_V8_HEAP_STATS_COMPUTED,
-                   content::NotificationService::AllSources());
-  }
-  ~RendererDetails() override {}
+      : callback_(callback), weak_factory_(this) {}
+  ~RendererDetails() {}
 
   void Request() {
     for (std::set<content::WebContents*>::iterator iter = web_contents_.begin();
-         iter != web_contents_.end(); ++iter)
-      (*iter)->GetRenderViewHost()->Send(new ChromeViewMsg_GetV8HeapStats);
+         iter != web_contents_.end(); ++iter) {
+      auto rph = (*iter)->GetRenderViewHost()->GetProcess();
+      auto resource_usage = resource_usage_reporters_[(*iter)];
+      DCHECK(resource_usage.get());
+      resource_usage->Refresh(base::Bind(
+          &RendererDetails::OnRefreshDone, weak_factory_.GetWeakPtr(),
+          *iter, base::GetProcId(rph->GetHandle())));
+    }
   }
 
   void AddWebContents(content::WebContents* content) {
     web_contents_.insert(content);
+
+    auto rph = content->GetRenderViewHost()->GetProcess();
+    content::ServiceRegistry* service_registry = rph->GetServiceRegistry();
+    ResourceUsageReporterPtr service;
+    if (service_registry)
+      service_registry->ConnectToRemoteService(&service);
+    resource_usage_reporters_.insert(std::make_pair(
+        content, make_linked_ptr(new ProcessResourceUsage(service.Pass()))));
+    DCHECK_EQ(web_contents_.size(), resource_usage_reporters_.size());
   }
 
   void Clear() {
     web_contents_.clear();
+    resource_usage_reporters_.clear();

[ncarter (slow), 2015/05/14 06:28:12]

Invalidating the weak pointers here seems like a good idea.

[Anand Mistry (off Chromium), 2015/05/14 07:30:41]

Done.

   }
 
   void RemoveWebContents(base::ProcessId) {
     // We don't have to detect which content is the caller of this method.
     if (!web_contents_.empty())
       web_contents_.erase(web_contents_.begin());
+    if (web_contents_.empty())
+      resource_usage_reporters_.clear();
   }
 
   int IsClean() {
@@ -156,23 +174,19 @@ class RendererDetails : public content::NotificationObserver {
   }
 
  private:
-  // NotificationObserver:
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override {
-    const base::ProcessId* pid =
-        content::Source<const base::ProcessId>(source).ptr();
-    const ChromeRenderMessageFilter::V8HeapStatsDetails* v8_heap =
-        content::Details<const ChromeRenderMessageFilter::V8HeapStatsDetails>(
-            details).ptr();
-    callback_.Run(*pid,
-                  v8_heap->v8_memory_allocated(),
-                  v8_heap->v8_memory_used());
+  void OnRefreshDone(content::WebContents* content, const base::ProcessId pid) {
+    auto iter = resource_usage_reporters_.find(content);
+    DCHECK(iter != resource_usage_reporters_.end());

[ncarter (slow), 2015/05/14 06:28:12]

This code was pretty dicey to begin with, so we can wind up in this callback in
some pretty weird situations:

1. It's possible |content| is not a valid WC pointer when the refresh completes
-- the tab could have been closed.
2. The resource_usage_reporters_ map might have been cleared between Refresh and
now -- the user could have hit the button again.
3. Because of [2] it's possible that we get back here with an entry in
resource_usage_reporters_[content] that is a different object from the one
currently invoking the callback.

Although it sounds like a pretty quick race that would be hard to lose, the v8
stats will actually return after an arbitrary delay if we're executing an
expensive js function. I just confirmed this in the console using "for (var iii
= 0; iii < 3000000; iii++) {}" in one tab while trying to collecting
memory-internals in another.

The old code seemed to somehow be immune to crashes because, although it
maintained a set of WebContents pointers, it didn't ever dereference them
anytime after the set was first built. Request() happens during the same message
loop task as all the AddWebContents calls.

Some possible ways forward (short of totally reworking this class, which I'd be
okay with too):
 - Don't retain any WebContents pointers after Request().
 - Have IsClean() be weak_factory_.HasWeakPtrs()
 - Have Clear() do weak_factory_.InvalidateWeakPtrs()
 - Probably kill off |web_contents_| altogether.
 - Bind the ProcessResourceUsage* to the OnRefreshDone callback.
 - Possibly -- if it's not too weird -- the ProcessResourceUsage objects can be
bound as Owned() or scoped_ptr<> instead of keeping them in a ScopedVector or
whatever resource_usage_reporters_ becomes.

Not particularly in love with that last one, but I do think it would work.

[Anand Mistry (off Chromium), 2015/05/14 07:30:41]

I've considered this, which is why |content| is only used as a key. But the
assumption is that Clear() hasn't been called and the usage reporter hasn't been
cleared. Invalidating the weak_ptr in Clear() removes this possibility.
Invalidating the weak_ptr fixes this.

[ncarter (slow), 2015/05/14 13:59:58]

OK. I'll try to come back here myself and do something to remove the
|web_contents| pointers.

Since you've fixed 487927, make sure you mention it in the CL.

[Anand Mistry (off Chromium), 2015/05/19 04:28:37]

So, I've done some more testing and it turns out I haven't fixed this bug.
Although it's logically fixed, there's an implementation bug in how Mojo
interacts with render processes. Since this is part of the fix, I'll leave the
bug line. But the bug won't be fixed. Not until I fix the core issue. That said,
memory-internals won't hang forever. If you close the crashed tab, it recovers.

+    linked_ptr<ProcessResourceUsage> usage = iter->second;
+    callback_.Run(pid, usage->GetV8MemoryAllocated(), usage->GetV8MemoryUsed());
   }
 
   V8DataCallback callback_;
-  content::NotificationRegistrar registrar_;
   std::set<content::WebContents*> web_contents_;  // This class does not own
+  std::map<content::WebContents*, linked_ptr<ProcessResourceUsage>>
+      resource_usage_reporters_;
+
+  base::WeakPtrFactory<RendererDetails> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RendererDetails);
 };