Merge pull request #182 from browserstack/fix/LOC-7325-local-start-callback-fallthrough

LOC-7325: stop uncatchable TypeError on empty binary output in Local.start

Author: 07souravkunda

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;
   };

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();
+    });
+  });
+});