Skip to content

QuickJS: fix error reporting in query server#5943

Open
Karthikeya1500 wants to merge 7 commits intoapache:mainfrom
Karthikeya1500:fix-quickjs-error-reporting
Open

QuickJS: fix error reporting in query server#5943
Karthikeya1500 wants to merge 7 commits intoapache:mainfrom
Karthikeya1500:fix-quickjs-error-reporting

Conversation

@Karthikeya1500
Copy link
Copy Markdown
Contributor

QuickJS: fix error reporting in query server

Problem

The QuickJS query server fails to report useful error messages when standard JavaScript errors or non-Error objects (like strings) are thrown outside of captured sandboxes.

  1. For standard Error objects: JSON.stringify(e) results in an empty object {}. The client receives ["error", "SomeError", {}] instead of the error details.
  2. For non-Error objects (e.g., throw "foo"): The server attempts to access e.stack, which is undefined on strings/primitives. This results in ["error", "unnamed_error", null].

Solution

Use the existing errstr(e) utility (already defined in util.js) to ensure that a descriptive string representation of the error is sent back to the client. This brings the QuickJS implementation in line with the SpiderMonkey behavior in loop.js and other parts of the codebase.

Testing

Verified that errstr(e) correctly identifies and stringifies both standard Error instances and primitive values thrown by user code.

If an unrecognized command is processed by globalThis.dispatch,
the error handler attempts to reference cmdkey without it
being defined. This throws a ReferenceError instead of generating
the intended CouchDB fatal error array.

This commit assigns cmd.shift() to a const variable cmdkey before
evaluating the switch statement.
Use the errstr(e) utility in handleError to ensure meaningful
error messages are returned to the client for both named and
unnamed errors.

- Replace e.stack with errstr(e) for unnamed errors
- Replace raw Error object with errstr(e) for named errors
Copy link
Copy Markdown
Contributor

@nickva nickva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Karthikeya1500!

errstr(e) for QuickJS will just call e.toString(). For Spidermonkey it handles a special case for an a SM-only function. So we could just define local QuickJS helper here or just call e.toString().

For some errors which are not Error objects, e.toString() or errstr(e) will fail I think? We check above for null as a special case, but I guess undefined also won't have e.toString() so maybe in a helper function we should check for that. Symbols may another thing which won't work well.

Wonder something like this could work:

try {`${e} stack: ${e.stack}`} catch {JSON.stringify(e)};

That is, we try to format both the error (its type and name) and the stack as a string, and if that fails, say, we get an undefined then we return what we did before.

Ideally we'd add test cases like the one we added in the previous pull request. Since we may have some behavior divergence between Spidermonkey and QuickJS we may have to use a different check depending on what engine is being used like we do here: https://github.com/apache/couchdb/blob/main/src/couch/test/eunit/couch_js_tests.erl#L445-L467

Also, please fill out the regular PR template we have for pull requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants