3

Bug 1649605, part 1: provide async support for promise workers

 2 years ago
source link: https://phabricator.services.mozilla.com/D113152
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Bug 1649605, part 1: provide async support for promise workers
ClosedPublic
Authored by emalysz on Apr 22 2021, 5:18 PM.
Diff Detail

Event Timeline

phab-bot published this revision for review.Apr 22 2021, 5:18 PM
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
toolkit/components/promiseworker/worker/PromiseWorker.js45

Wasn't sure if the DOMException part should be left here or the other patch. It's not necessary for async calls, but I could also rebase the PageThumbs patch onto this and land that while debugging the SessionFile patch

Comment Actions

Someone who knows more about PromiseWorker should probably review this too.

toolkit/components/promiseworker/worker/PromiseWorker.js45

Putting it in the patch that uses it maybe makes more sense.

175–178

Can't we just check else if (exn.constructor.name === "DOMException") ? The first check seems redundant.

emalysz marked an inline comment as done.
asuth accepted this revision.May 3 2021, 8:31 PM
Comment Actions

This is consistent with discussions we've had about changes in this area previously. (Specifically, that if we want unhandled rejections to "bubble" to the Worker binding in the parent, we need to help the runtime by converted "unhandledrejection" to an outright thrown error.

At a meta level, I do think it could probably make sense to have comment preceding these self.addEventListener boilerplate lines to indicate that it's boilerplate and where the documentation is on what boilerplate should be used, etc. Or perhaps better yet, have the PromiseWorker class have a method like bindToGlobal(global) and have it do all the addEventListener calls itself. But that seems like it would want to be an optional follow-up.

This revision is now accepted and ready to land.May 3 2021, 8:31 PM
Comment Actions

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!

asuth added 1 blocking reviewer(s): barret.May 3 2021, 8:33 PM
Comment Actions

Upgrading Barret to a blocking reviewer because I think the intent is that Barret still (re)reviews this, but phabricator won't show it as needing to be reviewed for non-blocking reviewers once there's any review.

This revision now requires review to proceed.May 3 2021, 8:33 PM
This revision is now accepted and ready to land.Thu, May 6, 6:12 PM
Revision Contents
PathSizeCoverage (All)Coverage (Touched)
browser/
components/
newtab/
PersonalityProvider/
3 lines--3 lines96%50%
sessionstore/
3 lines72%50%
toolkit/
components/
osfile/
modules/
3 lines82%100%
promiseworker/
tests/
xpcshell/
data/
3 lines--
worker/
9 lines--
reader/
3 lines87%50%
thumbnails/
3 lines61%100%
Diff 436470

browser/components/newtab/lib/PersonalityProvider/PersonalityProviderWorker.js

Show All 29 Linesworker.postMessage = function(message, ...transfers) { worker.postMessage = function(message, ...transfers) { self.postMessage(message, ...transfers); self.postMessage(message, ...transfers); }; }; worker.close = function() { worker.close = function() { self.close(); self.close(); }; };

self.addEventListener("message", msg => worker.handleMessage(msg)); self.addEventListener("message", msg => worker.handleMessage(msg)); self.addEventListener("unhandledrejection", function(error) { throw error.reason; });

browser/components/newtab/lib/cache-worker.js

Show First 20 LinesShow All 185 Lines▼ Show 20 Linesworker.postMessage = function(result, ...transfers) { worker.postMessage = function(result, ...transfers) { self.postMessage(result, ...transfers); self.postMessage(result, ...transfers); }; }; worker.close = function() { worker.close = function() { self.close(); self.close(); }; };

self.addEventListener("message", msg => worker.handleMessage(msg)); self.addEventListener("message", msg => worker.handleMessage(msg)); self.addEventListener("unhandledrejection", function(error) { throw error.reason; });

browser/components/sessionstore/SessionWorker.js

Show All 24 Linesworker.postMessage = function(result, ...transfers) { worker.postMessage = function(result, ...transfers) { self.postMessage(result, ...transfers); self.postMessage(result, ...transfers); }; }; worker.close = function() { worker.close = function() { self.close(); self.close(); }; };

self.addEventListener("message", msg => worker.handleMessage(msg)); self.addEventListener("message", msg => worker.handleMessage(msg)); self.addEventListener("unhandledrejection", function(error) { throw error.reason; });

// The various possible states // The various possible states

/** /** * We just started (we haven't written anything to disk yet) from * We just started (we haven't written anything to disk yet) from * `Paths.clean`. The backup directory may not exist. * `Paths.clean`. The backup directory may not exist. */ */ const STATE_CLEAN = "clean"; const STATE_CLEAN = "clean"; ▲ Show 20 LinesShow All 374 LinesShow Last 20 Lines

toolkit/components/osfile/modules/osfile_async_worker.js

Show First 20 LinesShow All 54 Lines▼ Show 20 Lines worker.postMessage = function(message, ...transfers) { self.postMessage(message, ...transfers); self.postMessage(message, ...transfers); }; }; worker.close = function() { worker.close = function() { self.close(); self.close(); }; }; let Meta = PromiseWorker.Meta; let Meta = PromiseWorker.Meta;

self.addEventListener("message", msg => worker.handleMessage(msg)); self.addEventListener("message", msg => worker.handleMessage(msg)); self.addEventListener("unhandledrejection", function(error) { throw error.reason; });

/** /** * A data structure used to track opened resources * A data structure used to track opened resources */ */ let ResourceTracker = function ResourceTracker() { let ResourceTracker = function ResourceTracker() { // A number used to generate ids // A number used to generate ids this._idgen = 0; this._idgen = 0; // A map from id to resource // A map from id to resource ▲ Show 20 LinesShow All 405 LinesShow Last 20 Lines

toolkit/components/promiseworker/tests/xpcshell/data/worker.js

Show All 18 Lines}; }; worker.close = function() { worker.close = function() { self.close(); self.close(); }; }; worker.log = function(...args) { worker.log = function(...args) { dump("Worker: " + args.join(" ") + "\n"); dump("Worker: " + args.join(" ") + "\n"); }; }; self.addEventListener("message", msg => worker.handleMessage(msg)); self.addEventListener("message", msg => worker.handleMessage(msg)); self.addEventListener("unhandledrejection", function(error) { throw error.reason; });

var Agent = { var Agent = { bounce(...args) { bounce(...args) { return args; return args; }, },

throwError(msg, ...args) { throwError(msg, ...args) { throw new Error(msg); throw new Error(msg); }, }, }; };

toolkit/components/promiseworker/worker/PromiseWorker.js

Show All 36 Linesconst EXCEPTION_NAMES = { const EXCEPTION_NAMES = { EvalError: "EvalError", EvalError: "EvalError", InternalError: "InternalError", InternalError: "InternalError", RangeError: "RangeError", RangeError: "RangeError", ReferenceError: "ReferenceError", ReferenceError: "ReferenceError", SyntaxError: "SyntaxError", SyntaxError: "SyntaxError", TypeError: "TypeError", TypeError: "TypeError", URIError: "URIError", URIError: "URIError", }; };

Wasn't sure if the DOMException part should be left here or the other patch. It's not necessary for async calls, but I could also rebase the PageThumbs patch onto this and land that while debugging the SessionFile patch

Putting it in the patch that uses it maybe makes more sense.

/** /** * A constructor used to return data to the caller thread while * A constructor used to return data to the caller thread while * also executing some specific treatment (e.g. shutting down * also executing some specific treatment (e.g. shutting down * the current thread, transmitting data instead of copying it). * the current thread, transmitting data instead of copying it). * * * @param {object=} data The data to return to the caller thread. * @param {object=} data The data to return to the caller thread. * @param {object=} meta Additional instructions, as an object * @param {object=} meta Additional instructions, as an object Show All 30 Lines * } * } * } * } * * * By default, the AbstractWorker is not connected to a message port, * By default, the AbstractWorker is not connected to a message port, * hence will not receive anything. * hence will not receive anything. * * * To connect it, use `onmessage`, as follows: * To connect it, use `onmessage`, as follows: * self.addEventListener("message", msg => myWorkerInstance.handleMessage(msg)); * self.addEventListener("message", msg => myWorkerInstance.handleMessage(msg)); * To handle rejected promises we receive from handleMessage, we must connect it to * the onError handler as follows: * self.addEventListener("unhandledrejection", function(error) { * throw error.reason; * }); */ */ function AbstractWorker(agent) { function AbstractWorker(agent) { this._agent = agent; this._agent = agent; } } AbstractWorker.prototype = { AbstractWorker.prototype = { // Default logger: discard all messages // Default logger: discard all messages log() {}, log() {},

/** /** * Handle a message. * Handle a message. */ */ handleMessage(msg) { async handleMessage(msg) { let data = msg.data; let data = msg.data; this.log("Received message", data); this.log("Received message", data); let id = data.id; let id = data.id;

let start; let start; let options; let options; if (data.args) { if (data.args) { options = data.args[data.args.length - 1]; options = data.args[data.args.length - 1]; Show All 9 Lines async handleMessage(msg) { } }

let result; let result; let exn; let exn; let durationMs; let durationMs; let method = data.fun; let method = data.fun; try { try { this.log("Calling method", method); this.log("Calling method", method); result = this.dispatch(method, data.args); result = await this.dispatch(method, data.args); this.log("Method", method, "succeeded"); this.log("Method", method, "succeeded"); } catch (ex) { } catch (ex) { exn = ex; exn = ex; this.log( this.log( "Error while calling agent method", "Error while calling agent method", method, method, exn, exn, exn.moduleStack || exn.stack || "" exn.moduleStack || exn.stack || "" Show All 24 Lines if (!exn) { } } if (result.meta.shutdown || false) { if (result.meta.shutdown || false) { // Time to close the worker // Time to close the worker this.close(); this.close(); } } } else { } else { this.postMessage({ ok: result, id, durationMs }); this.postMessage({ ok: result, id, durationMs }); } } } else if (exn.constructor.name in EXCEPTION_NAMES) { } else if (exn.constructor.name in EXCEPTION_NAMES) { // Rather than letting the DOM mechanism [de]serialize built-in // Rather than letting the DOM mechanism [de]serialize built-in // JS errors, which loses lots of information (in particular, // JS errors, which loses lots of information (in particular, // the constructor name, the moduleName and the moduleStack), // the constructor name, the moduleName and the moduleStack),

Can't we just check else if (exn.constructor.name === "DOMException") ? The first check seems redundant.

// we [de]serialize them manually with a little more care. // we [de]serialize them manually with a little more care. this.log("Sending back exception", exn.constructor.name, "id is", id); this.log("Sending back exception", exn.constructor.name, "id is", id); let error = { let error = { exn: exn.constructor.name, exn: exn.constructor.name, message: exn.message, message: exn.message, fileName: exn.moduleName || exn.fileName, fileName: exn.moduleName || exn.fileName, lineNumber: exn.lineNumber, lineNumber: exn.lineNumber, stack: exn.moduleStack, stack: exn.moduleStack, ▲ Show 20 LinesShow All 42 LinesShow Last 20 Lines

toolkit/components/reader/ReaderWorker.js

Show All 31 Lines}; }; worker.log = function(...args) { worker.log = function(...args) { if (DEBUG) { if (DEBUG) { dump("ReaderWorker: " + args.join(" ") + "\n"); dump("ReaderWorker: " + args.join(" ") + "\n"); } } }; };

self.addEventListener("message", msg => worker.handleMessage(msg)); self.addEventListener("message", msg => worker.handleMessage(msg)); self.addEventListener("unhandledrejection", function(error) { throw error.reason; });

var Agent = { var Agent = { /** /** * Parses structured article data from a document. * Parses structured article data from a document. * * * @param {object} uri URI data for the document. * @param {object} uri URI data for the document. * @param {string} serializedDoc The serialized document. * @param {string} serializedDoc The serialized document. * @param {object} options Options object to pass to Readability. * @param {object} options Options object to pass to Readability. * * * @return {object} Article object returned from Readability. * @return {object} Article object returned from Readability. */ */ parseDocument(uri, serializedDoc, options) { parseDocument(uri, serializedDoc, options) { let doc = new JSDOMParser().parse(serializedDoc, uri.spec); let doc = new JSDOMParser().parse(serializedDoc, uri.spec); return new Readability(doc, options).parse(); return new Readability(doc, options).parse(); }, }, }; };

toolkit/components/thumbnails/PageThumbsWorker.js

Show All 26 Linesworker.postMessage = function(message, ...transfers) { worker.postMessage = function(message, ...transfers) { self.postMessage(message, ...transfers); self.postMessage(message, ...transfers); }; }; worker.close = function() { worker.close = function() { self.close(); self.close(); }; };

self.addEventListener("message", msg => worker.handleMessage(msg)); self.addEventListener("message", msg => worker.handleMessage(msg)); self.addEventListener("unhandledrejection", function(error) { throw error.reason; });

var Agent = { var Agent = { // Checks if the specified file exists and has an age less than as // Checks if the specified file exists and has an age less than as // specifed (in seconds). // specifed (in seconds). isFileRecent: function Agent_isFileRecent(path, maxAge) { isFileRecent: function Agent_isFileRecent(path, maxAge) { try { try { let stat = OS.File.stat(path); let stat = OS.File.stat(path); let maxDate = new Date(); let maxDate = new Date(); ▲ Show 20 LinesShow All 138 LinesShow Last 20 Lines


About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK