fraserxu / electron-pdf

📄 A command line tool to generate PDF from URL, HTML or Markdown files.
MIT License
1.24k stars 137 forks source link

Uncaught ReferenceError: ipcApi is not defined #288

Open kilburn opened 3 years ago

kilburn commented 3 years ago

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:

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

codecounselor commented 3 years ago

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 commented 3 years ago

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:

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 commented 3 years ago

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 https://github.com/fraserxu/electron-pdf/blob/d5332fc160140713d4e0844eed6ab19970e30c4b/lib/exportJob.js#L353

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 commented 3 years ago

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 https://github.com/electron/electron/issues/23664 ).

kilburn commented 3 years ago

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 commented 3 years ago

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.