Bug 1534756 - Don't throw exception in ManifestObtainer if a document has no manifest r=snorp

We now return null when the manifest lacks a href or is missing.

Differential Revision: https://phabricator.services.mozilla.com/D26873

--HG--
extra : moz-landing-system : lando

dom/manifest/ManifestObtainer.jsm

@@ -37,12 +37,11 @@ var ManifestObtainer = { // jshint ignore:line
   * @return {Promise<Object>} The processed manifest.
   */
   async browserObtainManifest(aBrowser) {
-    const msgKey = "DOM:ManifestObtainer:Obtain";
     if (!isXULBrowser(aBrowser)) {
       throw new TypeError("Invalid input. Expected XUL browser.");
     }
     const mm = aBrowser.messageManager;
-    const {data: {success, result}} = await PromiseMessage.send(mm, msgKey);
+    const {data: {success, result}} = await PromiseMessage.send(mm, "DOM:ManifestObtainer:Obtain");
     if (!success) {
       const error = toError(result);
       throw error;
@@ -54,17 +53,11 @@ var ManifestObtainer = { // jshint ignore:line
    * @param  {Window} The content Window from which to extract the manifest.
    * @return {Promise<Object>} The processed manifest.
    */
-  async contentObtainManifest(aContent) {
+  contentObtainManifest(aContent) {
     if (!aContent || isXULBrowser(aContent)) {
       throw new TypeError("Invalid input. Expected a DOM Window.");
     }
-    let manifest;
-    try {
-      manifest = await fetchManifest(aContent);
-    } catch (err) {
-      throw err;
-    }
-    return manifest;
+    return fetchManifest(aContent).then(response => processResponse(response, aContent));
   },
 };
 
@@ -97,7 +90,7 @@ function isXULBrowser(aBrowser) {
  * @param {Window} aContentWindow The content window.
  * @return {Promise<Object>} The processed manifest.
  */
-const processResponse = async function(aResp, aContentWindow) {
+async function processResponse(aResp, aContentWindow) {
   const badStatus = aResp.status < 200 || aResp.status >= 300;
   if (aResp.type === "error" || badStatus) {
     const msg =
@@ -112,22 +105,22 @@ const processResponse = async function(aResp, aContentWindow) {
   };
   const manifest = ManifestProcessor.process(args);
   return manifest;
-};
+}
 
 /**
  * Asynchronously fetches a web manifest.
  * @param {Window} a The content Window from where to extract the manifest.
  * @return {Promise<Object>}
  */
-const fetchManifest = async function(aWindow) {
+async function fetchManifest(aWindow) {
   if (!aWindow || aWindow.top !== aWindow) {
-    let msg = "Window must be a top-level browsing context.";
+    const msg = "Window must be a top-level browsing context.";
     throw new Error(msg);
   }
   const elem = aWindow.document.querySelector("link[rel~='manifest']");
   if (!elem || !elem.getAttribute("href")) {
-    let msg = `No manifest to fetch at ${aWindow.location}`;
-    throw new Error(msg);
+    // There is no actual manifest to fetch, we just return null.
+    return new aWindow.Response("null");
   }
   // Throws on malformed URLs
   const manifestURL = new aWindow.URL(elem.href, elem.baseURI);
@@ -139,14 +132,8 @@ const fetchManifest = async function(aWindow) {
   }
   const request = new aWindow.Request(manifestURL, reqInit);
   request.overrideContentPolicyType(Ci.nsIContentPolicy.TYPE_WEB_MANIFEST);
-  let response;
-  try {
-    response = await aWindow.fetch(request);
-  } catch (err) {
-    throw err;
-  }
-  const manifest = await processResponse(response, aWindow);
-  return manifest;
-};
+  // Can reject...
+  return aWindow.fetch(request);
+}
 
 var EXPORTED_SYMBOLS = ["ManifestObtainer"]; // jshint ignore:line

dom/manifest/ManifestProcessor.jsm

@@ -41,6 +41,8 @@ const {ValueExtractor} = ChromeUtils.import("resource://gre/modules/ValueExtract
 // ImageObjectProcessor is used to process things like icons and images
 const {ImageObjectProcessor} = ChromeUtils.import("resource://gre/modules/ImageObjectProcessor.jsm");
 
+const domBundle = Services.strings.createBundle("chrome://global/locale/dom/dom.properties");
+
 var ManifestProcessor = { // jshint ignore:line
   get defaultDisplayMode() {
     return "browser";
@@ -60,26 +62,28 @@ var ManifestProcessor = { // jshint ignore:line
   //  * jsonText: the JSON string to be processed.
   //  * manifestURL: the URL of the manifest, to resolve URLs.
   //  * docURL: the URL of the owner doc, for security checks
-  process({
-    jsonText,
-    manifestURL: aManifestURL,
-    docURL: aDocURL,
-  }) {
-    const domBundle = Services.strings.createBundle("chrome://global/locale/dom/dom.properties");
-
+  process(aOptions) {
+    const {
+      jsonText,
+      manifestURL: aManifestURL,
+      docURL: aDocURL,
+    } = aOptions;
     const console = new ConsoleAPI({
       prefix: "Web Manifest",
     });
-    const manifestURL = new URL(aManifestURL);
-    const docURL = new URL(aDocURL);
     let rawManifest = {};
     try {
       rawManifest = JSON.parse(jsonText);
     } catch (e) {}
-    if (typeof rawManifest !== "object" || rawManifest === null) {
+    if (rawManifest === null) {
+      return null;
+    }
+    if (typeof rawManifest !== "object") {
       console.warn(domBundle.GetStringFromName("ManifestShouldBeObject"));
       rawManifest = {};
     }
+    const manifestURL = new URL(aManifestURL);
+    const docURL = new URL(aDocURL);
     const extractor = new ValueExtractor(console, domBundle);
     const imgObjProcessor = new ImageObjectProcessor(console, extractor);
     const processedManifest = {

dom/manifest/test/browser_ManifestObtainer_obtain.js

@@ -9,6 +9,18 @@ requestLongerTimeout(4);
 
 const tests = [
   // Fetch tests.
+  {
+    body: `<!-- no manifest in document -->`,
+    run(manifest) {
+      is(manifest, null, "Manifest without a href yields a null manifest.");
+    },
+  },
+  {
+    body: `<link rel="manifest">`,
+    run(manifest) {
+      is(manifest, null, "Manifest without a href yields a null manifest.");
+    },
+  },
   {
     body: `
       <link rel="manifesto" href='resource.sjs?body={"name":"fail"}'>

dom/manifest/test/test_ManifestProcessor_JSON.html

@@ -23,12 +23,22 @@ invalidJson.forEach((testString) => {
   SimpleTest.is(result.start_url, docURL.href, expected);
 });
 
-var validButUnhelpful = ["1", 1, "", "[{}]", "null"];
+var validButUnhelpful = ["1", 1, "", "[{}]"];
 validButUnhelpful.forEach((testString) => {
-  var expected = `Expect to recover from invalid JSON: ${testString}`;
+  var expected = `Expect to recover from valid JSON: ${testString}`;
   data.jsonText = testString;
   var result = processor.process(data);
   SimpleTest.is(result.start_url, docURL.href, expected);
 });
+
+// Bug 1534756 - Allow for null manifests
+var input = {
+  jsonText: "null",
+  manifestURL,
+  docURL,
+};
+var result = processor.process(input);
+SimpleTest.is(result, null, "Expected null when the input is null");
+
   </script>
 </head>

dom/manifest/test/test_ManifestProcessor_warnings.html

@@ -28,10 +28,6 @@ ConsoleAPI.prototype.warn = function(aWarning) {
     func: () => data.jsonText = JSON.stringify(1),
     warning: "Manifest should be an object.",
   },
-  {
-    func: () => data.jsonText = JSON.stringify(null),
-    warning: "Manifest should be an object.",
-  },
   {
     func: () => data.jsonText = JSON.stringify("a string"),
     warning: "Manifest should be an object.",

mobile/android/chrome/geckoview/GeckoViewContentChild.js

@@ -416,14 +416,7 @@ class GeckoViewContentChild extends GeckoViewChildModule {
         break;
       case "DOMContentLoaded": {
         content.requestIdleCallback(async () => {
-          let manifest = null;
-          try {
-            manifest = await ManifestObtainer.contentObtainManifest(content);
-          } catch (e) {
-            // Unfortunately, this throws if there is no manifest present, so we
-            // probably don't want to log anything here. Bug 1534756.
-          }
-
+          const manifest = await ManifestObtainer.contentObtainManifest(content);
           if (manifest) {
             this.eventDispatcher.sendRequest({
               type: "GeckoView:WebAppManifest",