New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Pass on errors thrown in `executeJavaScript` #11158

Merged
merged 5 commits into from Nov 20, 2017

Conversation

Projects
None yet
4 participants
@felixrieseberg
Member

felixrieseberg commented Nov 18, 2017

When you reject a Promise from a executeJavaScript call, we pass on the rejection. That's good! However, if you write JavaScript as recommended by the pros, you'll be rejecting with an instance of Error, which results in an broken result of {}.

The reason is simple: Error objects don't serialize. This is true for all of them (ReferenceError, TypeError, URIError, etc).

const myError = new Error('Something went wrong')
JSON.stringify(myError) === '{}' // true

The result is this, which makes no sense to developers who haven't studied Error objects and the IPC:

webContents.executeJavaScript('Promise.reject(new Error("Something went wrong"));')
  .catch((error) => {
     console.log(error) // This will log '{}'
  })

This PR provides a fix: Let's serialize known and common Error objects for these calls in the renderer and recreate them in the main process. Also: Tests!

With this PR:

webContents.executeJavaScript('Promise.reject(new Error("Something went wrong"));')
  .catch((error) => {
     console.log(error)
     // This will now log:
     // Error: Something went wrong
     //     at <anonymous>:1:16
     //     at EventEmitter.electron.ipcRenderer.on(...
     //     ...
  })

Closes #11155

@felixrieseberg felixrieseberg requested a review from electron/reviewers as a code owner Nov 18, 2017

@felixrieseberg felixrieseberg requested a review from MarshallOfSound Nov 18, 2017

@MarshallOfSound

Looks good to me, only concern is the magical __ELECTRON_SERIALIZED_ERROR__ but I can't come up with a better way of doing it without diving into our IPC / serialization stuff and we don't want to do that 😄

@@ -120,7 +130,18 @@ const asyncWebFrameMethods = function (requestId, method, callback, ...args) {
if (typeof callback === 'function') callback(result)
resolve(result)
} else {
reject(error)
if (error && error.__ELECTRON_SERIALIZED_ERROR__) {

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Nov 18, 2017

Contributor

error is guaranteed to be not null nor undefined here, do you really need if (error &&?

if (error && error.__ELECTRON_SERIALIZED_ERROR__) {
let rehydratedError = error
if (errorConstructors[error.name]) {

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Nov 18, 2017

Contributor

I guess this check can be moved to a condition above, so the whole change will be:

} else {
  if (/* is a serialized error */) {
    const rehydratedError = new errorConstructors[error.name](error.message)
    rehydratedError.stack = error.stack
    reject(rehydratedError)
  } else {
    reject(error)
  }
}
RangeError: 'Promise.reject(new RangeError("Wamp-wamp"))',
SyntaxError: 'Promise.reject(new SyntaxError("Wamp-wamp"))',
TypeError: 'Promise.reject(new TypeError("Wamp-wamp"))',
URIError: 'Promise.reject(new URIError("Wamp-wamp"))'

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Nov 18, 2017

Contributor

Maybe calls of Promise.reject() should be part of the test, not the list of error types?

const errorTypes = new Set([
  Error,
  ReferenceError,
  <...>
])

<...>

for (const ErrorType of errorTypes) {
  <...>
  ipcRenderer.send('executeJavaScript', `Promise.reject(new ${ErrorType.name}("Wamp-wamp"))`, true)
  <...>
}
@@ -217,6 +217,10 @@ app.on('ready', function () {
window.webContents.send('executeJavaScript-promise-response', result)
}).catch((error) => {
window.webContents.send('executeJavaScript-promise-error', error)
if (error && error.name) {
window.webContents.send('executeJavaScript-promise-error-name', error.name)

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Nov 18, 2017

Contributor

Do you need this only for tests? Can the 'executeJavaScript-promise-error' message be used instead?

This comment has been minimized.

@felixrieseberg

felixrieseberg Nov 18, 2017

Member

Sadly not (but this is a wonderful example of how un-intuitive this is) - if we send the received error over the IPC again, we'll get {} again 😆

@ckerr ckerr merged commit 5eb00e4 into master Nov 20, 2017

6 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@ckerr ckerr deleted the execute-errors branch Nov 20, 2017

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