diff --git a/CHANGELOG.md b/CHANGELOG.md index c34838c5..bc210a71 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - `#files` information about downloaded files - `#wait` wait for file download to be completed - `#set_behavior` where and whether to store file +- `Browser::Client#command` accepts :async parameter [#433] ### Changed - `Ferrum::Page#screeshot` accepts :area option [#410] @@ -17,10 +18,15 @@ instead of passing browser and making cyclic dependency on the browser instance, - Bump `websocket-driver` to `~> 0.7` [#432] - Got rid of `Concurrent::Async` in `Ferrum::Browser::Subscriber` [#432] - `Ferrum::Page#set_window_bounds` is renamed to `Ferrum::Page#window_bounds=` +- `Ferrum::Page` get right client from the Target and passes it down everywhere [#433] +- `Ferrum::Network::InterceptedRequest` accepts `Ferrum::Browser::Client` instead of `Ferrum::Page` [#433] +- `Ferrum::Browser::Client` -> `Ferrum::Client` [#433] ### Fixed - Exceptions within `.on()` were swallowed by a thread pool of `Concurrent::Async` [#432] +- `Ferrum::Context#add_target` puts wrong target to pendings sometimes [#433] +- Leaking connection descriptors in tests and after browser quit [#433] ### Removed diff --git a/lib/ferrum/browser.rb b/lib/ferrum/browser.rb index 544943b8..e1186472 100644 --- a/lib/ferrum/browser.rb +++ b/lib/ferrum/browser.rb @@ -4,11 +4,11 @@ require "forwardable" require "ferrum/page" require "ferrum/proxy" +require "ferrum/client" require "ferrum/contexts" require "ferrum/browser/xvfb" require "ferrum/browser/options" require "ferrum/browser/process" -require "ferrum/browser/client" require "ferrum/browser/binary" require "ferrum/browser/version_info" @@ -209,6 +209,7 @@ def quit return unless @client contexts.close_connections + @client.close @process.stop @client = @process = @contexts = nil diff --git a/lib/ferrum/browser/client.rb b/lib/ferrum/browser/client.rb deleted file mode 100644 index 2cbb729e..00000000 --- a/lib/ferrum/browser/client.rb +++ /dev/null @@ -1,109 +0,0 @@ -# frozen_string_literal: true - -require "forwardable" -require "ferrum/browser/subscriber" -require "ferrum/browser/web_socket" - -module Ferrum - class Browser - class Client - extend Forwardable - delegate %i[timeout timeout=] => :options - - attr_reader :options - - def initialize(ws_url, options) - @command_id = 0 - @options = options - @pendings = Concurrent::Hash.new - @ws = WebSocket.new(ws_url, options.ws_max_receive_size, options.logger) - @subscriber = Subscriber.new - - start - end - - def command(method, params = {}) - pending = Concurrent::IVar.new - message = build_message(method, params) - @pendings[message[:id]] = pending - @ws.send_message(message) - data = pending.value!(timeout) - @pendings.delete(message[:id]) - - raise DeadBrowserError if data.nil? && @ws.messages.closed? - raise TimeoutError unless data - - error, response = data.values_at("error", "result") - raise_browser_error(error) if error - response - end - - def on(event, &block) - @subscriber.on(event, &block) - end - - def subscribed?(event) - @subscriber.subscribed?(event) - end - - def close - @ws.close - # Give a thread some time to handle a tail of messages - @pendings.clear - @thread.kill unless @thread.join(1) - @subscriber.close - end - - def inspect - "#<#{self.class} " \ - "@command_id=#{@command_id.inspect} " \ - "@pendings=#{@pendings.inspect} " \ - "@ws=#{@ws.inspect}>" - end - - private - - def start - @thread = Utils::Thread.spawn do - loop do - message = @ws.messages.pop - break unless message - - if message.key?("method") - @subscriber << message - else - @pendings[message["id"]]&.set(message) - end - end - end - end - - def build_message(method, params) - { method: method, params: params }.merge(id: next_command_id) - end - - def next_command_id - @command_id += 1 - end - - def raise_browser_error(error) - case error["message"] - # Node has disappeared while we were trying to get it - when "No node with given id found", - "Could not find node with given id", - "Inspected target navigated or closed" - raise NodeNotFoundError, error - # Context is lost, page is reloading - when "Cannot find context with specified id" - raise NoExecutionContextError, error - when "No target with given id found" - raise NoSuchPageError - when /Could not compute content quads/ - raise CoordinatesNotFoundError - else - raise BrowserError, error - end - end - end - end -end diff --git a/lib/ferrum/client.rb b/lib/ferrum/client.rb new file mode 100644 index 00000000..37afa112 --- /dev/null +++ b/lib/ferrum/client.rb @@ -0,0 +1,113 @@ +# frozen_string_literal: true + +require "forwardable" +require "ferrum/client/subscriber" +require "ferrum/client/web_socket" + +module Ferrum + class Client + extend Forwardable + delegate %i[timeout timeout=] => :options + + attr_reader :options + + def initialize(ws_url, options) + @command_id = 0 + @options = options + @pendings = Concurrent::Hash.new + @ws = WebSocket.new(ws_url, options.ws_max_receive_size, options.logger) + @subscriber = Subscriber.new + + start + end + + def command(method, async: false, **params) + message = build_message(method, params) + + if async + @ws.send_message(message) + true + else + pending = Concurrent::IVar.new + @pendings[message[:id]] = pending + @ws.send_message(message) + data = pending.value!(timeout) + @pendings.delete(message[:id]) + + raise DeadBrowserError if data.nil? && @ws.messages.closed? + raise TimeoutError unless data + + error, response = data.values_at("error", "result") + raise_browser_error(error) if error + response + end + end + + def on(event, &block) + @subscriber.on(event, &block) + end + + def subscribed?(event) + @subscriber.subscribed?(event) + end + + def close + @ws.close + # Give a thread some time to handle a tail of messages + @pendings.clear + @thread.kill unless @thread.join(1) + @subscriber.close + end + + def inspect + "#<#{self.class} " \ + "@command_id=#{@command_id.inspect} " \ + "@pendings=#{@pendings.inspect} " \ + "@ws=#{@ws.inspect}>" + end + + private + + def start + @thread = Utils::Thread.spawn do + loop do + message = @ws.messages.pop + break unless message + + if message.key?("method") + @subscriber << message + else + @pendings[message["id"]]&.set(message) + end + end + end + end + + def build_message(method, params) + { method: method, params: params }.merge(id: next_command_id) + end + + def next_command_id + @command_id += 1 + end + + def raise_browser_error(error) + case error["message"] + # Node has disappeared while we were trying to get it + when "No node with given id found", + "Could not find node with given id", + "Inspected target navigated or closed" + raise NodeNotFoundError, error + # Context is lost, page is reloading + when "Cannot find context with specified id" + raise NoExecutionContextError, error + when "No target with given id found" + raise NoSuchPageError + when /Could not compute content quads/ + raise CoordinatesNotFoundError + else + raise BrowserError, error + end + end + end +end diff --git a/lib/ferrum/browser/subscriber.rb b/lib/ferrum/client/subscriber.rb similarity index 98% rename from lib/ferrum/browser/subscriber.rb rename to lib/ferrum/client/subscriber.rb index 1a9fe7ee..7fc52a64 100644 --- a/lib/ferrum/browser/subscriber.rb +++ b/lib/ferrum/client/subscriber.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Ferrum - class Browser + class Client class Subscriber INTERRUPTIONS = %w[Fetch.requestPaused Fetch.authRequired].freeze diff --git a/lib/ferrum/browser/web_socket.rb b/lib/ferrum/client/web_socket.rb similarity index 99% rename from lib/ferrum/browser/web_socket.rb rename to lib/ferrum/client/web_socket.rb index 02204298..2fcb3a06 100644 --- a/lib/ferrum/browser/web_socket.rb +++ b/lib/ferrum/client/web_socket.rb @@ -5,7 +5,7 @@ require "websocket/driver" module Ferrum - class Browser + class Client class WebSocket WEBSOCKET_BUG_SLEEP = 0.05 SKIP_LOGGING_SCREENSHOTS = !ENV["FERRUM_LOGGING_SCREENSHOTS"] diff --git a/lib/ferrum/context.rb b/lib/ferrum/context.rb index cd1d479e..e0628017 100644 --- a/lib/ferrum/context.rb +++ b/lib/ferrum/context.rb @@ -54,10 +54,11 @@ def create_target end def add_target(params) - target = Target.new(@client, params) - @targets.put_if_absent(target.id, target) - + new_target = Target.new(@client, params) + target = @targets.put_if_absent(new_target.id, new_target) + target ||= new_target # `put_if_absent` returns nil if added a new value or existing if there was one already @pendings.put(target, @client.timeout) if @pendings.empty? + target end def update_target(target_id, params) diff --git a/lib/ferrum/network/intercepted_request.rb b/lib/ferrum/network/intercepted_request.rb index fb63de22..51a5f052 100644 --- a/lib/ferrum/network/intercepted_request.rb +++ b/lib/ferrum/network/intercepted_request.rb @@ -10,9 +10,9 @@ class InterceptedRequest attr_accessor :request_id, :frame_id, :resource_type, :network_id, :status - def initialize(page, params) + def initialize(client, params) @status = nil - @page = page + @client = client @params = params @request_id = params["requestId"] @frame_id = params["frameId"] @@ -43,18 +43,18 @@ def respond(**options) options = options.merge(body: Base64.strict_encode64(options.fetch(:body, ""))) if has_body @status = :responded - @page.command("Fetch.fulfillRequest", **options) + @client.command("Fetch.fulfillRequest", **options) end def continue(**options) options = options.merge(requestId: request_id) @status = :continued - @page.command("Fetch.continueRequest", **options) + @client.command("Fetch.continueRequest", **options) end def abort @status = :aborted - @page.command("Fetch.failRequest", requestId: request_id, errorReason: "BlockedByClient") + @client.command("Fetch.failRequest", requestId: request_id, errorReason: "BlockedByClient") end def initial_priority diff --git a/lib/ferrum/page.rb b/lib/ferrum/page.rb index 501fb35a..8f689ae4 100644 --- a/lib/ferrum/page.rb +++ b/lib/ferrum/page.rb @@ -34,6 +34,11 @@ class Page attr_accessor :referrer attr_reader :context_id, :target_id, :event, :tracing + # Client connection. + # + # @return [Client] + attr_reader :client + # Mouse object. # # @return [Mouse] @@ -65,10 +70,10 @@ class Page attr_reader :downloads def initialize(client, context_id:, target_id:, proxy: nil) + @client = client @context_id = context_id @target_id = target_id @options = client.options - self.client = client @frames = Concurrent::Map.new @main_frame = Frame.new(nil, self) @@ -118,14 +123,14 @@ def go_to(url = nil) def close @headers.clear - client(browser: true).command("Target.closeTarget", targetId: @target_id) + client.command("Target.closeTarget", async: true, targetId: @target_id) close_connection true end def close_connection - @page_client&.close + client&.close end # @@ -334,7 +339,7 @@ def bypass_csp(enabled: true) def command(method, wait: 0, slowmoable: false, **params) iteration = @event.reset if wait.positive? sleep(@options.slowmo) if slowmoable && @options.slowmo.positive? - result = client.command(method, params) + result = client.command(method, **params) if wait.positive? # Wait a bit after command and check if iteration has @@ -358,7 +363,7 @@ def on(name, &block) end when :request client.on("Fetch.requestPaused") do |params, index, total| - request = Network::InterceptedRequest.new(self, params) + request = Network::InterceptedRequest.new(client, params) exchange = network.select(request.network_id).last exchange ||= network.build_exchange(request.network_id) exchange.intercepted_request = request @@ -502,15 +507,5 @@ def proxy=(options) @proxy_user = options&.[](:user) || @options.proxy&.[](:user) @proxy_password = options&.[](:password) || @options.proxy&.[](:password) end - - def client=(browser_client) - @browser_client = browser_client - ws_url = @options.ws_url.merge(path: "/devtools/page/#{@target_id}").to_s - @page_client = Browser::Client.new(ws_url, @options) - end - - def client(browser: false) - browser ? @browser_client : @page_client - end end end diff --git a/lib/ferrum/target.rb b/lib/ferrum/target.rb index 0ebf7208..8dfc8af2 100644 --- a/lib/ferrum/target.rb +++ b/lib/ferrum/target.rb @@ -28,7 +28,7 @@ def page def build_page(**options) maybe_sleep_if_new_window - Page.new(@client, context_id: context_id, target_id: id, **options) + Page.new(build_client, context_id: context_id, target_id: id, **options) end def id @@ -63,5 +63,13 @@ def maybe_sleep_if_new_window # Dirty hack because new window doesn't have events at all sleep(NEW_WINDOW_WAIT) if window? end + + private + + def build_client + options = @client.options + ws_url = options.ws_url.merge(path: "/devtools/page/#{id}").to_s + Client.new(ws_url, options) + end end end diff --git a/spec/browser_spec.rb b/spec/browser_spec.rb index e3835d9a..508a6f5e 100644 --- a/spec/browser_spec.rb +++ b/spec/browser_spec.rb @@ -238,6 +238,8 @@ ) end + after { browser.quit } + context "with absolute path" do let(:save_path) { "/tmp/ferrum" } @@ -278,7 +280,7 @@ allow(Ferrum::Browser::Process).to receive(:new).and_return(process) error = StandardError.new - allow(Ferrum::Browser::Client).to receive(:new).and_raise(error) + allow(Ferrum::Client).to receive(:new).and_raise(error) expect { Ferrum::Browser.new }.to raise_error(error) expect(process.pid).to be(nil) @@ -330,6 +332,8 @@ it "stops silently before go_to call" do browser = Ferrum::Browser.new expect { browser.quit }.not_to raise_error + ensure + browser&.quit end it "supports stopping the session", skip: Ferrum::Utils::Platform.windows? do @@ -523,6 +527,21 @@ expect(browser.contexts.size).to eq(0) end + it "closes page successfully" do + expect(browser.contexts.size).to eq(0) + + page = browser.create_page(new_context: true) + context = browser.contexts[page.context_id] + page.go_to("/ferrum/simple") + page.close + + expect(browser.contexts.size).to eq(1) + expect(context.targets.size).to be_between(0, 1) # needs some time to close target + + context.dispose + expect(browser.contexts.size).to eq(0) + end + it "supports calling with block" do expect(browser.contexts.size).to eq(0) diff --git a/spec/frame/runtime_spec.rb b/spec/frame/runtime_spec.rb index 82689e55..b6934e1c 100644 --- a/spec/frame/runtime_spec.rb +++ b/spec/frame/runtime_spec.rb @@ -14,6 +14,8 @@ context "with javascript errors" do let(:browser) { Ferrum::Browser.new(base_url: base_url, js_errors: true) } + after { browser.quit } + it "propagates a Javascript error to a ruby exception" do expect do browser.execute(%(throw new Error("zomg"))) diff --git a/spec/unit/browser_spec.rb b/spec/unit/browser_spec.rb index 64c14c6c..700330d5 100644 --- a/spec/unit/browser_spec.rb +++ b/spec/unit/browser_spec.rb @@ -23,6 +23,7 @@ def puts(*args) expect(file_log).to include("") ensure FileUtils.rm_f(file_path) + browser.quit end it "logs requests and responses" do @@ -33,6 +34,8 @@ def puts(*args) expect(logger.string).to include("return document.documentElement.outerHTML") expect(logger.string).to include("") + ensure + browser.quit end it "shows command line options passed" do @@ -41,5 +44,7 @@ def puts(*args) arguments = browser.command("Browser.getBrowserCommandLine")["arguments"] expect(arguments).to include("--blink-settings=imagesEnabled=false") + ensure + browser.quit end end diff --git a/spec/unit/process_spec.rb b/spec/unit/process_spec.rb index 8fde4395..519a2ee7 100644 --- a/spec/unit/process_spec.rb +++ b/spec/unit/process_spec.rb @@ -7,7 +7,7 @@ it "forcibly kills the child if it does not respond to SIGTERM" do allow(Process).to receive(:spawn).and_return(5678) allow(Process).to receive(:wait).and_return(nil) - allow(Ferrum::Browser::Client).to receive(:new).and_return(double.as_null_object) + allow(Ferrum::Client).to receive(:new).and_return(double.as_null_object) allow_any_instance_of(Ferrum::Browser::Process).to receive(:parse_ws_url) @@ -25,7 +25,7 @@ it "passes through env" do allow(Process).to receive(:wait).and_return(nil) - allow(Ferrum::Browser::Client).to receive(:new).and_return(double.as_null_object) + allow(Ferrum::Client).to receive(:new).and_return(double.as_null_object) allow(Process).to receive(:spawn).with({ "LD_PRELOAD" => "some.so" }, any_args).and_return(123_456_789)