Bug 1519762 - Fix a regression with browser.downloads.search(). r=kmag, a=RyanVM
authorAndrew Swan <aswan@mozilla.com>
Thu, 17 Jan 2019 11:12:46 -0800
changeset 509553 32f1e101f51434bb7419d30e94b4a31193cf11f8
parent 509552 99b68e9d4a7c2aad92a4a7c64e96c71702b37e64
child 509554 ff8f2153265d685e8486211e391a2fdc09185453
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag, RyanVM
bugs1519762, 1503760
milestone65.0
Bug 1519762 - Fix a regression with browser.downloads.search(). r=kmag, a=RyanVM The fixes in bug 1503760 inadvertently broke searches of downloads that have not yet started receiving data (which have a value of -1 for the totalBytes property). That regression is fixed here. Differential Revision: https://phabricator.services.mozilla.com/D16889
toolkit/components/extensions/parent/ext-downloads.js
toolkit/components/extensions/schemas/downloads.json
toolkit/components/extensions/test/xpcshell/test_ext_downloads_search.js
--- a/toolkit/components/extensions/parent/ext-downloads.js
+++ b/toolkit/components/extensions/parent/ext-downloads.js
@@ -261,18 +261,18 @@ const downloadQuery = query => {
     return ExtensionCommon.normalizeTime(arg).getTime();
   }
 
   const startedBefore = normalizeDownloadTime(query.startedBefore, true);
   const startedAfter = normalizeDownloadTime(query.startedAfter, false);
   // const endedBefore = normalizeDownloadTime(query.endedBefore, true);
   // const endedAfter = normalizeDownloadTime(query.endedAfter, false);
 
-  const totalBytesGreater = query.totalBytesGreater;
-  const totalBytesLess = query.totalBytesLess != null ? query.totalBytesLess : Number.MAX_VALUE;
+  const totalBytesGreater = query.totalBytesGreater !== null ? query.totalBytesGreater : -1;
+  const totalBytesLess = query.totalBytesLess !== null ? query.totalBytesLess : Number.MAX_VALUE;
 
   // Handle options for which we can have a regular expression and/or
   // an explicit value to match.
   function makeMatch(regex, value, field) {
     if (value == null && regex == null) {
       return input => true;
     }
 
@@ -318,17 +318,17 @@ const downloadQuery = query => {
       }
     } else if (item.startTime > startedBefore || item.startTime < startedAfter) {
       return false;
     }
 
     // todo endedBefore, endedAfter
 
     if (item.totalBytes == -1) {
-      if (query.totalBytesGreater != null || query.totalBytesLess != null) {
+      if (query.totalBytesGreater !== null || query.totalBytesLess !== null) {
         return false;
       }
     } else if (item.totalBytes <= totalBytesGreater || item.totalBytes >= totalBytesLess) {
       return false;
     }
 
     // todo: include danger
     const SIMPLE_ITEMS = ["id", "mime", "startTime", "endTime", "state",
--- a/toolkit/components/extensions/schemas/downloads.json
+++ b/toolkit/components/extensions/schemas/downloads.json
@@ -107,17 +107,18 @@
             "type": "boolean"
           },
           "danger": {
             "$ref": "DangerType",
             "description": "Indication of whether this download is thought to be safe or known to be suspicious."
           },
           "mime": {
             "description": "The file's MIME type.",
-            "type": "string"
+            "type": "string",
+            "optional": true
           },
           "startTime": {
             "description": "Number of milliseconds between the unix epoch and when this download began.",
             "type": "string"
           },
           "endTime": {
             "description": "Number of milliseconds between the unix epoch and when this download ended.",
             "optional": true,
@@ -252,18 +253,17 @@
           "endedAfter": {
             "description": "Limits results to downloads that ended after the given ms since the epoch.",
             "optional": true,
             "$ref": "DownloadTime"
           },
           "totalBytesGreater": {
             "description": "Limits results to downloads whose totalBytes is greater than the given integer.",
             "optional": true,
-            "type": "number",
-            "default": -1
+            "type": "number"
           },
           "totalBytesLess": {
             "description": "Limits results to downloads whose totalBytes is less than the given integer.",
             "optional": true,
             "type": "number"
           },
           "filenameRegex": {
             "description": "Limits results to <a href='#type-DownloadItem'>DownloadItems</a> whose <code>filename</code> matches the given regular expression.",
--- a/toolkit/components/extensions/test/xpcshell/test_ext_downloads_search.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_downloads_search.js
@@ -1,12 +1,14 @@
 /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
 /* vim: set sts=2 sw=2 et tw=80: */
 "use strict";
 
+PromiseTestUtils.whitelistRejectionsGlobally(/Message manager disconnected/);
+
 ChromeUtils.import("resource://gre/modules/Downloads.jsm");
 
 const server = createHttpServer();
 server.registerDirectory("/data/", do_get_file("data"));
 
 const BASE = `http://localhost:${server.identity.primaryPort}/data`;
 const TXT_FILE = "file_download.txt";
 const TXT_URL = BASE + "/" + TXT_FILE;
@@ -419,8 +421,54 @@ add_task(async function test_search() {
   await checkBadSearch({endedAfter: "i am not a time"}, /Type error/, "query.endedAfter is not a valid time");
   await checkBadSearch({urlRegex: "["}, /Invalid urlRegex/, "query.urlRegexp is not a valid regular expression");
   await checkBadSearch({filenameRegex: "["}, /Invalid filenameRegex/, "query.filenameRegexp is not a valid regular expression");
   await checkBadSearch({orderBy: "startTime"}, /Expected array/, "query.orderBy is not an array");
   await checkBadSearch({orderBy: ["bogus"]}, /Invalid orderBy field/, "query.orderBy references a non-existent field");
 
   await extension.unload();
 });
+
+// Test that downloads with totalBytes of -1 (ie, that have not yet started)
+// work properly.  See bug 1519762 for details of a past regression in
+// this area.
+add_task(async function test_inprogress() {
+  let resume, resumePromise = new Promise(resolve => { resume = resolve; });
+  server.registerPathHandler("/slow", async (request, response) => {
+    response.processAsync();
+    await resumePromise;
+    response.setHeader("Content-type", "text/plain");
+    response.write("");
+    response.finish();
+  });
+
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      permissions: ["downloads"],
+    },
+    background() {
+      browser.test.onMessage.addListener(async (msg, url) => {
+        let id = await browser.downloads.download({url});
+        let full = await browser.downloads.search({id});
+
+        browser.test.assertEq(full.length, 1,
+                              "Found new download in search results");
+        browser.test.assertEq(full[0].totalBytes, -1,
+                              "New download still has totalBytes == -1");
+
+        browser.downloads.onChanged.addListener(info => {
+          if (info.id == id && info.state.current == "complete") {
+            browser.test.notifyPass("done");
+          }
+        });
+
+        browser.test.sendMessage("started");
+      });
+    },
+  });
+
+  await extension.startup();
+  extension.sendMessage("go", `${BASE}/slow`);
+  await extension.awaitMessage("started");
+  resume();
+  await extension.awaitFinish("done");
+  await extension.unload();
+});