fix: add_target putting wrong target to pendings sometimes

lib/ferrum/browser.rb

@@ -209,6 +209,7 @@ def quit
       return unless @client
 
       contexts.close_connections
+
       @client.close
       @process.stop
       @client = @process = @contexts = nil

lib/ferrum/browser/client.rb

@@ -22,20 +22,26 @@ def initialize(ws_url, options)
         start
       end
 
-      def command(method, params = {})
-        pending = Concurrent::IVar.new
+      def command(method, async: false, **params)
         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
+        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)

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)

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

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

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
+      Browser::Client.new(ws_url, options)
+    end
   end
 end

spec/browser_spec.rb

@@ -238,6 +238,8 @@
         )
       end
 
+      after { browser.quit }
+
       context "with absolute path" do
         let(:save_path) { "/tmp/ferrum" }
 
@@ -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)
 

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")))

spec/unit/browser_spec.rb

@@ -23,6 +23,7 @@ def puts(*args)
     expect(file_log).to include("<html><head></head><body></body></html>")
   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("<html><head></head><body></body></html>")
+  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