Don't crash when calling stale remote listeners #8357

CharlieHess · 2017-01-06T21:33:59Z

When Electron tries to call a method in a deceased renderer it throws an exception. This behavior makes the remote module a bit of a hornet's nest, because it's quite easy to listen to events from the main process but often non-trivial to clean them up properly. Instead of crashing, we could dig into the underlying EventEmitter and remove the listeners that are hooked to callIntoRenderer. We could also display a console.warn so that the user knows they did something bad and still has a chance to fix it.

In addition this PR adds the names of the remote events to the error message, to make them easier to find. Increasingly often methods are wrapped in higher-level abstractions such as Promise or Observable so meta.location is not enough information on its own. The new warning looks like:

Attempting to call a function in a renderer window that has been closed or released.
Function provided here: remote-event-handler.html:11:33
Remote event names: remote-handler, other-remote-handler

kevinsawicki · 2017-01-09T16:38:42Z

This seems helpful, could you add a spec for this as well?

kevinsawicki · 2017-01-09T16:39:25Z

lib/browser/rpc-server.js

+      : eventsAttached[eventName] === callIntoRenderer
+  })
+
+  if (remoteEvents && remoteEvents.length > 0) {


I don't think Array.prototype.filter can return null so I think you can drop the first part of this check and just use remoteEvent.length > 0

kevinsawicki · 2017-01-09T16:41:49Z

lib/browser/rpc-server.js

+  let message = `Attempting to call a function in a renderer window that has been closed or released.` +
+    `\nFunction provided here: ${meta.location}`
+
+  if (!args || args.length === 0) return message


args will never be passed as null by the calling method so I think you can just use arg.length === 0 here.

kevinsawicki · 2017-01-09T16:42:16Z

lib/browser/rpc-server.js

@@ -146,6 +146,30 @@ const throwRPCError = function (message) {
  throw error
 }

+const rendererMissingErrorMessage = function (meta, args, callIntoRenderer) {


Since this is a new method, feel free to make this a => function if you'd like.

zeke · 2017-01-11T04:56:38Z

lib/browser/rpc-server.js

+    message += `\nRemote event names:`
+    remoteEvents.forEach((eventName) => {
+      message += ` ${eventName} `
+    })


This could be simplified to:

message += `\nRemote event names: ${remoteEvents.join(' ')}`

zeke · 2017-01-11T04:57:51Z

This is great! Should save people time and energy when debugging. 👍

CharlieHess · 2017-01-11T23:38:24Z

@kevinsawicki updated with your comments. As for the spec, I'm happy to write one but unsure where it should go? rpc-server doesn't have any specs; I could test this method in isolation but I dunno how meaningful those tests would be.

kevinsawicki · 2017-01-12T00:54:52Z

As for the spec, I'm happy to write one but unsure where it should go?

Usually they go in api-ipc-spec.js.

See #6963 for a recent pull request with a test example.

CharlieHess · 2017-01-12T23:08:38Z

@kevinsawicki I was able to get a spec in for the Attempting to call a function in a renderer window that has been closed or released error message, but only when it was wrapped by rpc-server.callFunction. Had a tougher time catching the exception from within the page (which is where we could check the events 😞).

kevinsawicki · 2017-01-12T23:12:21Z

Had a tougher time catching the exception from within the page (which is where we could check the events 😞).

I'll take a look 👀

kevinsawicki · 2017-01-13T00:31:45Z

lib/browser/rpc-server.js

+  if (args.length === 0) return message
+  if (!args[0].sender || !args[0].sender._events) return message
+
+  const eventsAttached = args[0].sender._events


Could this use https://nodejs.org/api/events.html#events_emitter_eventnames to get an array of event names?

We could, but we'll get all the events. We need to compare the event handler with callIntoRenderer to know if it's a remote listener, so we'll still have to iterate.

kevinsawicki · 2017-01-13T00:33:09Z

@CharlieHess I updated the spec, let me know what you think.

CharlieHess · 2017-01-13T01:06:33Z

@kevinsawicki new spec looks good, thanks! ✨

kevinsawicki · 2017-01-16T17:00:37Z

lib/browser/rpc-server.js

+  if (remoteEvents.length > 0) {
+    message += `\nRemote event names: ${remoteEvents.join(', ')}`
+    remoteEvents.forEach((eventName) => {
+      sender.removeListener(eventName, callIntoRenderer)


Would be great to assert this happens as well in the spec, perhaps by asserting listenerCount() goes down for each event name being listened to.

kevinsawicki · 2017-01-16T17:01:51Z

lib/browser/rpc-server.js

@@ -146,6 +146,28 @@ const throwRPCError = function (message) {
  throw error
 }

+const rendererMissingErrorMessage = (meta, args, callIntoRenderer) => {


Since this method now removes the listeners as well, I think perhaps it should be renamed and do the console.warn directly in here.

Maybe a name like logMissingRendererMessageAndRemoveListeners?

If possible, we'll dig into the function args and print the ones that are attached remotely.

kevinsawicki · 2017-01-25T22:23:54Z

Thanks for this @CharlieHess 👍

CharlieHess added the enhancement ✨ label Jan 6, 2017

kevinsawicki reviewed Jan 9, 2017

View reviewed changes

zeke reviewed Jan 11, 2017

View reviewed changes

kevinsawicki reviewed Jan 13, 2017

View reviewed changes

kevinsawicki force-pushed the remote-events-error-message branch from 1b979b7 to 55a245f Compare January 13, 2017 00:35

CharlieHess changed the title ~~Display more information about remote event crashes~~ Don't crash when calling stale remote listeners Jan 13, 2017

kevinsawicki reviewed Jan 16, 2017

View reviewed changes

kevinsawicki force-pushed the remote-events-error-message branch from 388e8b8 to 08b8ebd Compare January 24, 2017 23:05

CharlieHess and others added 11 commits January 25, 2017 12:36

Display more information about remote event crashes.

26e3ad3

If possible, we'll dig into the function args and print the ones that are attached remotely.

L I N T I N G

49c6446

L I N T I N G P T I I

41ea169

Review CommentZ

67f7a60

Simplify.

a0b24bd

Add a spec for the "renderer window closed" error.

b04db2e

Assert remote event names

4598640

Use EventEmitter public methods instead of _events.

63d8137

Argh.

bc2f1e8

Write a warning instead of crashing.

c213971

Assert that listener count decreases after a remove event.

f6410d3

CharlieHess and others added 3 commits January 25, 2017 12:36

Move console.warn inside the helper method.

ddedcf2

Check that sender is a WebContents instance

56a8eb3

Fix standard linting error

330ac5f

kevinsawicki force-pushed the remote-events-error-message branch from f364298 to 330ac5f Compare January 25, 2017 20:37

kevinsawicki merged commit 8e69f94 into master Jan 25, 2017

kevinsawicki deleted the remote-events-error-message branch January 25, 2017 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't crash when calling stale remote listeners #8357

Don't crash when calling stale remote listeners #8357

CharlieHess commented Jan 6, 2017 •

edited

kevinsawicki commented Jan 9, 2017

kevinsawicki Jan 9, 2017

kevinsawicki Jan 9, 2017

kevinsawicki Jan 9, 2017

zeke Jan 11, 2017

zeke commented Jan 11, 2017

CharlieHess commented Jan 11, 2017

kevinsawicki commented Jan 12, 2017

CharlieHess commented Jan 12, 2017

kevinsawicki commented Jan 12, 2017

kevinsawicki Jan 13, 2017

CharlieHess Jan 13, 2017

kevinsawicki commented Jan 13, 2017

CharlieHess commented Jan 13, 2017

kevinsawicki Jan 16, 2017

kevinsawicki Jan 16, 2017 •

edited

kevinsawicki commented Jan 25, 2017

Don't crash when calling stale remote listeners #8357

Don't crash when calling stale remote listeners #8357

Conversation

CharlieHess commented Jan 6, 2017 • edited

kevinsawicki commented Jan 9, 2017

kevinsawicki Jan 9, 2017

Choose a reason for hiding this comment

kevinsawicki Jan 9, 2017

Choose a reason for hiding this comment

kevinsawicki Jan 9, 2017

Choose a reason for hiding this comment

zeke Jan 11, 2017

Choose a reason for hiding this comment

zeke commented Jan 11, 2017

CharlieHess commented Jan 11, 2017

kevinsawicki commented Jan 12, 2017

CharlieHess commented Jan 12, 2017

kevinsawicki commented Jan 12, 2017

kevinsawicki Jan 13, 2017

Choose a reason for hiding this comment

CharlieHess Jan 13, 2017

Choose a reason for hiding this comment

kevinsawicki commented Jan 13, 2017

CharlieHess commented Jan 13, 2017

kevinsawicki Jan 16, 2017

Choose a reason for hiding this comment

kevinsawicki Jan 16, 2017 • edited

Choose a reason for hiding this comment

kevinsawicki commented Jan 25, 2017

CharlieHess commented Jan 6, 2017 •

edited

kevinsawicki Jan 16, 2017 •

edited