Fixes #750 - Throw error with stack trace when calling launch with an…

[austinkelleher]

… invalid executablePath option.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
• In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

[austinkelleher]

I signed it!

[googlebot]

CLAs look good, thanks!

[aslushnikov]

Good CL, thank you!

[aslushnikov]

let's add jsdoc:

/**
 * @param {string} chromeExecutable
 * @return {!Promise}
 */

[aslushnikov]

you want to extend default options with executable path:

const launchOptions = Object.assign({ executablePath: 'random-invalid-path' }, defaultBrowserOptions);

[austinkelleher]

I made a small change, but I don't think we should have { executablePath: ... } as the first argument to Object.assign(...) because if process.env.CHROME is supplied, then the executablePath supplied in Object.assign(...) will be overridden causing this test to fail.

const options = Object.assign({}, defaultBrowserOptions, {executablePath: 'random-invalid-path'});

https://github.com/GoogleChrome/puppeteer/pull/751/files#diff-c1129c8b045390789fa8ff62f2c6b4a9R111

[austinkelleher]

@aslushnikov Thanks for the feedback. I made the changes and squashed.

[JoelEinbinder]

The executablePath actually takes a command right now. So you can set executablePath to "chrome-unstable" and it will fail this check but run correctly. Instead I think we should check for an error on childProcess.spawn.

[austinkelleher]

@JoelEinbinder You're right. I will make the changes soon. Thanks.

[austinkelleher]

Hey, so I was looking at attaching an error event listener to the spawned process (here: https://github.com/GoogleChrome/puppeteer/blob/master/lib/Launcher.js#L81), but I can't really think of a good way to reject with an error back to the puppeteer.launch(...) caller without introducing a timeout. Something like this (untested):

static async launch(options) {
  ...
  const chromeProcess = childProcess.spawn(chromeExecutable, chromeArguments, {detached: true});

  await new Promise((resolve, reject) => {
    let timeout = setTimeout(resolve, 1000);
    chromeProcess.on('error', () => {
      clearTimeout(timeout);
      reject(new Error(`Error launching Chromium process with executable path "${chromeExecutable}"`));
    });
  });
  ...
}

I don't really like the idea of introducing a timer, but I'm not sure what the best way of getting around it is.

We could simply just console.error inside of the chromeProcess.on('error') handler, but I'm not a huge fan of that either.

Thoughts @aslushnikov @JoelEinbinder?

[JoelEinbinder]

You can race chromeProcess.on('error') against waitForWSEndpoint. If the process sends us a valid websocket endpoint, I think we can safely assume Chrome launched successfully.

[austinkelleher]

Thanks @JoelEinbinder. Sounds good. I will have the fix up soon.

[aslushnikov]

@austinkelleher any updates on this?

[austinkelleher]

Hey, @aslushnikov. I apologize for the delay on PR feedback. Got caught up in some other things. Should be able to get to this later today.

[aslushnikov]

@austinkelleher thanks and no rush, I was mostly curious if you still have interest in this PR.

[austinkelleher]

Whoops I need to make a few more changes. Hold on.

[austinkelleher]

@aslushnikov @JoelEinbinder Thanks again for the feedback. I've addressed them all.

[austinkelleher]

@aslushnikov @JoelEinbinder Can you please review my changes? Thanks!

[aslushnikov]

@austinkelleher I apologize for the long reply, I'm currently traveling.

Codewise, I think this solution is a little too involved. It should be enough to catch the 'error' event on the chrome process - check out the #863.

[aslushnikov]

Closing in favor of #863