Skip to content
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

Uncaught ReferenceError: ipcApi is not defined #288

Open
kilburn opened this issue May 23, 2021 · 6 comments
Open

Uncaught ReferenceError: ipcApi is not defined #288

kilburn opened this issue May 23, 2021 · 6 comments

Comments

@kilburn
Copy link

kilburn commented May 23, 2021

We've been using electron-pdf for a while. Recently we noticed that PDF generation seemed to be slower than it used to. Inspecting the logs we saw that electron-pdf was logging some error:

(node:17343) UnhandledPromiseRejectionWarning: Error: Script failed to execute, this normally means an error was thrown. Check the renderer console for the error.
    at WebFrame../lib/renderer/api/web-frame.ts.WebFrame.<computed> [as executeJavaScript] (electron/js2c/renderer_init.js:1806:33)
    at electron/js2c/renderer_init.js:3066:43
    at electron/js2c/renderer_init.js:2695:40
    at new Promise (<anonymous>)
    at EventEmitter.<anonymous> (electron/js2c/renderer_init.js:2695:9)
    at EventEmitter.emit (events.js:203:13)
    at Object.onMessage (electron/js2c/renderer_init.js:2469:16)
(node:17343) UnhandledPromiseRejectionWarning: Error: Script failed to execute, this normally means an error was thrown. Check the renderer console for the error.
    at WebFrame../lib/renderer/api/web-frame.ts.WebFrame.<computed> [as executeJavaScript] (electron/js2c/renderer_init.js:1806:33)
    at electron/js2c/renderer_init.js:3066:43
    at electron/js2c/renderer_init.js:2695:40
    at new Promise (<anonymous>)
    at EventEmitter.<anonymous> (electron/js2c/renderer_init.js:2695:9)
    at EventEmitter.emit (events.js:203:13)
    at Object.onMessage (electron/js2c/renderer_init.js:2469:16)

This didn't seem a problem in our code but something in electron or electron-pdf being wrong. After enabling electron's logging (passing --enable-logging to the command), we got the actual javascript error:

[17343:0523/083655.168148:INFO:CONSOLE(3)] "Uncaught ReferenceError: ipcApi is not defined", source: https://127.0.0.1/redacted

This pointed us to the real issue:

  • When electron-pdf prepares a job, it injects the lib/preload.js file, which is supposed to provide an ipcApi object with some methods that call node's internal APIs.
  • For some reason this stopped working at some point (this is the issue)
  • As a result, the view-ready event notification cannot be bridged back to electron-pdf and our PDFs are generated only after the timeout. That's why it got slower for us.

We have been able to reproduce this in several systems, including:

  • Ubuntu 18.04, node v10.24.0, electron 7.3.3, electron-pdf 7.0.1
  • Debian 10, node v10.24.0, electron 7.3.3, electron-pdf 7.0.1
  • Debian 10, node v12.16.3, electron 12.0.9, electron-pdf 10.0.2
@codecounselor
Copy link
Collaborator

That's a good write up, thanks.

My team is actually still on the 4.x branch of electron-pdf. So it's most likely that something in the new(er) version(s) of Electron is causing your issue, because we rely on the ipc and view-ready functionality as well and it's working as expected.

If you are are able to solve the problem I'll happily merge in a PR to the appropriate branch (ideally master)

@kilburn
Copy link
Author

kilburn commented May 24, 2021

We have found the issue. It is only triggered because of our options, explaining why you haven't seen more complaints about it. Let me explain:

  • We have been using electron-pdf since > 3 years ago. When we developed this functionality, the preload option was not being used (it was introduced in Added a partition for the window when cookies are being used  #144). Before then, the scripts injected to the BrowserWindow used electron's ipcRenderer object directly.
  • Because reasons, we had to disable the CORS protections for our case. This meant that we were passing a browserConfig that included the webPreferences key:
     const options = {
         pageSize: "A4",
         outputWait: 7500,
         waitForJSEvent: 'view-ready',
         browserConfig: JSON.stringify({
             webPreferences: {
                 webSecurity: false,
             }
         })
     };
  • Since the _getBrowserConfiguration function does a return _.extend(defaultOpts, cmdLineBrowserConfig) and our cmdLineBrowserConfig included the webPreferences key, our definition completely overrides the default definition from that function, which is what should be injecting the preload.js file (and enabling cookie isolation for that matter).
  • As a result, we get the ipcApi not defined errors becaue the preload.js file hasn't actually been processed!

The easiest way to solve this for our case would be to use _.merge instead of _.extend (so that the webPreferences default definitions get merged instead of overwritten) but this would prevent people from actually overriding the browserConfig options intentionally.

Another possibility would be to load the default options first and then setup the webPreferences on top, because this is tighly related to how electron-pdf works.

Any opinions?

@codecounselor
Copy link
Collaborator

codecounselor commented May 24, 2021

I think my original intent behind the browserConfig option was not to allow the webPreferences to be set.

I do not think we can break backwards compatibility by switching to _.merge.

One other consideration...

Is the webSecurity the only option you need to assign? At one point I did add a similar option to turn node integration on/off

const trustRemoteContent = _.get(this.options, 'trustRemoteContent', false)

We could add a flag for this single option.

const disableWebSecurity = _.get(this.options, 'disableWebSecurity', false)
...
webPreferences: {        
    nodeIntegration: trustRemoteContent,        
    preload: path.join(__dirname, 'preload.js'),
	webSecurity: !disableWebSecurity
}

We could add another CLI option for webPreferences like you suggest which would be most flexible, but encouraging people to couple themselves directly to Electron's API breaks encapsulation a bit more.

@kilburn
Copy link
Author

kilburn commented May 24, 2021

For our particular case we don't need this anymore (we can run with CORS enabled now) so we don't feel strongly about any approach. We are still testing, but it looks like we can now run without this and everything is fine.

I was trying to find a more general solution that allowed users to set any of the webPreference options because there seem to be some useful ones (e.g.: experimentalFeatures, defaultFont*, accessibleTitle). See: https://www.electronjs.org/docs/api/browser-window.

Of course we could create analogous options in electron-pdf for each of them, but this would be a big maintenance burden. That's why I was also proposing to leave out the webPreferences from the defaultOoptions, _extend that with the user's options and then _merge with electron-pdf options. Something like:

  _getBrowserConfiguration (args) {
    const pageDim = WindowTailor.getPageDimensions(args.pageSize, args.landscape)

    const defaultOpts = {
      width: pageDim.x,
      height: pageDim.y,
      enableLargerThanScreen: true,
      show: false,
      center: true, // Display in center of screen,
    }

    let cmdLineBrowserConfig = {}
    try {
      cmdLineBrowserConfig = JSON.parse(args.browserConfig || '{}')
    } catch (e) {
      this.error('Invalid browserConfig provided, using defaults. Value:',
        args.browserConfig,
        '\nError:', e)
    }
    const opts = _.extend(defaultOpts, cmdLineBrowserConfig)

    // This creates a new session for every browser window, otherwise the same
    // default session is used from the main process which would break support
    // for concurrency
    // see http://electron.atom.io/docs/api/browser-window/#new-browserwindowoptions options.partition
    opts.webPreferences.partition = this.jobId

    // This ensures that we define the ipcApi bridge to node's apis
    opts.webPreferences.preload = path.join(__dirname, 'preload.js')

    // This is for backwards compatibility
    opts.webPreferences.trustRemoteContent = _.get(this.options, 'trustRemoteContent', ._get(opts.webPreferences, 'trustRemoteContent', false))

    return opts
  }

As a side-note, notice that just passing webSecurity: false does not disable CORS anymore (see electron/electron#23664 ).

@kilburn
Copy link
Author

kilburn commented May 24, 2021

Or even this, where electron-pdf will only override the webPreferences when they are not setup by users but still allows them to do crazy things such as setting their own preload script:

  _getBrowserConfiguration (args) {
    const pageDim = WindowTailor.getPageDimensions(args.pageSize, args.landscape)

    const defaultOpts = {
      width: pageDim.x,
      height: pageDim.y,
      enableLargerThanScreen: true,
      show: false,
      center: true, // Display in center of screen,
    }

    let cmdLineBrowserConfig = {}
    try {
      cmdLineBrowserConfig = JSON.parse(args.browserConfig || '{}')
    } catch (e) {
      this.error('Invalid browserConfig provided, using defaults. Value:',
        args.browserConfig,
        '\nError:', e)
    }
    const opts = _.extend(defaultOpts, cmdLineBrowserConfig)

    opts.webPreferences = _.merge({
        // This creates a new session for every browser window, otherwise the same
        // default session is used from the main process which would break support
        // for concurrency
        // see http://electron.atom.io/docs/api/browser-window/#new-browserwindowoptions options.partition
        partition: this.jobId,
        // This ensures that we define the ipcApi bridge to node's apis
        preload: path.join(__dirname, 'preload.js'),
        // This is for backwards compatibility
        trustRemoteContent = _.get(this.options, 'trustRemoteContent', false)
    }, opts.webPreferences)

    return opts
  }

@codecounselor
Copy link
Collaborator

Or even this, where electron-pdf will only override the webPreferences when they are not setup by users but still allows them to do crazy things such as setting their own preload script:

  _getBrowserConfiguration (args) {
    const pageDim = WindowTailor.getPageDimensions(args.pageSize, args.landscape)

    const defaultOpts = {
      width: pageDim.x,
      height: pageDim.y,
      enableLargerThanScreen: true,
      show: false,
      center: true, // Display in center of screen,
    }

    let cmdLineBrowserConfig = {}
    try {
      cmdLineBrowserConfig = JSON.parse(args.browserConfig || '{}')
    } catch (e) {
      this.error('Invalid browserConfig provided, using defaults. Value:',
        args.browserConfig,
        '\nError:', e)
    }
    const opts = _.extend(defaultOpts, cmdLineBrowserConfig)

    opts.webPreferences = _.merge({
        // This creates a new session for every browser window, otherwise the same
        // default session is used from the main process which would break support
        // for concurrency
        // see http://electron.atom.io/docs/api/browser-window/#new-browserwindowoptions options.partition
        partition: this.jobId,
        // This ensures that we define the ipcApi bridge to node's apis
        preload: path.join(__dirname, 'preload.js'),
        // This is for backwards compatibility
        trustRemoteContent = _.get(this.options, 'trustRemoteContent', false)
    }, opts.webPreferences)

    return opts
  }

Let's go with this option. If you can also add documentation of the change that would be great.

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

No branches or pull requests

2 participants