Renderer subprocess crash on method invocation.

[flashd2n]

Preflight Checklist

• I have read the Contributing Guidelines for this project.
• I agree to follow the Code of Conduct that this project adheres to.
• I have searched the issue tracker for an issue that matches the one I want to file, without success.

Issue Details

• Electron Version:
  • 6.0.10
• Operating System:
  • Windows 10 (1903)
• Last Known Working Electron version:
  • 5.0.10

Expected Behavior

Invoking a method (defined in and provided by the main process) from the renderer process (attached to the global object in the preload script), should work and not crash the renderer process.

Actual Behavior

Invoking such method with simple arguments like a string or a simple object works, but invoking it with a complex nested object (no circular dependency and low in size) causes the renderer process to crash.

To Reproduce

Git Repo URL: https://github.com/flashd2n/electron-submit

$ git clone https://github.com/flashd2n/electron-submit.git
$ npm install
$ npm start

Additional Information

Starting the app will open a single window and it's devtools console. The main process will pass to the renderer an object (reproduce) with a property (invoke) - a method which returns a success message. In the renderer's preload script this object is assigned to the global window object. The spawned window has three buttons:

• Simple - invokes window.reproduce.invoke with a simple string and prints the result
• Medium - invokes window.reproduce.invoke with a medium in complexity object and prints the result
• Complex - invokes window.reproduce.invoke with a high in complex object and prints the result

Simple or Medium will print the success message the the window console, but Complex will crash subprocess and close the window - a crash message is displayed in the main process's console.

This was tried in Electron:

• 5.0.10 - all invocations work.
• 6.0.10 - the complex invocation crashes the subprocess.
• 7.0.0-beta.4 - the complex invocation crashes the subprocess.

[kirilpopov]

Seems like the deep nesting in the data object breaks it.

If the levels of nesting in the data you're transferring are dynamic your app can crash randomly.

[lkonstantinov]

This is broken by the switch from Chrome IPC to Mojo - #17406

Mojo verifies the structure and the size of the message and has a hard limitation of "depth" 100, which due to the way they count + the style of encoding is hit even with this relatively small message :(

The new implementation should've taken this into consideration and pre-encoded (stringifyed?) the message before sending it over Mojo.

[nornagon]

This should be addressed by #20214, but unfortunately that PR won't be backportable, since it's a behavior change.

In the mean time, would it be a reasonable workaround for you to call JSON.stringify() on your object before sending it over IPC?

[lkonstantinov]

Unfortunately, not really - our product consists of an Electron based container + application API.

If we're to make this change in the API, this means that all the applications will need to upgrade once because of the regression in the container and then upgrade again after #20214.

[nornagon]

As a workaround, would it be possible to wrap the IPC methods you're using to automatically stringify/parse? e.g.

const originalSend = ipcRenderer.send
ipcRenderer.send = (channel, ...args) => {
  originalSend(channel, JSON.stringify(args))
}

and similarly on the backend?

Also, do you have a code reference to the place where the Mojo depth limit is enforced? Thanks!

[lkonstantinov]

Mojo will call a validation filter before the sink is invoked with the message. The validation filter traverses the serialization structure and on every element does this:

  ValidationContext::ScopedDepthTracker depth_tracker(validation_context);
  if (validation_context->ExceedsMaxDepth()) {
    ReportValidationError(validation_context,
                          VALIDATION_ERROR_MAX_RECURSION_DEPTH);
    return false;
  }

Check src\mojo\public\cpp\bindings\lib\validation_util.h and src\mojo\public\cpp\bindings\lib\validation_context.h

[Nantris]

I've been tracking down a bug for the past day and finally discovered the ipcRenderer.send() or a relatively small object with a depth of far less than 100. By my count the depth is 25, and the total length of the stingified object is just 1933 characters. Worked fine on Electron@4.x, but crashes 6.0.11.

Stringifying ourselves isn't the end of the world, but I'm confused about why I need to stringify for such a small and shallow object. Is this expected behavior? It seems like no.

Relatedly, is there any way to warn the user about what caused the crash?

[nornagon]

@slapbox do you have a repro? it may be that there are additional layers introduced by the base::Value serialization such that the depth-100 limit is reached for even a smaller object.

Also, would you be able to try it out on the Electron 8 beta series? Electron 8 now uses the new Structured Clone-based IPC, so it should hopefully be able to handle much deeper objects.

[Nantris]

@nornagon thanks for your (very speedy) reply. I'll investigate both things as soon as time allows.

[kureus]

@nornagon Following your question to @slapbox to test against Electron 8, I also encountered this issue with marktext/marktext#1797 and building against 8.0.0 beta 4 saw the crash disappear.

[Nantris]

Haha that's the exact same issue we're seeing, 5 list items crashes the app unless stringificatied before sending over ipc. I can't test Electron 8 yet, but it seems extremely likely that it will fix the issue for me as well. Thanks @kureus for your report.

[NilSet]

Not just windows, this reproduces on mac as well.

[NilSet]

Is it possible to catch the mojo validation error in electron 6 and 7? Unsure what the behavior should be, but crashing the renderer processes is not it.

[Nantris]

@NilSet - you can try Devtron, but I don't know if the IPC call that crashes things will be reflected on there. Your best bet is to figure out the action that caused the crash, and then look for places where you are making related IPC calls. Anywhere that might be an object of substantial depth should be stringified. If Devtron doesn't help, and I expect it won't, I have no other ideas.

[nornagon]

#21922 ought to resolve this, I hope! At least, to the extent that it ought to behave much closer to the pre-Mojo version vis-a-vis how the values are serialized under the hood. I tested @flashd2n's repro with that patch applied and was able to successfully send the most nested object in that repro, which is ~10 levels deep.

[NilSet]

will @nornagon 's fix be backported to 6 as well?

[lincolnthree]

Hey all. This is still failing for me in Electron 8.x. Is there a known workaround beyond using JSON.stringify() for all IPC?

[nornagon]

@lincolnthree do you have a fiddle that reproduces the issue? This shouldn't be happening on 8.x.

[lincolnthree]

I don't. Let me see if I can make a more minimal repro without some proprietary stuff. Currently updating a very large app.

[lincolnthree]

Okay @nornagon, I have done quite a bit more research, and I believe the issue is actually not the IPC calls, rather, Service Worker integration when nodeIntegrationInWorker=true.

I believe I hit the following issue, as disabling nodeIntegrationInWorker (in an effort to minimize the application I packed up for you, of course, yay!) stabilized the application:

https://stackoverflow.com/questions/59107643/how-can-i-register-a-service-worker-in-electron-when-node-integration-is-enabled

#19150

Please disregard for this issue. I initially had seen success doing stringification, but I think I had probably disabled the service worker integration somehow during my modifications (or was running in DEV mode, where SW is disabled by default.)

[nornagon]

Ok, cool, I'll close this issue then as I believe it's been resolved.