fix: Browser#create_page ensure block masks original error with NoMethodError#582
Open
collin-pereira wants to merge 1 commit intorubycdp:mainfrom
Open
fix: Browser#create_page ensure block masks original error with NoMethodError#582collin-pereira wants to merge 1 commit intorubycdp:mainfrom
collin-pereira wants to merge 1 commit intorubycdp:mainfrom
Conversation
c5f4147 to
0ff8879
Compare
0ff8879 to
f6e4ce7
Compare
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
When
Browser#create_pageis called withnew_context: trueand a block, an exception raised bycontexts.create(...)(for exampleFerrum::DeadBrowserErrorwhen the browser dies before the context is created) is masked by aNoMethodErrorfrom theensureblock.The local
contextisnilat that point because the assignment never completed. Theensureblock then callscontext.disposeunconditionally and raisesNoMethodError: undefined method 'dispose' for nil, which propagates in place of the original error. Callers who rescueFerrum::Error(the gem's documented contract) won't catch the maskedNoMethodError, so legitimate errors likeDeadBrowserErrorslip past their handlers.Fix
The adjacent cleanup of
pagealready uses safe navigation (page&.close) for the same reason; this just makescontext.disposesymmetric.A regression spec stubs
browser.contexts.createto raiseFerrum::DeadBrowserErrorand asserts the original error propagates through the block form. Without the fix it fails withexpected Ferrum::DeadBrowserError ... got NoMethodError: undefined method 'dispose' for nil.