1802738 - Errors logged when trying to "Remove from History" a canceled download

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Shane Hughes [:aminomancer]]

This is a low severity issue just to deal with an error log. Everything is fine behaviorally, but we're logging an expected error when the user clicks "Remove from History" on a canceled download. Here's the stack:

DOMException: Could not stat file(path/to/file) because it does not exist DownloadCore.sys.mjs:104:13
    isPlaceholder resource://gre/modules/DownloadCore.sys.mjs:104
    removeData resource://gre/modules/DownloadCore.sys.mjs:2639
    removeData resource://gre/modules/DownloadCore.sys.mjs:2996
    _promiseRemovePartialData resource://gre/modules/DownloadCore.sys.mjs:938
    removePartialData resource://gre/modules/DownloadCore.sys.mjs:949
    finalize resource://gre/modules/DownloadCore.sys.mjs:1115
    deleteDownload resource:///modules/DownloadsCommon.sys.mjs:349
    cmd_delete resource:///modules/DownloadsViewUI.sys.mjs:1171
    doCommand chrome://browser/content/downloads/downloads.js:1137
    doCommand chrome://browser/content/downloads/downloads.js:1328
    goDoCommand chrome://global/content/globalOverlay.js:128
    <anonymous> chrome://global/content/editMenuOverlay.js:85

I didn't notice this before with this particular command (the "Remove from History" context menu item), but I saw something similar when we were working on the "Delete" menuitem for bug 1745624. Basically stat is going to fail because canceling the download already removed the placeholder. But afaik that is the fastest way to check if it exists, which is the only way to know if it's going to throw, so there's no point trying to check existence before statting. But we could just add this pattern which is used elsewhere in the downloads code:

--- a/toolkit/components/downloads/DownloadCore.sys.mjs
+++ b/toolkit/components/downloads/DownloadCore.sys.mjs
@@ -101,7 +101,9 @@
 async function isPlaceholder(path) {
   try {
     if ((await IOUtils.stat(path)).size == 0) {
       return true;
     }
   } catch (ex) {
-    console.error(ex);
+    if (ex.name != "NotFoundError") {
+      console.error(ex);
+    }
   }
   return false;
 }

However, I have another idea to just not put this explicit logging behavior in the helper function at all, since not every caller of removeData is expecting it not to throw (because IOUtils.stat throws when the file is not found, and some callers can be invoked on a canceled download). Each caller should handle the error individually IMO, since some want to throw on a NotFoundError, while others just want to gracefully fizzle out. Alternatively, we could add an optional parameter to this function telling it not to log the error. Or an array of error names not to log.

Comment 1

[Shane Hughes [:aminomancer]]

Attachment: Bug 1802738 - Fix the error log when deleting canceled downloads. r=mak

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb9ba144b7d9
Fix the error log when deleting canceled downloads. r=mak

Comment 3

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/cb9ba144b7d9