[pull] main from enyo:main

packages/dropzone/CHANGELOG.md

@@ -1,3 +1,27 @@
+## 6.3.2
+
+### Patch Changes
+
+- [#2371](https://github.com/enyo/dropzone/pull/2371) [`536d94a`](https://github.com/enyo/dropzone/commit/536d94afd9da8f55b4e4976bd527909946e75e4a) - Fix `cancelUpload` leaving parallel chunks uploading.
+
+  `file.xhr` only holds the request that started last, so cancelling a chunked upload running with `parallelChunkUploads` aborted that one request and left every other in-flight chunk streaming to the server — burning the user's bandwidth and writing orphaned chunks for a file the UI already showed as canceled.
+
+  Every chunk keeps its own request, so `cancelUpload` now aborts all of the ones still running. Uploads that are not chunked are unaffected.
+
+- [#2372](https://github.com/enyo/dropzone/pull/2372) [`a1a67df`](https://github.com/enyo/dropzone/commit/a1a67dfe16c100b2346974c6d3a7270fc661c423) - Fix `emit` skipping a listener when another one removes itself.
+
+  `emit` walked the live callback array, so a listener that called `off` for itself — the usual shape of a one-shot listener, and of teardown code — spliced the array out from under the loop and the listener registered right after it never ran. `emit` now iterates over a snapshot.
+
+  One consequence worth knowing about: a listener registered from inside another listener no longer runs during that same `emit`, it runs from the next one. That is what Node's `EventEmitter` does, and it is the only way to keep the removal case correct.
+
+- [#2370](https://github.com/enyo/dropzone/pull/2370) [`0e3625d`](https://github.com/enyo/dropzone/commit/0e3625d05d5861561447be3de64e5481e2186bb1) - Fix the thumbnail queue deadlocking when a file cannot be read.
+
+  `createThumbnail` only listened for `FileReader`'s `load` event. A file that had been moved, locked by another process, or was otherwise unreadable since it was dropped fires `error` instead, so the callback was never invoked and `_processThumbnailQueue` kept its lock forever: no file added afterwards got a thumbnail, and with `resizeWidth`/`resizeHeight` or a `transformFile` that uses `createThumbnail`, the upload never started either.
+
+  The read error now reaches the callback the same way an undecodable image already did, so the file gets `dictThumbnailError` and the queue moves on.
+
+  `DropzoneThumbnailCallback` says what it has always done, too: its first argument is `string | Event`, the error event standing in for the data URL when no thumbnail could be produced. That also fixes `displayExistingFile`, which used to emit that event as a thumbnail when the image URL failed to load, leaving the preview with `img.src` set to `"[object Event]"`.
+
 ## 6.3.1
 
 ### Patch Changes

packages/dropzone/package.json

@@ -1,6 +1,6 @@
 {
   "name": "dropzone",
-  "version": "6.3.1",
+  "version": "6.3.2",
   "description": "Handles drag and drop of files for you.",
   "keywords": [
     "drag and drop",

packages/dropzone/src/dropzone.ts

@@ -68,9 +68,14 @@ export type DropzoneTransformCallback = (file: DropzoneFile | Blob) => void;
 /**
  * Invoked with the rendered thumbnail as a data URL, and the canvas it was
  * drawn on -- which is null when the image needed no resizing.
+ *
+ * There is no separate error callback: when the thumbnail cannot be produced,
+ * because the file cannot be read or the image cannot be decoded, this is
+ * invoked with the error event in place of the data URL. Check with
+ * `typeof dataUrl === "string"` before using it.
  */
 export type DropzoneThumbnailCallback = (
-  dataUrl: string,
+  dataUrl: string | Event,
   canvas?: HTMLCanvasElement | null,
 ) => void;
 
@@ -979,11 +984,11 @@ export default class Dropzone extends Emitter {
       this.options.thumbnailHeight,
       this.options.thumbnailMethod,
       true,
-      (dataUrl: string) => {
-        // `createThumbnailFromUrl` hands its callback the error event when the
-        // image cannot be decoded, so anything that is not a data URL means
-        // the thumbnail failed. Emitting it as one would set the preview's
-        // `img.src` to "[object Event]" and render a broken image. See #2218.
+      (dataUrl: string | Event) => {
+        // Both failure paths -- the file not being readable, and the image
+        // not being decodable -- hand the callback the error event. Emitting
+        // that as a thumbnail would set the preview's `img.src` to
+        // "[object Event]" and render a broken image. See #2218 and #2365.
         if (typeof dataUrl === "string") {
           this.emit("thumbnail", file, dataUrl);
         } else {
@@ -1039,9 +1044,10 @@ export default class Dropzone extends Emitter {
       height,
       resizeMethod,
       true,
-      (dataUrl: string, canvas?: HTMLCanvasElement | null) => {
+      (dataUrl: string | Event, canvas?: HTMLCanvasElement | null) => {
         if (canvas == null) {
-          // The image has not been resized
+          // The image has not been resized, or could not be read or decoded at
+          // all -- either way there is nothing to send but the original file.
           return callback(file);
         } else {
           let { resizeMimeType } = this.options;
@@ -1094,6 +1100,18 @@ export default class Dropzone extends Emitter {
       this.createThumbnailFromUrl(file, width, height, resizeMethod, fixOrientation, callback);
     };
 
+    // A file that cannot be read -- moved, locked by another process, or on a
+    // drive that went away since it was dropped -- fires `error` and never
+    // `load`. Without this the callback is never invoked at all, which leaves
+    // `_processThumbnailQueue` holding its lock forever: no later file gets a
+    // thumbnail, and an upload waiting on `transformFile` never sends. See
+    // #2365.
+    fileReader.onerror = (e) => {
+      if (callback != null) {
+        callback(e);
+      }
+    };
+
     fileReader.readAsDataURL(file);
   }
 
@@ -1117,8 +1135,14 @@ export default class Dropzone extends Emitter {
       this.emit("thumbnail", mockFile, imageUrl);
       if (callback) callback();
     } else {
-      let onDone = (thumbnail: string) => {
-        this.emit("thumbnail", mockFile, thumbnail);
+      let onDone = (thumbnail: string | Event) => {
+        // An image URL that does not load -- gone from the server, or blocked
+        // by CORS -- arrives here as the error event. Emitting that as a
+        // thumbnail would set the preview's `img.src` to "[object Event]", so
+        // leave the preview alone and just report that we are done.
+        if (typeof thumbnail === "string") {
+          this.emit("thumbnail", mockFile, thumbnail);
+        }
         if (callback) callback();
       };
       mockFile.dataURL = imageUrl;
@@ -1247,9 +1271,8 @@ export default class Dropzone extends Emitter {
 
     if (callback != null) {
       // The same callback does double duty: it receives the thumbnail on
-      // success, and is called as the image's error handler on failure, where
-      // an Event arrives instead of a data URL.
-      img.onerror = callback as unknown as OnErrorEventHandler;
+      // success, and the error event on failure.
+      img.onerror = (e) => callback(e);
     }
 
     return (img.src = file.dataURL!);
@@ -1321,7 +1344,18 @@ export default class Dropzone extends Emitter {
       for (let groupedFile of groupedFiles) {
         groupedFile.status = Dropzone.CANCELED;
       }
-      if (typeof file.xhr !== "undefined") {
+      if (file.upload.chunked && file.upload.chunks) {
+        // `file.xhr` only ever holds the request that started last, so with
+        // `parallelChunkUploads` aborting it leaves every other chunk
+        // streaming to the server for a file the user has already canceled.
+        // Each chunk keeps its own request, so abort them all. See #2366.
+        for (let chunk of file.upload.chunks) {
+          if (chunk && chunk.xhr && chunk.status === Dropzone.UPLOADING) {
+            chunk.status = Dropzone.CANCELED;
+            chunk.xhr.abort();
+          }
+        }
+      } else if (typeof file.xhr !== "undefined") {
         file.xhr.abort();
       }
       for (let groupedFile of groupedFiles) {

packages/dropzone/src/emitter.ts

@@ -25,7 +25,12 @@ export default class Emitter {
     let callbacks = this._callbacks[event!];
 
     if (callbacks) {
-      for (let callback of callbacks) {
+      // Iterating the live array would skip a listener whenever one of them
+      // removes itself: `off` splices in place, so everything after the
+      // removed entry shifts down past the loop's index. Snapshotting also
+      // means a listener added by another listener does not run until the
+      // next emit, which is how EventEmitter behaves. See #2367.
+      for (let callback of callbacks.slice()) {
         callback.apply(this, args);
       }
     }

packages/dropzone/test/unit-tests/all.js

@@ -1363,6 +1363,52 @@ describe("Dropzone", function () {
             dropzone.addFile(corrupt);
           }));
 
+        it("should emit an error and keep the queue moving if a file can't be read", async function () {
+          dropzone.processFile = function () {};
+          dropzone.uploadFile = function () {};
+
+          // A file that has gone away since it was dropped, or is locked by
+          // another process, makes FileReader fire `error` and never `load`.
+          // See #2365.
+          let readAsDataURL = vi
+            .spyOn(FileReader.prototype, "readAsDataURL")
+            .mockImplementation(function () {
+              setTimeout(() => this.dispatchEvent(new ProgressEvent("error")), 0);
+            });
+
+          let unreadable = getMockFile("image/png", "unreadable.png");
+
+          let message = await new Promise(function (resolve) {
+            dropzone.on("error", function (file, message) {
+              if (file === unreadable) resolve(message);
+            });
+            dropzone.addFile(unreadable);
+          });
+
+          expect(message).toBe(dropzone.options.dictThumbnailError);
+          // The lock has to be released, or nothing queued behind the failed
+          // file is ever processed again.
+          expect(dropzone._processingThumbnail).toBe(false);
+
+          readAsDataURL.mockRestore();
+
+          let readable = await new Promise(function (resolve) {
+            let canvas = document.createElement("canvas");
+            canvas.width = canvas.height = 10;
+            canvas.toBlob(
+              (blob) => resolve(new File([blob], "readable.png", { type: "image/png" })),
+              "image/png",
+            );
+          });
+
+          let thumbnailed = await new Promise(function (resolve) {
+            dropzone.on("thumbnail", (file) => resolve(file));
+            dropzone.addFile(readable);
+          });
+
+          expect(thumbnailed).toBe(readable);
+        });
+
         it("should not let the thumbnail itself be dragged", function () {
           dropzone.processFile = function () {};
           dropzone.uploadFile = function () {};
@@ -2162,6 +2208,36 @@ describe("Dropzone", function () {
               }, 10);
             }));
 
+          it("should abort every chunk still in flight when the upload is canceled", () =>
+            new Promise((done) => {
+              dropzone.options.chunking = true;
+              dropzone.options.chunkSize = 1;
+              dropzone.options.parallelChunkUploads = 3;
+
+              let file = getMockFile("text/html", "chunked-file", ["abcdef"]);
+              dropzone.addFile(file);
+
+              setTimeout(function () {
+                // Three of the six chunks are in flight, each with its own
+                // request.
+                expect(requests.length).toBe(3);
+                expect(requests.map((request) => request.aborted)).toEqual([false, false, false]);
+
+                dropzone.cancelUpload(file);
+
+                expect(file.status).toBe(Dropzone.CANCELED);
+                // `file.xhr` only holds the request that started last, so the
+                // other two used to keep streaming to the server. See #2366.
+                expect(requests.map((request) => request.aborted)).toEqual([true, true, true]);
+                expect(file.upload.chunks.map((chunk) => chunk.status)).toEqual([
+                  Dropzone.CANCELED,
+                  Dropzone.CANCELED,
+                  Dropzone.CANCELED,
+                ]);
+                done();
+              }, 10);
+            }));
+
           it("should never start fewer than one chunk", () =>
             new Promise((done) => {
               startChunked({ parallelChunkUploads: 0 });

packages/dropzone/test/unit-tests/emitter.js

@@ -70,6 +70,63 @@ describe("Emitter", function () {
     return expect(callCount2).toBe(1);
   });
 
+  describe(".emit() while the listeners change", function () {
+    it("should still run every listener when one removes itself", function () {
+      let calls = [];
+
+      // A one-shot listener is the usual way to end up here: it unregisters
+      // itself from inside the very emit that is walking the array. Splicing
+      // it out shifted everything after it down past the loop's index, so the
+      // listener that followed was silently skipped. See #2367.
+      let first = function () {
+        calls.push("first");
+        emitter.off("test", first);
+      };
+      emitter.on("test", first);
+      emitter.on("test", () => calls.push("second"));
+      emitter.on("test", () => calls.push("third"));
+
+      emitter.emit("test");
+
+      expect(calls).toEqual(["first", "second", "third"]);
+
+      // And it really is gone for the next one.
+      calls = [];
+      emitter.emit("test");
+      expect(calls).toEqual(["second", "third"]);
+    });
+
+    it("should still run the listeners that were registered when off() removes them all", function () {
+      let calls = [];
+
+      emitter.on("test", function () {
+        calls.push("first");
+        emitter.off("test");
+      });
+      emitter.on("test", () => calls.push("second"));
+
+      emitter.emit("test");
+
+      expect(calls).toEqual(["first", "second"]);
+      expect(emitter._callbacks["test"]).toBe(undefined);
+    });
+
+    it("should not run a listener added by another listener until the next emit", function () {
+      let calls = [];
+
+      emitter.on("test", function () {
+        calls.push("first");
+        emitter.on("test", () => calls.push("added"));
+      });
+
+      emitter.emit("test");
+      expect(calls).toEqual(["first"]);
+
+      emitter.emit("test");
+      expect(calls).toEqual(["first", "first", "added"]);
+    });
+  });
+
   describe(".off()", function () {
     let callback1 = function () {};
     let callback2 = function () {};

packages/dropzone/test/unit-tests/public-api.js

@@ -66,6 +66,20 @@ describe("public API", function () {
       expect(dropzone.createThumbnailFromUrl).toHaveBeenCalledTimes(1);
     });
 
+    it("should not emit a thumbnail when the image url fails to load", async function () {
+      create();
+      let thumbnail = null;
+      dropzone.on("thumbnail", (file, url) => (thumbnail = url));
+
+      // A URL that never resolves to an image: the preview would otherwise be
+      // handed the error event and set `img.src` to "[object Event]".
+      await new Promise((done) =>
+        dropzone.displayExistingFile(mockFile(), "/does-not-exist.png", done),
+      );
+
+      expect(thumbnail).toBe(null);
+    });
+
     // #2003 and the 7.0 roadmap: files added this way never reach this.files,
     // so maxFiles cannot see them. The documented workaround is to push them
     // by hand, which is why fixing it is a breaking change rather than a