From f5e5f16f87c10a07d04db75f76ef070287a187b4 Mon Sep 17 00:00:00 2001 From: Matchu Date: Sat, 26 Jun 2021 12:04:40 -0700 Subject: [PATCH] Improve movie cancels and error handling Oops, my inbox was getting full of uncaught promise rejections of `loadImage`! I'm pretty sure they're caused when multiple images in a movie fail to load (e.g. network problems), but we fail to cancel them. So, the first failure would be caught as a part of `Promise.all` in `loadMovieLibrary`, but then subsequent failures wouldn't be caught by anything, and would propagate up to the console and to Sentry as uncaught errors. In this change, we make a number of improvements to cancellation. The most relevant change for this bug is that `loadMovieLibrary` will now automatically cancel all resource promises when it throws an error! But this improved robustness also enabled us to finally offer a simple `cancel()` method on movie library promises, which we now available ourselves of at call sites, too. --- src/app/components/OutfitMovieLayer.js | 219 ++++++++++++++++--------- src/app/components/OutfitPreview.js | 13 +- src/app/util.js | 14 ++ 3 files changed, 169 insertions(+), 77 deletions(-) diff --git a/src/app/components/OutfitMovieLayer.js b/src/app/components/OutfitMovieLayer.js index 12e4cc3..da4375d 100644 --- a/src/app/components/OutfitMovieLayer.js +++ b/src/app/components/OutfitMovieLayer.js @@ -124,7 +124,8 @@ function OutfitMovieLayer({ React.useEffect(() => { let canceled = false; - loadMovieLibrary(libraryUrl) + const movieLibraryPromise = loadMovieLibrary(libraryUrl); + movieLibraryPromise .then((library) => { if (canceled) { return; @@ -137,14 +138,25 @@ function OutfitMovieLayer({ }) .catch((e) => { console.error(`Error loading outfit movie layer: ${libraryUrl}`, e); + if (!toast.isActive("use-preload-layers-movie-failed")) { + toast({ + id: "use-preload-layers-movie-failed", + status: "warning", + title: "Oops, we couldn't load one of these animations.", + description: "We'll show a static image version instead.", + duration: null, + isClosable: true, + }); + } }); return () => { canceled = true; + movieLibraryPromise.cancel(); setLibrary(null); setMovieClip(null); }; - }, [libraryUrl]); + }, [libraryUrl, toast]); // This effect puts the `movieClip` on the `stage`, when both are ready. React.useEffect(() => { @@ -261,91 +273,150 @@ function OutfitMovieLayer({ } function loadScriptTag(src) { - return new Promise((resolve, reject) => { - const script = document.createElement("script"); - script.onload = () => resolve(script); - script.onerror = (e) => reject(e); + let script; + let canceled = false; + let resolved = false; + + const scriptTagPromise = new Promise((resolve, reject) => { + script = document.createElement("script"); + script.onload = () => { + if (canceled) return; + resolved = true; + resolve(script); + }; + script.onerror = (e) => { + if (canceled) return; + reject(e); + }; script.src = src; document.body.appendChild(script); }); + + scriptTagPromise.cancel = () => { + if (resolved) return; + script.src = ""; + canceled = true; + }; + + return scriptTagPromise; } const MOVIE_LIBRARY_CACHE = new LRU(10); -export async function loadMovieLibrary(librarySrc) { - // First, check the LRU cache. This will enable us to quickly return movie - // libraries, without re-loading and re-parsing and re-executing. - const cachedLibrary = MOVIE_LIBRARY_CACHE.get(librarySrc); - if (cachedLibrary) { - return cachedLibrary; - } +export function loadMovieLibrary(librarySrc) { + const cancelableResourcePromises = []; + const cancelAllResources = () => + cancelableResourcePromises.forEach((p) => p.cancel()); - // These library JS files are interesting in their operation. It seems like - // the idea is, it pushes an object to a global array, and you need to snap - // it up and see it at the end of the array! And I don't really see a way to - // like, get by a name or ID that we know by this point. So, here we go, just - // try to grab it once it arrives! - // - // I'm not _sure_ this method is reliable, but it seems to be stable so far - // in Firefox for me. The things I think I'm observing are: - // - Script execution order should match insert order, - // - Onload execution order should match insert order, - // - BUT, script executions might be batched before onloads. - // - So, each script grabs the _first_ composition from the list, and - // deletes it after grabbing. That way, it serves as a FIFO queue! - // I'm not suuure this is happening as I'm expecting, vs I'm just not seeing - // the race anymore? But fingers crossed! - await loadScriptTag(safeImageUrl(librarySrc)); - const [compositionId, composition] = Object.entries( - window.AdobeAn.compositions - )[0]; - if (Object.keys(window.AdobeAn.compositions).length > 1) { - console.warn( - `Grabbing composition ${compositionId}, but there are >1 here: `, - Object.keys(window.AdobeAn.compositions).length + // Most of the logic for `loadMovieLibrary` is inside this async function. + // But we want to attach more fields to the promise before returning it; so + // we declare this async function separately, then call it, then edit the + // returned promise! + const createMovieLibraryPromise = async () => { + // First, check the LRU cache. This will enable us to quickly return movie + // libraries, without re-loading and re-parsing and re-executing. + const cachedLibrary = MOVIE_LIBRARY_CACHE.get(librarySrc); + if (cachedLibrary) { + return cachedLibrary; + } + + // Then, load the script tag. (Make sure we set it up to be cancelable!) + const scriptPromise = loadScriptTag(safeImageUrl(librarySrc)); + cancelableResourcePromises.push(scriptPromise); + await scriptPromise; + + // These library JS files are interesting in their operation. It seems like + // the idea is, it pushes an object to a global array, and you need to snap + // it up and see it at the end of the array! And I don't really see a way to + // like, get by a name or ID that we know by this point. So, here we go, just + // try to grab it once it arrives! + // + // I'm not _sure_ this method is reliable, but it seems to be stable so far + // in Firefox for me. The things I think I'm observing are: + // - Script execution order should match insert order, + // - Onload execution order should match insert order, + // - BUT, script executions might be batched before onloads. + // - So, each script grabs the _first_ composition from the list, and + // deletes it after grabbing. That way, it serves as a FIFO queue! + // I'm not suuure this is happening as I'm expecting, vs I'm just not seeing + // the race anymore? But fingers crossed! + if (Object.keys(window.AdobeAn?.compositions || {}).length === 0) { + throw new Error( + `Movie library ${librarySrc} did not add a composition to window.AdobeAn.compositions.` + ); + } + const [compositionId, composition] = Object.entries( + window.AdobeAn.compositions + )[0]; + if (Object.keys(window.AdobeAn.compositions).length > 1) { + console.warn( + `Grabbing composition ${compositionId}, but there are >1 here: `, + Object.keys(window.AdobeAn.compositions).length + ); + } + delete window.AdobeAn.compositions[compositionId]; + const library = composition.getLibrary(); + + // One more loading step as part of loading this library is loading the + // images it uses for sprites. + // + // TODO: I guess the manifest has these too, so if we could use our DB cache + // to get the manifest to us faster, then we could avoid a network RTT + // on the critical path by preloading these images before the JS file + // even gets to us? + const librarySrcDir = librarySrc.split("/").slice(0, -1).join("/"); + const manifestImages = new Map( + library.properties.manifest.map(({ id, src }) => [ + id, + loadImage(librarySrcDir + "/" + src, { + crossOrigin: "anonymous", + }), + ]) ); - } - delete window.AdobeAn.compositions[compositionId]; - const library = composition.getLibrary(); - // One more loading step as part of loading this library is loading the - // images it uses for sprites. - // - // TODO: I guess the manifest has these too, so if we could use our DB cache - // to get the manifest to us faster, then we could avoid a network RTT - // on the critical path by preloading these images before the JS file - // even gets to us? - const librarySrcDir = librarySrc.split("/").slice(0, -1).join("/"); - const manifestImages = new Map( - library.properties.manifest.map(({ id, src }) => [ - id, - loadImage(librarySrcDir + "/" + src, { - crossOrigin: "anonymous", - }), - ]) - ); - await Promise.all(manifestImages.values()); + // Wait for the images, and make sure they're cancelable while we do. + const manifestImagePromises = manifestImages.values(); + cancelableResourcePromises.push(...manifestImagePromises); + await Promise.all(manifestImagePromises); - // Finally, once we have the images loaded, the library object expects us to - // mutate it (!) to give it the actual image and sprite sheet objects from - // the loaded images. That's how the MovieClip's internal JS objects will - // access the loaded data! - const images = composition.getImages(); - for (const [id, image] of manifestImages.entries()) { - images[id] = await image; - } - const spriteSheets = composition.getSpriteSheet(); - for (const { name, frames } of library.ssMetadata) { - const image = await manifestImages.get(name); - spriteSheets[name] = new window.createjs.SpriteSheet({ - images: [image], - frames, - }); - } + // Finally, once we have the images loaded, the library object expects us to + // mutate it (!) to give it the actual image and sprite sheet objects from + // the loaded images. That's how the MovieClip's internal JS objects will + // access the loaded data! + const images = composition.getImages(); + for (const [id, image] of manifestImages.entries()) { + images[id] = await image; + } + const spriteSheets = composition.getSpriteSheet(); + for (const { name, frames } of library.ssMetadata) { + const image = await manifestImages.get(name); + spriteSheets[name] = new window.createjs.SpriteSheet({ + images: [image], + frames, + }); + } - MOVIE_LIBRARY_CACHE.set(librarySrc, library); + MOVIE_LIBRARY_CACHE.set(librarySrc, library); - return library; + return library; + }; + + const movieLibraryPromise = createMovieLibraryPromise().catch((e) => { + // When any part of the movie library fails, we also cancel the other + // resources ourselves, to avoid stray throws for resources that fail after + // the parent catches the initial failure. We re-throw the initial failure + // for the parent to handle, though! + cancelAllResources(); + throw e; + }); + + // To cancel a `loadMovieLibrary`, cancel all of the resource promises we + // load as part of it. That should effectively halt the async function above + // (anything not yet loaded will stop loading), and ensure that stray + // failures don't trigger uncaught promise rejection warnings. + movieLibraryPromise.cancel = cancelAllResources; + + return movieLibraryPromise; } export function buildMovieClip(library, libraryUrl) { diff --git a/src/app/components/OutfitPreview.js b/src/app/components/OutfitPreview.js index d4f7dde..8345f61 100644 --- a/src/app/components/OutfitPreview.js +++ b/src/app/components/OutfitPreview.js @@ -387,12 +387,15 @@ export function usePreloadLayers(layers) { // Start preloading the movie. But we won't block on it! The blocking // request will still be the image, which we'll show as a // placeholder, which should usually be noticeably faster! - const movieAssetPromise = loadMovieLibrary( + const movieLibraryPromise = loadMovieLibrary( layer.canvasMovieLibraryUrl - ).then((library) => ({ + ); + const movieAssetPromise = movieLibraryPromise.then((library) => ({ library, libraryUrl: layer.canvasMovieLibraryUrl, })); + movieAssetPromise.libraryUrl = layer.canvasMovieLibraryUrl; + movieAssetPromise.cancel = () => movieLibraryPromise.cancel(); movieAssetPromises.push(movieAssetPromise); // The minimal asset for the movie case is *either* the image *or* @@ -439,7 +442,11 @@ export function usePreloadLayers(layers) { (alreadyHasAnimations) => alreadyHasAnimations || assetHasAnimations ); }; - movieAssetPromises.forEach((p) => p.then(checkHasAnimations)); + movieAssetPromises.forEach((p) => + p.then(checkHasAnimations).catch((e) => { + console.error(`Error preloading movie library ${p.libraryUrl}:`, e); + }) + ); }; loadAssets(); diff --git a/src/app/util.js b/src/app/util.js index d4842fc..ec401eb 100644 --- a/src/app/util.js +++ b/src/app/util.js @@ -356,9 +356,12 @@ export function loadImage(rawSrc, { crossOrigin = null } = {}) { const src = safeImageUrl(rawSrc, { crossOrigin }); const image = new Image(); let canceled = false; + let resolved = false; + const promise = new Promise((resolve, reject) => { image.onload = () => { if (canceled) return; + resolved = true; resolve(image); }; image.onerror = () => { @@ -370,10 +373,21 @@ export function loadImage(rawSrc, { crossOrigin = null } = {}) { } image.src = src; }); + promise.cancel = () => { + // NOTE: To keep `cancel` a safe and unsurprising call, we don't cancel + // resolved images. That's because our approach to cancelation + // mutates the Image object we already returned, which could be + // surprising if the caller is using the Image and expected the + // `cancel` call to only cancel any in-flight network requests. + // (e.g. we cancel a DTI movie when it unloads from the page, but + // it might stick around in the movie cache, and we want those images + // to still work!) + if (resolved) return; image.src = ""; canceled = true; }; + return promise; }