Bug 1151597 - Step 1: Change memory reporting IPC to send one report per message. r=erahm

Also fixes bug 1005154 -- since there's now a method for "end of report",
we might as well call it from ActorDestroy instead of Recv__delete__.

--HG--
extra : rebase_source : 89f467fbc553a1a3a4d9b144fff747fa3447f21b

dom/ipc/ContentChild.cpp

@@ -871,43 +871,32 @@ ContentChild::AllocPMemoryReportRequestChild(const uint32_t& aGeneration,
     return actor;
 }
 
-// This is just a wrapper for InfallibleTArray<MemoryReport> that implements
-// nsISupports, so it can be passed to nsIMemoryReporter::CollectReports.
-class MemoryReportsWrapper final : public nsISupports {
-    ~MemoryReportsWrapper() {}
-public:
-    NS_DECL_ISUPPORTS
-    explicit MemoryReportsWrapper(InfallibleTArray<MemoryReport>* r) : mReports(r) { }
-    InfallibleTArray<MemoryReport> *mReports;
-};
-NS_IMPL_ISUPPORTS0(MemoryReportsWrapper)
-
 class MemoryReportCallback final : public nsIMemoryReporterCallback
 {
 public:
     NS_DECL_ISUPPORTS
 
-    explicit MemoryReportCallback(const nsACString& aProcess)
-    : mProcess(aProcess)
+    explicit MemoryReportCallback(MemoryReportRequestChild* aActor,
+                                  const nsACString& aProcess)
+    : mActor(aActor)
+    , mProcess(aProcess)
     {
     }
 
     NS_IMETHOD Callback(const nsACString& aProcess, const nsACString &aPath,
                         int32_t aKind, int32_t aUnits, int64_t aAmount,
                         const nsACString& aDescription,
-                        nsISupports* aiWrappedReports) override
+                        nsISupports* aUnused) override
     {
-        MemoryReportsWrapper *wrappedReports =
-            static_cast<MemoryReportsWrapper *>(aiWrappedReports);
-
         MemoryReport memreport(mProcess, nsCString(aPath), aKind, aUnits,
                                aAmount, nsCString(aDescription));
-        wrappedReports->mReports->AppendElement(memreport);
+        mActor->SendReport(memreport);
         return NS_OK;
     }
 private:
     ~MemoryReportCallback() {}
 
+    nsRefPtr<MemoryReportRequestChild> mActor;
     const nsCString mProcess;
 };
 NS_IMPL_ISUPPORTS(
@@ -943,21 +932,18 @@ NS_IMETHODIMP MemoryReportRequestChild::Run()
     ContentChild *child = static_cast<ContentChild*>(Manager());
     nsCOMPtr<nsIMemoryReporterManager> mgr = do_GetService("@mozilla.org/memory-reporter-manager;1");
 
-    InfallibleTArray<MemoryReport> reports;
-
     nsCString process;
     child->GetProcessName(process);
     child->AppendProcessId(process);
 
     // Run the reporters.  The callback will turn each measurement into a
     // MemoryReport.
-    nsRefPtr<MemoryReportsWrapper> wrappedReports =
-        new MemoryReportsWrapper(&reports);
-    nsRefPtr<MemoryReportCallback> cb = new MemoryReportCallback(process);
-    mgr->GetReportsForThisProcessExtended(cb, wrappedReports, mAnonymize,
+    nsRefPtr<MemoryReportCallback> cb =
+        new MemoryReportCallback(this, process);
+    mgr->GetReportsForThisProcessExtended(cb, nullptr, mAnonymize,
                                           FileDescriptorToFILE(mDMDFile, "wb"));
 
-    bool sent = Send__delete__(this, reports);
+    bool sent = Send__delete__(this);
     return sent ? NS_OK : NS_ERROR_FAILURE;
 }
 

dom/ipc/ContentParent.cpp

@@ -411,10 +411,13 @@ public:
 
     virtual void ActorDestroy(ActorDestroyReason aWhy) override;
 
-    virtual bool Recv__delete__(InfallibleTArray<MemoryReport>&& aReport) override;
+    virtual bool RecvReport(const MemoryReport& aReport) override;
+    virtual bool Recv__delete__() override;
 
 private:
     const uint32_t mGeneration;
+    // Non-null if we haven't yet called EndChildReport() on it.
+    nsRefPtr<nsMemoryReporterManager> mReporterManager;
 
     ContentParent* Owner()
     {
@@ -426,27 +429,41 @@ MemoryReportRequestParent::MemoryReportRequestParent(uint32_t aGeneration)
     : mGeneration(aGeneration)
 {
     MOZ_COUNT_CTOR(MemoryReportRequestParent);
+    mReporterManager = nsMemoryReporterManager::GetOrCreate();
+    NS_WARN_IF(!mReporterManager);
+}
+
+bool
+MemoryReportRequestParent::RecvReport(const MemoryReport& aReport)
+{
+    if (mReporterManager) {
+        mReporterManager->HandleChildReport(mGeneration, aReport);
+    }
+    return true;
+}
+
+bool
+MemoryReportRequestParent::Recv__delete__()
+{
+    // Notifying the reporter manager is done in ActorDestroy, because
+    // it needs to happen even if the child process exits mid-report.
+    // (The reporter manager will time out eventually, but let's avoid
+    // that if possible.)
+    return true;
 }
 
 void
 MemoryReportRequestParent::ActorDestroy(ActorDestroyReason aWhy)
 {
-  // Implement me! Bug 1005154
+    if (mReporterManager) {
+        mReporterManager->EndChildReport(mGeneration, aWhy == Deletion);
+        mReporterManager = nullptr;
     }
-
-bool
-MemoryReportRequestParent::Recv__delete__(nsTArray<MemoryReport>&& childReports)
-{
-    nsRefPtr<nsMemoryReporterManager> mgr =
-        nsMemoryReporterManager::GetOrCreate();
-    if (mgr) {
-        mgr->HandleChildReports(mGeneration, childReports);
-    }
-    return true;
 }
 
 MemoryReportRequestParent::~MemoryReportRequestParent()
 {
+    MOZ_ASSERT(!mReporterManager);
     MOZ_COUNT_DTOR(MemoryReportRequestParent);
 }
 

dom/ipc/PMemoryReportRequest.ipdl

@@ -21,7 +21,8 @@ protocol PMemoryReportRequest {
   manager PContent;
 
 parent:
-  __delete__(MemoryReport[] report);
+  Report(MemoryReport aReport);
+  __delete__();
 };
 
 }

xpcom/base/nsMemoryReporterManager.cpp

@@ -1486,18 +1486,11 @@ nsMemoryReporterManager::GetReportsForThisProcessExtended(
   return NS_OK;
 }
 
-// This function has no return value.  If something goes wrong, there's no
-// clear place to report the problem to, but that's ok -- we will end up
-// hitting the timeout and executing TimeoutCallback().
-void
-nsMemoryReporterManager::HandleChildReports(
-  const uint32_t& aGeneration,
-  const InfallibleTArray<dom::MemoryReport>& aChildReports)
+nsMemoryReporterManager::GetReportsState*
+nsMemoryReporterManager::GetStateForGeneration(uint32_t aGeneration)
 {
   // Memory reporting only happens on the main thread.
-  if (!NS_IsMainThread()) {
-    MOZ_CRASH();
-  }
+  MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
   GetReportsState* s = mGetReportsState;
 
@@ -1516,7 +1509,7 @@ nsMemoryReporterManager::HandleChildReports(
     MEMORY_REPORTING_LOG(
       "HandleChildReports: no request in flight (aGen=%u)\n",
       aGeneration);
-    return;
+    return nullptr;
   }
 
   if (aGeneration != s->mGeneration) {
@@ -1527,29 +1520,61 @@ nsMemoryReporterManager::HandleChildReports(
     MEMORY_REPORTING_LOG(
       "HandleChildReports: gen mismatch (aGen=%u, s->gen=%u)\n",
       aGeneration, s->mGeneration);
+    return nullptr;
+  }
+
+  return s;
+}
+
+// This function has no return value.  If something goes wrong, there's no
+// clear place to report the problem to, but that's ok -- we will end up
+// hitting the timeout and executing TimeoutCallback().
+void
+nsMemoryReporterManager::HandleChildReport(
+  uint32_t aGeneration,
+  const dom::MemoryReport& aChildReport)
+{
+  GetReportsState* s = GetStateForGeneration(aGeneration);
+  if (!s) {
     return;
   }
 
-  // Process the reports from the child process.
-  for (uint32_t i = 0; i < aChildReports.Length(); i++) {
-    const dom::MemoryReport& r = aChildReports[i];
-
   // Child reports should have a non-empty process.
-    MOZ_ASSERT(!r.process().IsEmpty());
+  MOZ_ASSERT(!aChildReport.process().IsEmpty());
 
   // If the call fails, ignore and continue.
-    s->mHandleReport->Callback(r.process(), r.path(), r.kind(),
-                               r.units(), r.amount(), r.desc(),
+  s->mHandleReport->Callback(aChildReport.process(),
+                             aChildReport.path(),
+                             aChildReport.kind(),
+                             aChildReport.units(),
+                             aChildReport.amount(),
+                             aChildReport.desc(),
                              s->mHandleReportData);
 }
 
+void
+nsMemoryReporterManager::EndChildReport(uint32_t aGeneration, bool aSuccess)
+{
+  GetReportsState* s = GetStateForGeneration(aGeneration);
+  if (!s) {
+    return;
+  }
+
+  s->mNumChildProcessesCompleted++;
+
+  if (aSuccess) {
+    MEMORY_REPORTING_LOG("HandleChildReports (aGen=%u): completed child %d\n",
+                         aGeneration, s->mNumChildProcessesCompleted);
+  } else {
+    // Unfortunately, there's no way to indicate this in the report yet.
+    // (Also, we don't have the child's identifier at this point.)
+    MEMORY_REPORTING_LOG("HandleChildReports (aGen=%u): child %d exited"
+                         " during report\n",
+                         aGeneration, s->mNumChildProcessesCompleted);
+  }
+
   // If all the child processes have reported, we can cancel the timer and
   // finish up.  Otherwise, just return.
-
-  s->mNumChildProcessesCompleted++;
-  MEMORY_REPORTING_LOG("HandleChildReports (aGen=%u): completed child %d\n",
-                       aGeneration, s->mNumChildProcessesCompleted);
-
   if (s->mNumChildProcessesCompleted >= s->mNumChildProcesses &&
       s->mParentDone) {
     s->mTimer->Cancel();

xpcom/base/nsMemoryReporterManager.h

@@ -54,8 +54,11 @@ public:
   //   (mGetReportsState) for when the child processes report back, including a
   //   timer.  Control then returns to the main event loop.
   //
-  // - HandleChildReports() is called (asynchronously) once per child process
-  //   that reports back.  If all child processes report back before time-out,
+  // - HandleChildReport() is called (asynchronously) once per child process
+  //   reporter callback.
+  //
+  // - EndChildReport() is called (asynchronously) once per child process that
+  //   finishes reporting back.  If all child processes do so before time-out,
   //   the timer is cancelled.  (The number of child processes is part of the
   //   saved request state.)
   //
@@ -63,7 +66,7 @@ public:
   //   don't respond within the time threshold.
   //
   // - FinishReporting() finishes things off.  It is *always* called -- either
-  //   from HandleChildReports() (if all child processes have reported back) or
+  //   from EndChildReport() (if all child processes have reported back) or
   //   from TimeoutCallback() (if time-out occurs).
   //
   // All operations occur on the main thread.
@@ -95,6 +98,11 @@ public:
   //   is reported, because there's nothing sensible to be done about it at
   //   this late stage.
   //
+  // - If the time-out occurs after a child process has sent some reports but
+  //   before it has signaled completion (see bug 1151597), then what it
+  //   successfully sent will be included, with no explicit indication that it
+  //   is incomplete.
+  //
   // Now, what what happens if a child process is created/destroyed in the
   // middle of a request?  Well, GetReportsState contains a copy of
   // mNumChildProcesses which it uses to determine finished-ness.  So...
@@ -103,24 +111,23 @@ public:
   //   and the GetReportsState's mNumChildProcesses won't account for it.  So
   //   the reported data will reflect how things were when the request began.
   //
-  // - If a process is destroyed before reporting back, we'll just hit the
-  //   time-out, because we'll have received reports (barring other errors)
-  //   from N-1 child process.  So the reported data will reflect how things
-  //   are when the request ends.
+  // - If a process is destroyed before it starts reporting back, the reported
+  //   data will reflect how things are when the request ends.
+  //
+  // - If a process is destroyed after it starts reporting back but before it
+  //   finishes, the reported data will contain a partial report for it.
   //
   // - If a process is destroyed after reporting back, but before all other
   //   child processes have reported back, it will be included in the reported
   //   data.  So the reported data will reflect how things were when the
   //   request began.
   //
-  // The inconsistencies between these three cases are unfortunate but
-  // difficult to avoid.  It's enough of an edge case to not be worth doing
-  // more.
+  // The inconsistencies between these cases are unfortunate but difficult to
+  // avoid.  It's enough of an edge case to not be worth doing more.
   //
-  void HandleChildReports(
-    const uint32_t& aGeneration,
-    const InfallibleTArray<mozilla::dom::MemoryReport>& aChildReports);
-  nsresult FinishReporting();
+  void HandleChildReport(uint32_t aGeneration,
+                         const mozilla::dom::MemoryReport& aChildReport);
+  void EndChildReport(uint32_t aGeneration, bool aSuccess);
 
   // Functions that (a) implement distinguished amounts, and (b) are outside of
   // this module.
@@ -175,6 +182,7 @@ private:
   nsresult RegisterReporterHelper(nsIMemoryReporter* aReporter,
                                   bool aForce, bool aStrongRef);
   nsresult StartGettingReports();
+  nsresult FinishReporting();
 
   static void TimeoutCallback(nsITimer* aTimer, void* aData);
   // Note: this timeout needs to be long enough to allow for the
@@ -234,6 +242,8 @@ private:
   // new/delete for this because its lifetime doesn't match block scope or
   // anything like that.
   GetReportsState* mGetReportsState;
+
+  GetReportsState* GetStateForGeneration(uint32_t aGeneration);
 };
 
 #define NS_MEMORY_REPORTER_MANAGER_CID \