-
-
Notifications
You must be signed in to change notification settings - Fork 87
Open
Description
Detail Bug Report
Summary
- Context: The
getBrowser()function inpackages/browserless/src/index.jsdetects disconnected browsers and triggers a respawn to restore connectivity. - Bug: When
getBrowser()callsrespawn()at line 68, it doesn't await the returned promise, causing unhandled promise rejections if the respawn fails. - Actual vs. expected: If
driver.spawn()fails during respawn, thePromise.allinrespawn()rejects without a handler, creating an unhandled rejection. Expected behavior is that all promise rejections should be caught and handled. - Impact: In strict Node.js environments (
--unhandled-rejections=strict), this causes application crashes. In normal environments, it produces spurious error logs that make debugging difficult.
Code with Bug
packages/browserless/src/index.js:
const getBrowser = async () => {
if (isClosed) return browserProcessPromise
const browserProcess = await lock(async () => {
const browserProcess = await browserProcessPromise
if (browserProcess.isConnected()) return browserProcess
respawn() // <-- BUG 🔴 Promise returned by respawn() is not awaited/caught, so failures become unhandled rejections
})
return browserProcess || getBrowser()
}const respawn = () =>
!isClosed &&
Promise.all([
browserProcessPromise.then(driver.close),
(browserProcessPromise = spawn({ respawn: true }))
])Additional unhandled-rejection path (event handler ignores returned promise):
promise.then(async browser => {
if (launchOpts.mode !== 'connect') browser.once('disconnected', getBrowser) // <-- BUG 🔴 returned promise is not handled
// ...
})Explanation
respawn()returns a rejecting promise whenspawn({respawn: true})fails.getBrowser()callsrespawn()withoutawait/.catch(), so that rejection is not observed by any handler, producingunhandledRejectionevents (and process crashes under--unhandled-rejections=strict).- The issue can trigger multiple times because
respawn()may be invoked both from explicitgetBrowser()calls and via the browserdisconnectedevent listener, wheregetBrowseris used as a fire-and-forget handler.
Recommended Fix
- In
getBrowser(),await respawn()(or explicitly attach.catch()if it must be fire-and-forget). - Wrap the
disconnectedhandler so the promise returned bygetBrowser()is caught/logged (instead of passinggetBrowserdirectly).
History
This bug was introduced in commit f6b34ac. The commit refactored the locking mechanism from mutexify to superlock, and during this refactor, the await keyword was accidentally removed from the respawn() call inside getBrowser().
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels