fix(downloads): don't address the browser's implicit context by id - #628
Merged
Conversation
Passing `:save_path` to a browser that came up with its own startup window died on the first page with `Failed to find browser context for id`, `Browser.setDownloadBehavior` scoped the download behavior to `browserContextId` unconditionally. That window lives in the browser's implicit context, which Chrome refuses to address by id, the same reason `Target.createTarget` omits it there. `Target#context_id`, and with it `Page#context_id`, is now nil for that context, so anything scoping a command to a browser context can tell it apart by the id alone. The command must keep going through the page's session, sending it on the browser session delivers `Browser.downloadWillBegin`/`downloadProgress` there and `page.downloads` stops tracking the files it saves. Closes #546
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #546.
The bug
Downloads#set_behaviorsentbrowserContextId: @page.context_idunconditionally. For a page in the browser's implicit context — the one Chrome puts its startup window in — that id isn't addressable, so the first page died with:It reproduces on
mainagainst any browser that came up with its own startup window (a Chrome in a sidecar container, browserless) when:save_pathis set, sinceprepare_pageconfigures the download behavior for every page. It is not flatten-specific —flatten: falsefails identically — and not version-specific either, Chrome 152 fails the same way as the 119 in the report. Same root cause as #578/#627, which fixed it forTarget.createTargetonly.The fix
Target#context_id, and with itPage#context_id, isnilfor the implicit context, so a caller scoping a command to a browser context can tell it apart from the id alone, without reaching for the context object:Context#create_targetandContexts#disposekeep theimplicit?checks they already had, so all three CDP calls that take abrowserContextIdare covered. Those three are the only ones in the codebase — everything else is target- or session-scoped.Two alternatives that don't work, both checked against a live browser:
browserContextId: downloads in a context we created come backcanceled, the behavior lands on the default context instead.Browser.downloadWillBegin/downloadProgressare then delivered to the browser session andpage.downloads.files/#waittrack nothing. The command has to stay on the page's session.Cuprite builds its own pages with
Page.new(target.client, context_id: target.context_id, ...), so it picks this up unchanged.Verification
spec/downloads_spec.rb, usingwith_external_browser(incognito: false)for a browser with a startup window. It fails withFailed to find browser context for idwithout the fix.save_path.rbs validatepasses.Page#context_idisString?now, sinceTargetis what passes it and it can be nil.