Skip to content

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

Draft
vivianludrick wants to merge 1 commit into
masterfrom
fix/LOC-7325-local-start-callback-fallthrough
Draft

LOC-7325: stop uncatchable TypeError on empty binary output in Local.start#182
vivianludrick wants to merge 1 commit into
masterfrom
fix/LOC-7325-local-start-callback-fallthrough

Conversation

@vivianludrick

Copy link
Copy Markdown
Collaborator

Fixes an uncatchable TypeError thrown out of Local.start() when the BrowserStackLocal binary exits with no output.

JIRA Story: https://browserstack.atlassian.net/browse/LOC-7325

The bug

start() handles the binary's output inside an execFile callback. The empty-output branch called back with No output received but did not return, so control fell through to the next statement, which dereferences data['message']['message'] on data = {}:

TypeError: Cannot read properties of undefined (reading 'message')
    at .../browserstack-local/lib/Local.js:127:50

Two things make this worse than a normal error path:

  • The callback fires twice — once legitimately with No output received, then again from the throwing statement.
  • The caller cannot catch it. The throw happens inside a callback invoked by node's internal exithandler, so no try/catch around local.start(...) intercepts it. It surfaces as an uncaughtException, which means the blast radius is set by the host process's exception policy, not by this package. In the case that surfaced it, a host with a fatal uncaughtException handler lost its entire reporting plane because an optional tunnel failed to start.

The trigger is not exotic — any environment where the binary exits without emitting JSON reaches it: wrong or blocked binary path, killed process, permission failure, or a shimmed binary in CI.

The fix

Three paths reached the same unguarded deref; all three are now closed:

Path Before After
Empty stdout and stderr callback, then fall through and throw callback once, then return
Terminal branch of the error handler callback, then fall through and throw callback once, then return
Non-connected payload with no message key throws falls back to Failed to start BrowserStack Local

Also guarded JSON.parse: non-JSON output (a plain-text crash message, for instance) threw a SyntaxError from the same uncatchable position. It is now reported through the callback as Invalid output received: <reason>, with the raw output attached as the error's extra field.

startSync shared the unguarded deref and now uses the same helper. Its empty-output branch already returned, so it was never exposed to the fall-through.

Every changed path now invokes the callback exactly once and lets the caller handle the failure normally.

Tests

Added test/local_start_output_handling.js — drives start() with stub binaries for each output shape and asserts the callback fires exactly once and that nothing escapes as an uncaughtException. No credentials or network needed.

Verified the tests actually catch the defect by toggling the fix:

  • On master: 3 of the 4 fail, with the ticket's exact TypeError: Cannot read properties of undefined (reading 'message').
  • With this change: all 4 pass.

Full suite, excluding the LocalBinary > Download block that needs real credentials:

  • master: 28 passing, 3 failing, 2 pending
  • this branch: 32 passing, 3 failing, 2 pending

Same 3 failures before and after (should return is running properly ×2, should stop local) — all pre-existing and credential-gated, none related to this change. npm run pretest (eslint over lib/* index.js) is clean.

Note: the fix avoids optional chaining because the repo's eslint config sets env: es6 (ES2015).

Scope

Code fix only — no version bump or publish here. 1.5.13 is the latest published version and carries the defect, so this needs a release to reach consumers.

…start

`start()` handles the binary's output inside an `execFile` callback. The
empty-output branch called back with 'No output received' but did not
return, so control fell through to `data['message']['message']` on
`data = {}`. That threw a TypeError, and because the throw happens inside
a callback invoked by node's internal exithandler, no try/catch around
`local.start(...)` could intercept it — it surfaced as an
uncaughtException in the host process.

Three paths reached the same unguarded deref:

- empty stdout and stderr (the reported one) — now returns after the
  callback, so it fires exactly once
- the terminal branch of the `error` handler, which also fell through
- any non-connected payload with no `message` key

Also guards `JSON.parse`: non-JSON output threw a SyntaxError from the
same uncatchable position, and is now reported through the callback with
the raw output attached as `extra`.

`startSync` shared the unguarded deref and now uses the same helper. Its
empty-output branch already returned, so it was not exposed to the
fall-through.

Adds regression tests driving start() with stub binaries for each output
shape, asserting the callback fires exactly once and nothing escapes as
an uncaughtException. They need no credentials or network. Three of the
four fail on master with the TypeError from the ticket.
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

Successfully merging this pull request may close these issues.

1 participant