BrowserCrawler closes ctx.page before errorHandler runs on navigation errors

[nikitachapovskii-dev]

Discovered in WCC PR

but turns out it closes the page on error before errorHandler runs ... _handleNavigationTimeout which closes crawlingContext.page.close() is being called on any navigation error

on actor side errorHandler can't reliably interact with ctx.page after a navigation error because the page is already closed by the time it runs.

The culprit is BrowserCrawler._handleNavigationTimeout

crawlee/packages/browser-crawler/src/internals/browser-crawler.ts

Lines 691 to 699 in d3a29d9

	protected async _handleNavigationTimeout(crawlingContext: Context, error: Error): Promise<void> {
	const { session } = crawlingContext;
	if (error && error.constructor.name === 'TimeoutError') {
	handleRequestTimeout({ session, errorMessage: error.message });
	}
	await crawlingContext.page.close();
	}

despite the name, it's called on every navigation error (timeout, network failure, Download is starting, frame detached, ...) and unconditionally does await page.close().

That close is also redundant: _cleanupContext already closes the page in the finally of _runRequestFunction. So the eager close just races the user's errorHandler (and, via activePages--, the browser pool's killer) for no real win.

Where eager close actually helps

Going through the cases:

• TimeoutError — page is probably hung on resources, eager close releases them immediately. Genuinely useful.
• net::ERR_* (DNS/refused/SSL/etc.) — nothing was loaded, nothing to release.
• Download is starting — page is healthy, browser holds the captured Download.
• frame detached / page already closed — _cleanupContext handles it idempotently anyway.

Only the timeout case justifies eager close.

[nikitachapovskii-dev]

Proposed fix :

Scope the eager close to its name:

protected async _handleNavigationTimeout(crawlingContext: Context, error: Error): Promise<void> {
    const { session } = crawlingContext;                                                                                                                  
    if (error && error.constructor.name === 'TimeoutError') {
        handleRequestTimeout({ session, errorMessage: error.message }); 
        await crawlingContext.page.close();    // move closing here
    }
}

Happy to open a PR if this looks good.
@barjin @janbuchar

[janbuchar]

@nikitachapovskii-dev thanks for reporting this. The issue should be fixed already in v4. Is there any workaround that you could utilize before it gets released?

[nikitachapovskii-dev]

Thanks for taking a look @janbuchar !

Is there any workaround that you could utilize before it gets released?

Unfortunately there's no clean workaround at the moment 😔
page is already closed by the time errorHandler runs, and anything we try there is racing the browser pool killer. The best I've come up with is to just open the Playwright stream before any async pre-checks to win that race for small files, but it's not reliable for large/slow downloads.

[moaedy]

Opened #3648.

[barjin]

The PR above has been closed (and the branch has been deleted), but it was, imo, a step in the right direction. Can you please open a new one with your changes, so we can discuss the details @nikitachapovskii-dev ?

Cheers!

[moaedy]

#3655 opened new PR (changed branch name)

[nikitachapovskii-dev]

Thanks @moaedy !