diff --git a/lib/Local.js b/lib/Local.js index 8f783d7..9d482b6 100644 --- a/lib/Local.js +++ b/lib/Local.js @@ -57,7 +57,7 @@ function Local(){ else return new LocalError('No output received'); if(data['state'] != 'connected'){ - return new LocalError(data['message']['message']); + return new LocalError(that.getErrorMessage(data)); } else { that.pid = data['pid']; that.isProcessRunning = true; @@ -112,19 +112,25 @@ function Local(){ return; } else { callback(new LocalError(error.toString())); + return; } } var data = {}; - if(stdout) - data = JSON.parse(stdout); - else if(stderr) - data = JSON.parse(stderr); - else + var output = stdout || stderr; + if(!output) { callback(new LocalError('No output received')); + return; + } + try { + data = JSON.parse(output); + } catch(parseError) { + callback(new LocalError('Invalid output received: ' + parseError.message, output)); + return; + } if(data['state'] != 'connected'){ - callback(new LocalError(data['message']['message'])); + callback(new LocalError(that.getErrorMessage(data))); } else { that.pid = data['pid']; that.isProcessRunning = true; @@ -134,6 +140,17 @@ function Local(){ }, options['bs-host']); }; + // The binary reports failures as {"state": "...", "message": {"message": "..."}}, + // but not every non-connected payload carries a message key. Dereferencing it + // blindly throws, and inside the execFile callback that throw is an + // uncaughtException the caller cannot catch. See LOC-7325. + this.getErrorMessage = function(data){ + var message = data && data['message']; + if(message && typeof message === 'object') + message = message['message']; + return message || 'Failed to start BrowserStack Local'; + }; + this.isRunning = function(){ return this.pid && running(this.pid) && this.isProcessRunning; }; diff --git a/test/local_start_output_handling.js b/test/local_start_output_handling.js new file mode 100644 index 0000000..458759d --- /dev/null +++ b/test/local_start_output_handling.js @@ -0,0 +1,104 @@ +var expect = require('expect.js'), + fs = require('fs'), + os = require('os'), + path = require('path'), + browserstack = require('../index'); + +// Regression tests for LOC-7325. +// +// `Local.start` handles the binary's output inside an `execFile` callback. A +// throw there is raised by node's internal exithandler, so no try/catch around +// `start()` can intercept it — it surfaces as an uncaughtException and the +// blast radius is set by the host process's exception policy. These tests drive +// `start()` with stub binaries that reproduce each output shape and assert the +// callback fires exactly once with an error, and that nothing throws. +// +// Stubs are shell scripts, so these are skipped on Windows. +describe('Local.start output handling', function () { + var stubDir, bsLocal; + + function stub(name, body) { + var stubPath = path.join(stubDir, name); + fs.writeFileSync(stubPath, '#!/bin/sh\n' + body + '\n', { mode: 0o755 }); + return stubPath; + } + + // Drives start() with the given stub and collects every callback invocation + // plus any uncaughtException raised out of the execFile callback. + function run(stubPath, done) { + var calls = [], uncaught = []; + var existing = process.listeners('uncaughtException'); + process.removeAllListeners('uncaughtException'); + process.on('uncaughtException', function (err) { uncaught.push(err); }); + + bsLocal.binaryPath = stubPath; + bsLocal.start({ key: 'dummy-key', localIdentifier: 'loc-7325' }, function (error) { + calls.push(error); + }); + + // Settle past the execFile callback before asserting, so a second + // (throwing) invocation would have happened by now if it were going to. + setTimeout(function () { + process.removeAllListeners('uncaughtException'); + existing.forEach(function (listener) { process.on('uncaughtException', listener); }); + done(calls, uncaught); + }, 1000); + } + + before(function () { + stubDir = fs.mkdtempSync(path.join(os.tmpdir(), 'bs-local-7325-')); + }); + + beforeEach(function () { + bsLocal = new browserstack.Local(); + // Keep the stubs from clobbering ./local.log in the repo root. + bsLocal.logfile = path.join(stubDir, 'local.log'); + }); + + if (os.platform().match(/win32/i)) { + it.skip('skipped on Windows (stub binaries are shell scripts)'); + return; + } + + it('reports an error exactly once when the binary exits with no output', function (done) { + this.timeout(10000); + run(stub('empty-output.sh', 'exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0]).to.be.an('object'); + expect(calls[0].message).to.equal('No output received'); + done(); + }); + }); + + it('reports an error exactly once when the binary emits non-JSON output', function (done) { + this.timeout(10000); + run(stub('garbage-output.sh', 'echo "segmentation fault"; exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0].message).to.match(/^Invalid output received: /); + expect(calls[0].extra).to.match(/segmentation fault/); + done(); + }); + }); + + it('reports a fallback message when a non-connected payload has no message key', function (done) { + this.timeout(10000); + run(stub('no-message-key.sh', 'echo \'{"state":"disconnected"}\'; exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0].message).to.equal('Failed to start BrowserStack Local'); + done(); + }); + }); + + it('surfaces the binary message when a non-connected payload carries one', function (done) { + this.timeout(10000); + run(stub('with-message.sh', 'echo \'{"state":"disconnected","message":{"message":"Invalid key"}}\'; exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0].message).to.equal('Invalid key'); + done(); + }); + }); +});