Fix Worker was terminated error when loading is cancelled#20503
Fix Worker was terminated error when loading is cancelled#20503calixteman merged 1 commit intomozilla:masterfrom
Worker was terminated error when loading is cancelled#20503Conversation
Fixes mozilla#11595, where cancelling loading with `loadingTask.destroy()` before it finishes throws a `Worker was terminated` error that CANNOT be caught. When worker is terminated, an error is thrown here: https://github.com/mozilla/pdf.js/blob/6c746260a98766b8ece27018d2c48436cfcafa24/src/core/worker.js#L374 Then `onFailure` runs, in which we throw again via `ensureNotTerminated()`. However, this second error is never caught (and cannot be), resulting in console spam. There is no need to throw any additional errors since the termination is already reported [here](https://github.com/mozilla/pdf.js/blob/6c746260a98766b8ece27018d2c48436cfcafa24/src/core/worker.js#L371-L373), and `onFailure` is supposed to handle errors, not throw them.
|
Is it possible to write a test ? |
@calixteman I think the closest way would be something like the following: Codefunction makeAsyncCallback() {
let resolve;
const promise = new Promise(r => {
resolve = r;
});
const func = function () {
resolve();
};
return { func, promise };
}
function sleep(ms) {
return new Promise(resolve => setTimeout(resolve, ms));
}
function onUnhandledRejection(event) {
fail(event.reason || event);
}
it("no unhandled error is thrown when loading is cancelled", async function () {
if (isNodeJS) {
pending("Worker is not supported in Node.js.");
}
const { func: onProgress, promise: waitForProgress } = makeAsyncCallback();
window.addEventListener("unhandledrejection", onUnhandledRejection);
try {
const loadingTask = getDocument(basicApiGetDocumentParams);
loadingTask.onProgress = onProgress;
await waitForProgress;
await loadingTask.destroy();
// There's probably a better way to wait a bit.
await sleep(1000);
} finally {
window.removeEventListener("unhandledrejection", onUnhandledRejection);
}
});However, it still doesn’t work because triggering this error requires very specific timing. For example, I created a simple reproduction by modifying this file, and I can only get this error when throttling is enabled (probably because the PDF is too small). So i think it won't be possible to write a reliable test. Code<!doctype html>
<html>
<head>
<meta charset="UTF-8" />
<title>'Hello, world!' example</title>
</head>
<body>
<h1>'Hello, world!' example</h1>
<button id="reload">Reload</button>
<br /><br />
<canvas id="the-canvas" style="border: 1px solid black; direction: ltr"></canvas>
<script src="../../build/generic/build/pdf.mjs" type="module"></script>
<script id="script" type="module">
pdfjsLib.GlobalWorkerOptions.workerSrc = "../../build/generic/build/pdf.worker.mjs";
const url = "./helloworld.pdf";
const canvas = document.getElementById("the-canvas");
const ctx = canvas.getContext("2d");
let loadingTask = null;
async function loadDocument() {
if (loadingTask) {
await loadingTask.destroy();
loadingTask = null;
}
loadingTask = pdfjsLib.getDocument({url});
try {
const pdf = await loadingTask.promise;
const page = await pdf.getPage(1);
const scale = 1.5;
const viewport = page.getViewport({ scale });
const dpr = window.devicePixelRatio || 1;
canvas.width = viewport.width * dpr;
canvas.height = viewport.height * dpr;
canvas.style.width = viewport.width + "px";
canvas.style.height = viewport.height + "px";
const transform = dpr !== 1 ? [dpr, 0, 0, dpr, 0, 0] : null;
page.render({
canvasContext: ctx,
viewport,
transform,
}).promise;
} catch (err) {
if (loadingTask?.destroyed) return;
console.error(err);
}
}
// initial load
loadDocument();
document.getElementById("reload").onclick = loadDocument;
</script>
<hr />
<h2>JavaScript code:</h2>
<pre id="code"></pre>
<script>
document.getElementById("code").textContent =
document.getElementById("script").text;
</script>
</body>
</html>Screen.Recording.2026-01-09.at.21.35.16.mov |
|
I think it should be good. |
|
@timvandermeij Any thoughts on this PR? |
timvandermeij
left a comment
There was a problem hiding this comment.
I don't foresee unwanted side-effects of doing this and can follow your reasoning, so this looks good to me assuming all tests pass.
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.193.163.58:8877/d64f89e45248d85/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/0a702a2ae504a94/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/0a702a2ae504a94/output.txt Total script time: 0.28 mins |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/d64f89e45248d85/output.txt Total script time: 83.86 mins
Image differences available at: http://54.193.163.58:8877/d64f89e45248d85/reftest-analyzer.html#web=eq.log |
|
/botio-linux test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/0057f4a07e910df/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/0057f4a07e910df/output.txt Total script time: 0.36 mins |
|
The bot is unhappy with the branch name. |
Fixes #11595, where cancelling loading with
loadingTask.destroy()before it finishes throws aWorker was terminatederror that CANNOT be caught.When worker is terminated, an error is thrown here:
pdf.js/src/core/worker.js
Line 374 in 6c74626
Then
onFailureruns, in which we throw again viaensureNotTerminated(). However, this second error is never caught (and cannot be), resulting in console spam.There is no need to throw any additional errors since the termination is already reported here, and
onFailureis supposed to handle errors, not throw them.