Inconsistent handling of missing costumes (web site vs. desktop app) #367

cwillisf · 2022-03-08T21:34:22Z

Expected Behavior

Loading into the desktop app a project which refers to a missing costume should cause that costume to appear as a placeholder image, just like it does on the Scratch website.

Actual Behavior

Loading into the desktop app a project which refers to a missing costume causes an error message and prevents the project from loading at all.

Note that this means a single missing costume will prevent using the desktop app to recover any other data from a project -- even intact sprites, costumes, or sounds.

Steps to Reproduce

Open the standalone Scratch app on a Mac or Windows computer
Load a project which refers to a missing costume

Example project: Missing Blue Guy (broken).sb3.zip

Screenshots

When this happens, the developer console looks like this:

cwillisf · 2022-03-08T21:36:29Z

This appears to be due to the storage system making an assumption about the web helper being populated, which isn't true in the desktop app. The desktop app should not be using WebHelper at all, so it certainly shouldn't be causing an error in the console.

apple502j · 2022-03-09T18:19:37Z

@cwillisf There exists a legitimate reason to use a WebHelper (music extension assets), however for costumes that's not the case. We could remove these lines: https://github.com/LLK/scratch-gui/blob/scratch-desktop/src/lib/storage.js#L15

However this does not fix the root cause, which can be fixed by adding the following line after https://github.com/LLK/scratch-storage/blob/develop/src/WebHelper.js#L102

if (!store) return Promise.resolve(null);

I also found another issue while debugging this: if the asset server could not be fetched at all (due to CORS, network disconnection, blocking the URL using developer tools etc) the code ends up in an infinite loop, spamming hundreds of logs per second and never loading the project.

cwillisf · 2022-03-09T19:08:52Z

Ooh, thanks for the reminder about the music assets! I stand by my assertion that the desktop app should avoid using WebHelper, since we intend it to be usable in completely offline environments, but considering the music extension means we're a little further from that ideal than I had been thinking when I wrote that comment. In particular, it's not just a technical issue like I had been thinking, but instead involves design decisions and user experience tradeoffs. I expect we won't decide to change it any time soon, but like you said, we can fix the costume case without breaking the music extension.

Thanks for bringing up that infinite loop issue, too! That's... not great 😅

@AndreyNikitin

FetchTool should resolve null for the data request if the request gets a 404 response. WebHelper should properly handle this case and resolve null for the asset (instead of an asset skeleton with null 'data' field) if the asset is nowhere to be found. WebHelper should also report errors properly. BREAKING CHANGE: The WebHelper was not previously resolving with null assets even though this was the documented intended behavior of ScratchStorage.load builds on and closes scratchfoundation#363 (thanks @AndreyNikitin!), closes scratchfoundation#156, might be related to scratchfoundation#367

@AndreyNikitin

* fix: polyfill 'fetch' for Node target * fix(tests): remove 'fetch' workaround This workaround was making the tests pass even when `fetch` was undefined. That made sense if you assume that the only Node-based clients of `scratch-storage` are its own tests, but it doesn't help (for example) Node-based tests in `scratch-vm`. Since we now ensure that `fetch` is polyfilled for Node, this workaround is no longer necessary. * fix(fetchtool): catch fetch exception on get resource * fix(fetchtool): update another fetch * fix(fetchtool, webhelper): handle asset requests with a 404 response FetchTool should resolve null for the data request if the request gets a 404 response. WebHelper should properly handle this case and resolve null for the asset (instead of an asset skeleton with null 'data' field) if the asset is nowhere to be found. WebHelper should also report errors properly. BREAKING CHANGE: The WebHelper was not previously resolving with null assets even though this was the documented intended behavior of ScratchStorage.load builds on and closes scratchfoundation#363 (thanks @AndreyNikitin!), closes scratchfoundation#156, might be related to scratchfoundation#367 * test(fetchtool): test handling 404 response to get request Add a test that FetchTool.get resolves null when it encounters a 404 for the get request * fix(fetchtool): create Uint8Array on promise result instead of promise We were previously calling new Uint8Array on a promise instead of the result of the promise * fix(fetchworkertool.worker): update `get` to handle 404 response * test(fetchtool): update tests to handle promise reject and resolve * fix(fetchworkertool): don't create Uint8Array for null response * fix(worker): report worker errors as strings Error objects can't be sent with `postMessage`, so report errors using strings instead. * fix(worker): protect against null/undefined errors Co-authored-by: Karishma Chadha <[email protected]> * feat(circleci): add circleci * ci: use node orb and node 16 * test: fix and modernize tests * test: stop using deprecated methods * chore: upgrade package-lock.json to lockfileVersion 2 ```sh npm ci && npm i --package-lock-only && npm i --package-lock-only ``` * refactor: use cross-fetch instead of node-fetch Previously, we had Webpack's ProvidePlugin injecting `node-fetch` as a polyfill into Node builds, and web builds just relied on the `fetch` built into the browser (if it's there). Now, we use `cross-fetch` everywhere, and as a ponyfill instead of a polyfill. This broke the unit tests in `fetch-tool.js` because they were overriding `global.fetch` to mock `node-fetch`. Now, those tests use `tap.mock` to mock `cross-fetch`. Of course, `tap.mock` didn't exist in the old version of `node-tap`, so I also upgraded that. * chore: insert scratchFetch into FetchTool's pipeline * feat: allow adding metadata to scratchFetch * fix: apply metadata to FetchWorkerTool too * fix: don't use ?. in src/ because VM isn't ready for that * feat: allow removing request metadata * docs: clarify (and fix) jsdoc for is*Supported in FetchTool * fix: send Headers to fetch worker safely Without this, the Headers object gets corrupted by postMessage and results in CORS errors complaining about a header field called "map" * test: expose scratchFetch API better to support testing * refactor: store & merge headers using Headers object This code will more directly follow Headers rules. In particular: - Attempting to add metadata with a name that isn't a valid header name will cause an error immediately instead of in `applyMetadata` (likely at fetch time). - Header names are not case sensitive. Using a Headers object internally means that the metadata API will respect that. For example, `setMetadata('a', ...)` will override an earlier value from `setMetadata('A', ...)`. Previously, this happened "accidentally" in `applyMetadata` instead of in the metadata container itself. * test: mock cross-fetch more completely Also emphasize that the `Headers` class used in `scratchFetch.js` comes from `cross-fetch` * refactor: turn mockFetch into a full module mock This should make it harder to make a mistake mocking `cross-fetch` again * test: add test for case sensitivity in metadata keys * fix: rename headers to fit HTTP convention * chore(deps): pin node orb to 5.1.0 * refactor: don't use !arraybuffer-loader! notation * test: wait for checkAsset in load-default-assets tests * test: convert all tests from node-tap to jest * test: mock cross-fetch for download-known-assets, too * test: fix transformer for backslashes Injecting the path into the "code" was causing the backslashes to be interpreted as escape characters instead of path separators. I could have escaped them, but this approach is more similar to what Webpack does with `arraybuffer-loader` and (IMO) easier to read. * feat: control scratchFetch with query param * test: add '?scratchMetadata=1' in metadata test * fix: tell browsers to use browser build * test: verify default asset sizes This checks for things like accidentally using the filename as the file content, etc. * chore: repo rename for npm publish * chore: package-lock update * build: github actions build * build: ci pin node version --------- Co-authored-by: Christopher Willis-Ford <[email protected]> Co-authored-by: Karishma Chadha <[email protected]> Co-authored-by: meyerhot95 <[email protected]> Co-authored-by: meyerhot95 <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

carljbowman assigned kchadha Mar 10, 2022

carljbowman added High Severity priority 3 Low Impact labels Mar 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inconsistent handling of missing costumes (web site vs. desktop app) #367

Inconsistent handling of missing costumes (web site vs. desktop app) #367

cwillisf commented Mar 8, 2022

cwillisf commented Mar 8, 2022

apple502j commented Mar 9, 2022

cwillisf commented Mar 9, 2022

Inconsistent handling of missing costumes (web site vs. desktop app) #367

Inconsistent handling of missing costumes (web site vs. desktop app) #367

Comments

cwillisf commented Mar 8, 2022

Expected Behavior

Actual Behavior

Steps to Reproduce

Screenshots

cwillisf commented Mar 8, 2022

apple502j commented Mar 9, 2022

cwillisf commented Mar 9, 2022