Bug 1598258 - Add a list of additional minidumps to the event delivered when a plug-in crashes r=froydnj
authorGabriele Svelto <gsvelto@mozilla.com>
Fri, 22 Nov 2019 07:30:23 +0000
changeset 503310 86dc639426da3cabfea26fec84c1b19db7acc274
parent 503309 4082b974a1e6a4810996f7b3b56e829987da6b81
child 503311 e894fff26e422166618de4401464929e9b9cb62e
push id36833
push userbtara@mozilla.com
push dateFri, 22 Nov 2019 21:40:53 +0000
treeherdermozilla-central@2c912e46295e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1598258
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1598258 - Add a list of additional minidumps to the event delivered when a plug-in crashes r=froydnj This patch adds a new field to the `plugin-crashed` event that holds the list of additional minidumps associated with a crash report. The test infrastructure is modified to use it which also fixes a race when processing the .extra file. The reftest machinery has also been modified to take the new field into account. Differential Revision: https://phabricator.services.mozilla.com/D54107
browser/base/content/test/plugins/browser_pluginCrashCommentAndURL.js
browser/base/content/test/plugins/browser_pluginCrashReportNonDeterminism.js
dom/plugins/base/nsNPAPIPlugin.cpp
dom/plugins/base/nsNPAPIPlugin.h
dom/plugins/base/nsPluginHost.cpp
dom/plugins/base/nsPluginHost.h
dom/plugins/ipc/PluginModuleParent.cpp
dom/plugins/test/mochitest/hang_test.js
dom/plugins/test/mochitest/test_crash_notify.xul
dom/plugins/test/mochitest/test_crash_notify_no_report.xul
ipc/glue/CrashReporterHost.h
layout/tools/reftest/reftest.jsm
testing/specialpowers/content/SpecialPowersParent.jsm
--- a/browser/base/content/test/plugins/browser_pluginCrashCommentAndURL.js
+++ b/browser/base/content/test/plugins/browser_pluginCrashCommentAndURL.js
@@ -162,30 +162,41 @@ add_task(async function() {
   // have deleted the crash dump files for us.
   let crashObserver = (subject, topic, data) => {
     if (topic != "plugin-crashed") {
       return;
     }
 
     let propBag = subject.QueryInterface(Ci.nsIPropertyBag2);
     let minidumpID = propBag.getPropertyAsAString("pluginDumpID");
+    let additionalDumps = propBag.getPropertyAsACString("additionalMinidumps");
 
     Services.crashmanager.ensureCrashIsPresent(minidumpID).then(() => {
       let minidumpDir = Services.dirsvc.get("UAppData", Ci.nsIFile);
       minidumpDir.append("Crash Reports");
       minidumpDir.append("pending");
 
       let pluginDumpFile = minidumpDir.clone();
       pluginDumpFile.append(minidumpID + ".dmp");
 
       let extraFile = minidumpDir.clone();
       extraFile.append(minidumpID + ".extra");
 
       pluginDumpFile.remove(false);
       extraFile.remove(false);
+
+      if (additionalDumps.length) {
+        const names = additionalDumps.split(",");
+        for (const name of names) {
+          let additionalDumpFile = minidumpDir.clone();
+          additionalDumpFile.append(minidumpID + "-" + name + ".dmp");
+          additionalDumpFile.remove(false);
+        }
+      }
+
       crashDeferred.resolve();
     });
   };
 
   Services.obs.addObserver(crashObserver, "plugin-crashed");
 
   setTestPluginEnabledState(Ci.nsIPluginTag.STATE_ENABLED);
 
--- a/browser/base/content/test/plugins/browser_pluginCrashReportNonDeterminism.js
+++ b/browser/base/content/test/plugins/browser_pluginCrashReportNonDeterminism.js
@@ -100,32 +100,45 @@ let crashDeferred = null;
 // about them.
 let crashObserver = (subject, topic, data) => {
   if (topic != "plugin-crashed") {
     return;
   }
 
   let propBag = subject.QueryInterface(Ci.nsIPropertyBag2);
   let minidumpID = propBag.getPropertyAsAString("pluginDumpID");
+  let additionalMinidumps = propBag.getPropertyAsACString(
+    "additionalMinidumps"
+  );
 
   Services.crashmanager.ensureCrashIsPresent(minidumpID).then(() => {
     let minidumpDir = Services.dirsvc.get("ProfD", Ci.nsIFile);
     minidumpDir.append("minidumps");
 
     let pluginDumpFile = minidumpDir.clone();
     pluginDumpFile.append(minidumpID + ".dmp");
 
     let extraFile = minidumpDir.clone();
     extraFile.append(minidumpID + ".extra");
 
     ok(pluginDumpFile.exists(), "Found minidump");
     ok(extraFile.exists(), "Found extra file");
 
     pluginDumpFile.remove(false);
     extraFile.remove(false);
+
+    if (additionalMinidumps.length) {
+      const names = additionalMinidumps.split(",");
+      for (const name of names) {
+        let additionalDumpFile = minidumpDir.clone();
+        additionalDumpFile.append(minidumpID + "-" + name + ".dmp");
+        additionalDumpFile.remove(false);
+      }
+    }
+
     crashDeferred.resolve();
   });
 };
 
 Services.obs.addObserver(crashObserver, "plugin-crashed");
 // plugins.testmode will make PluginParent:Test:ClearCrashData work.
 Services.prefs.setBoolPref("plugins.testmode", true);
 registerCleanupFunction(() => {
--- a/dom/plugins/base/nsNPAPIPlugin.cpp
+++ b/dom/plugins/base/nsNPAPIPlugin.cpp
@@ -184,19 +184,20 @@ nsNPAPIPlugin::nsNPAPIPlugin() {
   mLibrary = nullptr;
 }
 
 nsNPAPIPlugin::~nsNPAPIPlugin() {
   delete mLibrary;
   mLibrary = nullptr;
 }
 
-void nsNPAPIPlugin::PluginCrashed(const nsAString& pluginDumpID) {
+void nsNPAPIPlugin::PluginCrashed(const nsAString& aPluginDumpID,
+                                  const nsACString& aAdditionalMinidumps) {
   RefPtr<nsPluginHost> host = nsPluginHost::GetInst();
-  host->PluginCrashed(this, pluginDumpID);
+  host->PluginCrashed(this, aPluginDumpID, aAdditionalMinidumps);
 }
 
 inline PluginLibrary* GetNewPluginLibrary(nsPluginTag* aPluginTag) {
   AUTO_PROFILER_LABEL("GetNewPluginLibrary", OTHER);
 
   if (!aPluginTag) {
     return nullptr;
   }
--- a/dom/plugins/base/nsNPAPIPlugin.h
+++ b/dom/plugins/base/nsNPAPIPlugin.h
@@ -37,17 +37,18 @@ class nsNPAPIPlugin final {
 
 #if defined(XP_MACOSX) && !defined(__LP64__)
   void SetPluginRefNum(short aRefNum);
 #endif
 
   // The IPC mechanism notifies the nsNPAPIPlugin if the plugin
   // crashes and is no longer usable. pluginDumpID is the ID of the minidump
   // that was written, or empty if no minidump was written.
-  void PluginCrashed(const nsAString& pluginDumpID);
+  void PluginCrashed(const nsAString& aPluginDumpID,
+                     const nsACString& aAdditionalMinidumps);
 
   nsresult Shutdown();
 
   static nsresult RetainStream(NPStream* pstream, nsISupports** aRetainedPeer);
 
  private:
   ~nsNPAPIPlugin();
 
--- a/dom/plugins/base/nsPluginHost.cpp
+++ b/dom/plugins/base/nsPluginHost.cpp
@@ -2748,17 +2748,18 @@ static void CheckForDisabledWindows() {
         }
       }
     }
   } while (haveWindows);
 }
 #endif
 
 void nsPluginHost::PluginCrashed(nsNPAPIPlugin* aPlugin,
-                                 const nsAString& pluginDumpID) {
+                                 const nsAString& aPluginDumpID,
+                                 const nsACString& aAdditionalMinidumps) {
   nsPluginTag* crashedPluginTag = TagForPlugin(aPlugin);
   MOZ_ASSERT(crashedPluginTag);
 
   // Notify the app's observer that a plugin crashed so it can submit
   // a crashreport.
   bool submittedCrashReport = false;
   nsCOMPtr<nsIObserverService> obsService =
       mozilla::services::GetObserverService();
@@ -2773,17 +2774,19 @@ void nsPluginHost::PluginCrashed(nsNPAPI
     }
     propbag->SetPropertyAsUint32(NS_LITERAL_STRING("runID"), runID);
 
     nsCString pluginName;
     crashedPluginTag->GetName(pluginName);
     propbag->SetPropertyAsAString(NS_LITERAL_STRING("pluginName"),
                                   NS_ConvertUTF8toUTF16(pluginName));
     propbag->SetPropertyAsAString(NS_LITERAL_STRING("pluginDumpID"),
-                                  pluginDumpID);
+                                  aPluginDumpID);
+    propbag->SetPropertyAsACString(NS_LITERAL_STRING("additionalMinidumps"),
+                                   aAdditionalMinidumps);
     propbag->SetPropertyAsBool(NS_LITERAL_STRING("submittedCrashReport"),
                                submittedCrashReport);
     obsService->NotifyObservers(propbag, "plugin-crashed", nullptr);
     // see if an observer submitted a crash report.
     propbag->GetPropertyAsBool(NS_LITERAL_STRING("submittedCrashReport"),
                                &submittedCrashReport);
   }
 
@@ -2794,17 +2797,17 @@ void nsPluginHost::PluginCrashed(nsNPAPI
     if (instance->GetPlugin() == aPlugin) {
       // notify the content node (nsIObjectLoadingContent) that the
       // plugin has crashed
       RefPtr<dom::Element> domElement;
       instance->GetDOMElement(getter_AddRefs(domElement));
       nsCOMPtr<nsIObjectLoadingContent> objectContent(
           do_QueryInterface(domElement));
       if (objectContent) {
-        objectContent->PluginCrashed(crashedPluginTag, pluginDumpID,
+        objectContent->PluginCrashed(crashedPluginTag, aPluginDumpID,
                                      submittedCrashReport);
       }
 
       instance->Destroy();
       mInstances.RemoveElement(instance);
       OnPluginInstanceDestroyed(crashedPluginTag);
     }
   }
--- a/dom/plugins/base/nsPluginHost.h
+++ b/dom/plugins/base/nsPluginHost.h
@@ -165,17 +165,18 @@ class nsPluginHost final : public nsIPlu
     eSpecialType_Test,
     // Informs some decisions about OOP and quirks
     eSpecialType_Flash
   };
   static SpecialType GetSpecialType(const nsACString& aMIMEType);
 
   static nsresult PostPluginUnloadEvent(PRLibrary* aLibrary);
 
-  void PluginCrashed(nsNPAPIPlugin* plugin, const nsAString& pluginDumpID);
+  void PluginCrashed(nsNPAPIPlugin* aPlugin, const nsAString& aPluginDumpID,
+                     const nsACString& aAdditionalMinidumps);
 
   nsNPAPIPluginInstance* FindInstance(const char* mimetype);
   nsNPAPIPluginInstance* FindOldestStoppedInstance();
   uint32_t StoppedInstanceCount();
 
   nsTArray<RefPtr<nsNPAPIPluginInstance>>* InstanceArray();
 
   // Return the tag for |aLibrary| if found, nullptr if not.
--- a/dom/plugins/ipc/PluginModuleParent.cpp
+++ b/dom/plugins/ipc/PluginModuleParent.cpp
@@ -1417,22 +1417,24 @@ void PluginModuleParent::NotifyPluginCra
     return;
   }
 
   if (!mPlugin) {
     return;
   }
 
   nsString dumpID;
+  nsCString additionalMinidumps;
 
   if (mCrashReporter && mCrashReporter->HasMinidump()) {
     dumpID = mCrashReporter->MinidumpID();
+    additionalMinidumps = mCrashReporter->AdditionalMinidumps();
   }
 
-  mPlugin->PluginCrashed(dumpID);
+  mPlugin->PluginCrashed(dumpID, additionalMinidumps);
 }
 
 PPluginInstanceParent* PluginModuleParent::AllocPPluginInstanceParent(
     const nsCString& aMimeType, const nsTArray<nsCString>& aNames,
     const nsTArray<nsCString>& aValues) {
   NS_ERROR("Not reachable!");
   return nullptr;
 }
--- a/dom/plugins/test/mochitest/hang_test.js
+++ b/dom/plugins/test/mochitest/hang_test.js
@@ -15,16 +15,19 @@ var testObserver = {
     is(topic, "plugin-crashed", "Checking correct topic");
     is(data, null, "Checking null data");
     ok(subject instanceof Ci.nsIPropertyBag2, "got Propbag");
     ok(subject instanceof Ci.nsIWritablePropertyBag2, "got writable Propbag");
 
     var pluginId = subject.getPropertyAsAString("pluginDumpID");
     isnot(pluginId, "", "got a non-empty plugin crash id");
 
+    var additionalDumps = subject.getPropertyAsACString("additionalMinidumps");
+    isnot(additionalDumps, "", "got a non-empty additionalDumps field");
+
     // check plugin dump and extra files
     let profD = Services.dirsvc.get("ProfD", Ci.nsIFile);
     profD.append("minidumps");
     let pluginDumpFile = profD.clone();
     pluginDumpFile.append(pluginId + ".dmp");
     ok(pluginDumpFile.exists(), "plugin minidump exists");
 
     let pluginExtraFile = profD.clone();
@@ -34,20 +37,25 @@ var testObserver = {
     let extraData = parseKeyValuePairsFromFile(pluginExtraFile);
 
     // check additional dumps
 
     ok(
       "additional_minidumps" in extraData,
       "got field for additional minidumps"
     );
-    let additionalDumps = extraData.additional_minidumps.split(",");
-    ok(additionalDumps.includes("browser"), "browser in additional_minidumps");
+    is(
+      additionalDumps,
+      extraData.additional_minidumps,
+      "the annotation matches the propbag entry"
+    );
+    let dumpNames = extraData.additional_minidumps.split(",");
+    ok(dumpNames.includes("browser"), "browser in additional_minidumps");
 
-    for (let name of additionalDumps) {
+    for (let name of dumpNames) {
       let file = profD.clone();
       file.append(pluginId + "-" + name + ".dmp");
       ok(file.exists(), "additional dump '" + name + "' exists");
     }
 
     // check cpu usage field
 
     ok("PluginCpuUsage" in extraData, "got extra field for plugin cpu usage");
--- a/dom/plugins/test/mochitest/test_crash_notify.xul
+++ b/dom/plugins/test/mochitest/test_crash_notify.xul
@@ -31,16 +31,17 @@ var testObserver = {
     ok(true, "Observer fired");
     is(topic, "plugin-crashed", "Checking correct topic");
     is(data,  null, "Checking null data");
     ok((subject instanceof Ci.nsIPropertyBag2), "got Propbag");
     ok((subject instanceof Ci.nsIWritablePropertyBag2), "got writable Propbag");
 
     var id = subject.getPropertyAsAString("pluginDumpID");
     isnot(id, "", "got a non-empty crash id");
+    ok(subject.hasKey("additionalMinidumps"), "the additionalMinidumps key is present");
     let directoryService =
       Cc["@mozilla.org/file/directory_service;1"].
       getService(Ci.nsIProperties);
     let profD = directoryService.get("ProfD", Ci.nsIFile);
     profD.append("minidumps");
     let dumpFile = profD.clone();
     dumpFile.append(id + ".dmp");
     ok(dumpFile.exists(), "minidump exists");
--- a/dom/plugins/test/mochitest/test_crash_notify_no_report.xul
+++ b/dom/plugins/test/mochitest/test_crash_notify_no_report.xul
@@ -29,16 +29,17 @@ var testObserver = {
     is(topic, "plugin-crashed", "Checking correct topic");
     is(data,  null, "Checking null data");
     ok((subject instanceof Ci.nsIPropertyBag2), "got Propbag");
     ok((subject instanceof Ci.nsIWritablePropertyBag2),
 "got writable Propbag");
 
     var id = subject.getPropertyAsAString("pluginDumpID");
     isnot(id, "", "got a non-empty crash id");
+    ok(subject.hasKey("additionalMinidumps"), "the additionalMinidumps key is present");
     let directoryService =
       Cc["@mozilla.org/file/directory_service;1"].
       getService(Ci.nsIProperties);
     let pendingD = directoryService.get("UAppData",
                                         Ci.nsIFile);
     pendingD.append("Crash Reports");
     pendingD.append("pending");
     let dumpFile = pendingD.clone();
--- a/ipc/glue/CrashReporterHost.h
+++ b/ipc/glue/CrashReporterHost.h
@@ -85,16 +85,19 @@ class CrashReporterHost {
   void AddAnnotation(CrashReporter::Annotation aKey, unsigned int aValue);
   void AddAnnotation(CrashReporter::Annotation aKey, const nsACString& aValue);
 
   bool HasMinidump() const { return !mDumpID.IsEmpty(); }
   const nsString& MinidumpID() const {
     MOZ_ASSERT(HasMinidump());
     return mDumpID;
   }
+  const nsCString& AdditionalMinidumps() const {
+    return mExtraAnnotations[CrashReporter::Annotation::additional_minidumps];
+  }
 
  private:
   static void AsyncAddCrash(int32_t aProcessType, int32_t aCrashType,
                             const nsString& aChildDumpID);
 
   // Get the nsICrashService crash type to use for an impending crash.
   int32_t GetCrashType();
 
--- a/layout/tools/reftest/reftest.jsm
+++ b/layout/tools/reftest/reftest.jsm
@@ -1619,27 +1619,37 @@ function RecvUpdateCanvasForInvalidation
 
 function RecvUpdateWholeCanvasForInvalidation()
 {
     UpdateWholeCurrentCanvasForInvalidation();
 }
 
 function OnProcessCrashed(subject, topic, data)
 {
-    var id;
-    subject = subject.QueryInterface(Ci.nsIPropertyBag2);
+    let id;
+    let additionalDumps;
+    let propbag = subject.QueryInterface(Ci.nsIPropertyBag2);
+
     if (topic == "plugin-crashed") {
-        id = subject.get("pluginDumpID");
+        id = propbag.get("pluginDumpID");
+        additionalDumps = propbag.getPropertyAsACString("additionalMinidumps");
     } else if (topic == "ipc:content-shutdown") {
-        id = subject.get("dumpID");
+        id = propbag.get("dumpID");
     }
+
     if (id) {
         g.expectedCrashDumpFiles.push(id + ".dmp");
         g.expectedCrashDumpFiles.push(id + ".extra");
     }
+
+    if (additionalDumps && additionalDumps.length != 0) {
+      for (const name of additionalDumps.split(',')) {
+        g.expectedCrashDumpFiles.push(id + "-" + name + ".dmp");
+      }
+    }
 }
 
 function RegisterProcessCrashObservers()
 {
     var os = Cc[NS_OBSERVER_SERVICE_CONTRACTID]
              .getService(Ci.nsIObserverService);
     os.addObserver(OnProcessCrashed, "plugin-crashed");
     os.addObserver(OnProcessCrashed, "ipc:content-shutdown");
--- a/testing/specialpowers/content/SpecialPowersParent.jsm
+++ b/testing/specialpowers/content/SpecialPowersParent.jsm
@@ -21,63 +21,16 @@ XPCOMUtils.defineLazyModuleGetters(this,
 });
 
 class SpecialPowersError extends Error {
   get name() {
     return "SpecialPowersError";
   }
 }
 
-function parseKeyValuePairs(text) {
-  var lines = text.split("\n");
-  var data = {};
-  for (let i = 0; i < lines.length; i++) {
-    if (lines[i] == "") {
-      continue;
-    }
-
-    // can't just .split() because the value might contain = characters
-    let eq = lines[i].indexOf("=");
-    if (eq != -1) {
-      let [key, value] = [
-        lines[i].substring(0, eq),
-        lines[i].substring(eq + 1),
-      ];
-      if (key && value) {
-        data[key] = value.replace(/\\n/g, "\n").replace(/\\\\/g, "\\");
-      }
-    }
-  }
-  return data;
-}
-
-function parseKeyValuePairsFromFile(file) {
-  var fstream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(
-    Ci.nsIFileInputStream
-  );
-  fstream.init(file, -1, 0, 0);
-  var is = Cc["@mozilla.org/intl/converter-input-stream;1"].createInstance(
-    Ci.nsIConverterInputStream
-  );
-  is.init(
-    fstream,
-    "UTF-8",
-    1024,
-    Ci.nsIConverterInputStream.DEFAULT_REPLACEMENT_CHARACTER
-  );
-  var str = {};
-  var contents = "";
-  while (is.readString(4096, str) != 0) {
-    contents += str.value;
-  }
-  is.close();
-  fstream.close();
-  return parseKeyValuePairs(contents);
-}
-
 function getTestPlugin(pluginName) {
   var ph = Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost);
   var tags = ph.getPluginTags();
   var name = pluginName || "Test Plug-in";
   for (var tag of tags) {
     if (tag.name == name) {
       return tag;
     }
@@ -275,19 +228,21 @@ class SpecialPowersParent extends JSWind
       case "plugin-crashed":
       case "ipc:content-shutdown":
         var message = { type: "crash-observed", dumpIDs: [] };
         aSubject = aSubject.QueryInterface(Ci.nsIPropertyBag2);
         if (aTopic == "plugin-crashed") {
           addDumpIDToMessage("pluginDumpID");
 
           let pluginID = aSubject.getPropertyAsAString("pluginDumpID");
-          let extra = this._getExtraData(pluginID);
-          if (extra && "additional_minidumps" in extra) {
-            let dumpNames = extra.additional_minidumps.split(",");
+          let additionalMinidumps = aSubject.getPropertyAsACString(
+            "additionalMinidumps"
+          );
+          if (additionalMinidumps.length != 0) {
+            let dumpNames = additionalMinidumps.split(",");
             for (let name of dumpNames) {
               message.dumpIDs.push({
                 id: pluginID + "-" + name,
                 extension: "dmp",
               });
             }
           }
         } else {
@@ -315,25 +270,16 @@ class SpecialPowersParent extends JSWind
     if (!this._pendingCrashDumpDir) {
       this._pendingCrashDumpDir = Services.dirsvc.get("UAppData", Ci.nsIFile);
       this._pendingCrashDumpDir.append("Crash Reports");
       this._pendingCrashDumpDir.append("pending");
     }
     return this._pendingCrashDumpDir;
   }
 
-  _getExtraData(dumpId) {
-    let extraFile = this._getCrashDumpDir().clone();
-    extraFile.append(dumpId + ".extra");
-    if (!extraFile.exists()) {
-      return null;
-    }
-    return parseKeyValuePairsFromFile(extraFile);
-  }
-
   _deleteCrashDumpFiles(aFilenames) {
     var crashDumpDir = this._getCrashDumpDir();
     if (!crashDumpDir.exists()) {
       return false;
     }
 
     var success = aFilenames.length != 0;
     aFilenames.forEach(function(crashFilename) {