Bug 1579795 - Part 1: Improve error handling when fetching the manifest r=jdescottes,fluent-reviewers,flod
☠☠ backed out by 4ad0eb343fac ☠ ☠
authorBelén Albeza <balbeza@mozilla.com>
Fri, 18 Oct 2019 13:42:32 +0000
changeset 559503 193f93f381cd99aa98a8f59cd2c83ee1195f1b87
parent 559502 7cf32690c0c04f3025ca2fc5cb3caecb5f061bf6
child 559504 eb0eaf505108a97dbf5d464187f90539ca7e2c3c
push id12177
push usercsabou@mozilla.com
push dateMon, 21 Oct 2019 14:52:16 +0000
treeherdermozilla-beta@1918a9cd33bc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdescottes, fluent-reviewers, flod
bugs1579795
milestone71.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 1579795 - Part 1: Improve error handling when fetching the manifest r=jdescottes,fluent-reviewers,flod Differential Revision: https://phabricator.services.mozilla.com/D49594
devtools/client/application/src/actions/manifest.js
devtools/client/application/src/modules/services.js
devtools/client/application/test/components/manifest/components_application_panel-ManifestLoader.test.js
devtools/client/locales/en-US/application.ftl
--- a/devtools/client/application/src/actions/manifest.js
+++ b/devtools/client/application/src/actions/manifest.js
@@ -1,30 +1,40 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
-const { services } = require("../modules/services");
+const { l10n } = require("../modules/l10n");
+
+const { services, ManifestDevToolsError } = require("../modules/services");
 const {
   FETCH_MANIFEST_FAILURE,
   FETCH_MANIFEST_START,
   FETCH_MANIFEST_SUCCESS,
   RESET_MANIFEST,
 } = require("../constants");
 
 function fetchManifest() {
   return async (dispatch, getState) => {
     dispatch({ type: FETCH_MANIFEST_START });
-    const { manifest, errorMessage } = await services.fetchManifest();
+    try {
+      const manifest = await services.fetchManifest();
+      dispatch({ type: FETCH_MANIFEST_SUCCESS, manifest });
+    } catch (error) {
+      let errorMessage = error.message;
 
-    if (!errorMessage) {
-      dispatch({ type: FETCH_MANIFEST_SUCCESS, manifest });
-    } else {
+      // since Firefox DevTools errors may not make sense for the user, swap
+      // their message for a generic one.
+      if (error instanceof ManifestDevToolsError) {
+        console.error(error);
+        errorMessage = l10n.getString("manifest-loaded-devtools-error");
+      }
+
       dispatch({ type: FETCH_MANIFEST_FAILURE, error: errorMessage });
     }
   };
 }
 
 function resetManifest() {
   return { type: RESET_MANIFEST };
 }
--- a/devtools/client/application/src/modules/services.js
+++ b/devtools/client/application/src/modules/services.js
@@ -1,34 +1,58 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
+class ManifestDevToolsError extends Error {
+  constructor(...params) {
+    super(...params);
+
+    this.name = "ManifestDevToolsError";
+  }
+}
+
 class Services {
   init(toolbox) {
     this._toolbox = toolbox;
   }
 
   selectTool(toolId) {
     this._assertInit();
     return this._toolbox.selectTool(toolId);
   }
 
   async fetchManifest() {
-    this._assertInit();
+    let response;
 
-    const manifestFront = await this._toolbox.target.getFront("manifest");
-    const response = await manifestFront.fetchCanonicalManifest();
+    try {
+      this._assertInit();
+      const manifestFront = await this._toolbox.target.getFront("manifest");
+      response = await manifestFront.fetchCanonicalManifest();
+    } catch (error) {
+      throw new ManifestDevToolsError(
+        error.message,
+        error.fileName,
+        error.lineNumber
+      );
+    }
 
-    return response;
+    if (response.errorMessage) {
+      throw new Error(response.errorMessage);
+    }
+
+    return response.manifest;
   }
 
   _assertInit() {
     if (!this._toolbox) {
       throw new Error("Services singleton has not been initialized");
     }
   }
 }
 
-// exports a singleton, which will be used across all application panel modules
-exports.services = new Services();
+module.exports = {
+  ManifestDevToolsError,
+  // exports a singleton, which will be used across all application panel modules
+  services: new Services(),
+};
--- a/devtools/client/application/test/components/manifest/components_application_panel-ManifestLoader.test.js
+++ b/devtools/client/application/test/components/manifest/components_application_panel-ManifestLoader.test.js
@@ -13,16 +13,17 @@ const {
 } = require("devtools/client/application/test/components/helpers/helpers");
 // Import fixtures
 const {
   MANIFEST_NO_ISSUES,
 } = require("devtools/client/application/test/components/fixtures/data/constants");
 
 // Import app modules
 const {
+  ManifestDevToolsError,
   services,
 } = require("devtools/client/application/src/modules/services");
 
 const {
   FETCH_MANIFEST_FAILURE,
   FETCH_MANIFEST_START,
   FETCH_MANIFEST_SUCCESS,
 } = require("devtools/client/application/src/constants");
@@ -47,17 +48,17 @@ describe("ManifestLoader", () => {
     );
 
     return setupStore({ manifest: manifestState });
   }
 
   it("loads a manifest when mounted and triggers actions when loading is OK", async () => {
     const fetchManifestSpy = jest
       .spyOn(services, "fetchManifest")
-      .mockResolvedValue({ manifest: MANIFEST_NO_ISSUES, errorMessage: "" });
+      .mockResolvedValue(MANIFEST_NO_ISSUES);
 
     const store = buildStore({});
 
     shallow(ManifestLoader({ store })).dive();
     await flushPromises();
 
     expect(store.getActions()).toEqual([
       { type: FETCH_MANIFEST_START },
@@ -65,31 +66,55 @@ describe("ManifestLoader", () => {
     ]);
 
     fetchManifestSpy.mockRestore();
   });
 
   it("loads a manifest when mounted and triggers actions when loading fails", async () => {
     const fetchManifestSpy = jest
       .spyOn(services, "fetchManifest")
-      .mockResolvedValue({ manifest: null, errorMessage: "lorem ipsum" });
+      .mockRejectedValue(new Error("lorem ipsum"));
 
     const store = buildStore({});
 
     shallow(ManifestLoader({ store })).dive();
     await flushPromises();
 
     expect(store.getActions()).toEqual([
       { type: FETCH_MANIFEST_START },
       { type: FETCH_MANIFEST_FAILURE, error: "lorem ipsum" },
     ]);
 
     fetchManifestSpy.mockRestore();
   });
 
+  it("loads a manifest when mounted and triggers actions when loading fails due to devtools", async () => {
+    const error = new ManifestDevToolsError(":(");
+    const fetchManifestSpy = jest
+      .spyOn(services, "fetchManifest")
+      .mockRejectedValue(error);
+    const consoleErrorSpy = jest
+      .spyOn(console, "error")
+      .mockImplementation(() => {});
+
+    const store = buildStore({});
+
+    shallow(ManifestLoader({ store })).dive();
+    await flushPromises();
+
+    expect(store.getActions()).toEqual([
+      { type: FETCH_MANIFEST_START },
+      { type: FETCH_MANIFEST_FAILURE, error: "manifest-loaded-devtools-error" },
+    ]);
+    expect(consoleErrorSpy).toHaveBeenCalledWith(error);
+
+    fetchManifestSpy.mockRestore();
+    consoleErrorSpy.mockRestore();
+  });
+
   it("renders a message when it is loading", async () => {
     const store = buildStore({ isLoading: true });
     const wrapper = shallow(ManifestLoader({ store })).dive();
     expect(wrapper).toMatchSnapshot();
   });
 
   it("renders a message when manifest has loaded OK", async () => {
     const store = buildStore({
--- a/devtools/client/locales/en-US/application.ftl
+++ b/devtools/client/locales/en-US/application.ftl
@@ -100,19 +100,24 @@ manifest-item-presentation = Presentatio
 manifest-item-icons = Icons
 
 # Text displayed while we are loading the manifest file
 manifest-loading = Loading manifest…
 
 # Text displayed when the manifest has been successfully loaded
 manifest-loaded-ok = Manifest loaded.
 
-# Text displayed when there has been an error while trying to load the manifest
+# Text displayed as a caption when there has been an error while trying to
+# load the manifest
 manifest-loaded-error = There was an error while loading the manifest:
 
+# Text displayed as an error when there has been a Firefox DevTools error while
+# trying to load the manifest
+manifest-loaded-devtools-error = Firefox DevTools error
+
 # Text displayed when the page has no manifest available
 manifest-non-existing = No manifest found to inspect.
 
 # Text displayed when the page has a manifest embedded in a Data URL and
 # thus we cannot link to it.
 manifest-json-link-data-url = The manifest is embedded in a Data URL.
 
 # Text displayed at manifest icons to label their purpose, as declared