fix(browser): let quit clean up a browser whose contexts were never built - #634
Open
PhilMeyr wants to merge 1 commit into
Open
fix(browser): let quit clean up a browser whose contexts were never built#634PhilMeyr wants to merge 1 commit into
PhilMeyr wants to merge 1 commit into
Conversation
…uilt When start raises after Client.new but before Contexts.new returns, the browser keeps a non-nil @client and a nil @contexts. quit passes its @client guard, then calls contexts.close_connections and dies with NoMethodError before resetting any state, so restart can never recover. Guarding with safe navigation lets quit close the client, stop the process and clear the ivars, which is exactly what restart needs.
Member
|
I think we need to use attr_reader vs ivar and browser_spec is for systems spec, not unit tests |
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.
Problem
Browser#quitguards on@clientbut then callscontexts.close_connectionsunconditionally:startcan leave the browser with exactly the state that guard doesn't cover —@clientset and@contextsstillnil:Contexts.newsends CDP commands (find_implicit_context,subscribe,auto_attach,discover), so a browser that dies right after the websocket handshake raises there, after@clientwas assigned. Therescuestops the process and re-raises without clearing the ivars.That state is unrecoverable.
restartisquit; start, so the nextrestartentersquit, passesreturn unless @client, and raisesNoMethodError: undefined method 'close_connections' for nilbefore reaching the line that resets the ivars. Every subsequentrestartfails the same way, on the same object, forever.We hit this in production: Chrome started, aborted a moment later, and the long-lived browser singleton in our PDF worker was wedged until the process was restarted by hand. Downstream code that reasonably checks
browser.clientbefore restarting sees a non-nil client and never even attempts recovery.Fix
Safe-navigate the
contextscall.quitthen does the right thing for a half-started browser:@client.closereleases the websocket and its thread,@process.stopreaps the process, and the ivars are cleared, so the followingstartruns on a clean object.This is deliberately the smaller of the two possible fixes. Clearing
@client/@contextsinstart'srescueinstead would leave the client unclosed — lettingquithandle it keeps the existing cleanup path responsible for cleanup.The adjacent
page&.close/context&.disposeincreate_pageguard the same way, for the same reason (#582).Regression spec
Added to
#restart: stubContexts.newto raise, let onerestartfail, un-stub, and assert the browser restarts. Without the fix it fails withbundle exec rspec spec/browser_spec.rb— 52 examples, 0 failures, 3 pending. Rubocop clean.