Merge pull request #160 from rubycdp/fix-114

Fix #114 DOM methods should not be async

Author: route

.github/workflows/tests.yml

@@ -14,6 +14,7 @@ jobs:
     runs-on: ubuntu-latest
     env:
       FERRUM_PROCESS_TIMEOUT: 20
+      FERRUM_DEFAULT_TIMEOUT: 10
       BUNDLE_GEMFILE: .github/gemfiles/${{ matrix.gemfile }}.gemfile
     steps:
       - name: Checkout code

lib/ferrum/browser.rb

@@ -24,7 +24,7 @@ class Browser
                 mouse keyboard
                 screenshot pdf mhtml viewport_size
                 frames frame_by main_frame
-                evaluate evaluate_on evaluate_async execute
+                evaluate evaluate_on evaluate_async execute evaluate_func
                 add_script_tag add_style_tag bypass_csp
                 on goto] => :page
     delegate %i[default_user_agent] => :process

lib/ferrum/frame/dom.rb

@@ -37,54 +37,54 @@ def body
       end
 
       def xpath(selector, within: nil)
-        code = <<~JS
-          let selector = arguments[0];
-          let within = arguments[1] || document;
-          let results = [];
+        expr = <<~JS
+          function(selector, within) {
+            let results = [];
+            within ||= document
 
-          let xpath = document.evaluate(selector, within, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null);
-          for (let i = 0; i < xpath.snapshotLength; i++) {
-            results.push(xpath.snapshotItem(i));
-          }
+            let xpath = document.evaluate(selector, within, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null);
+            for (let i = 0; i < xpath.snapshotLength; i++) {
+              results.push(xpath.snapshotItem(i));
+            }
 
-          arguments[2](results);
+            return results;
+          }
         JS
 
-        evaluate_async(code, @page.timeout, selector, within)
+        evaluate_func(expr, selector, within)
       end
 
       def at_xpath(selector, within: nil)
-        code = <<~JS
-          let selector = arguments[0];
-          let within = arguments[1] || document;
-          let xpath = document.evaluate(selector, within, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null);
-          let result = xpath.snapshotItem(0);
-          arguments[2](result);
+        expr = <<~JS
+          function(selector, within) {
+            within ||= document
+            let xpath = document.evaluate(selector, within, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null);
+            return xpath.snapshotItem(0);
+          }
         JS
-
-        evaluate_async(code, @page.timeout, selector, within)
+        evaluate_func(expr, selector, within)
       end
 
       def css(selector, within: nil)
-        code = <<~JS
-          let selector = arguments[0];
-          let within = arguments[1] || document;
-          let results = Array.from(within.querySelectorAll(selector));
-          arguments[2](results);
+        expr = <<~JS
+          function(selector, within) {
+            within ||= document
+            return Array.from(within.querySelectorAll(selector));
+          }
         JS
 
-        evaluate_async(code, @page.timeout, selector, within)
+        evaluate_func(expr, selector, within)
       end
 
       def at_css(selector, within: nil)
-        code = <<~JS
-          let selector = arguments[0];
-          let within = arguments[1] || document;
-          let result = within.querySelector(selector);
-          arguments[2](result);
+        expr = <<~JS
+          function(selector, within) {
+            within ||= document
+            return within.querySelector(selector);
+          }
         JS
 
-        evaluate_async(code, @page.timeout, selector, within)
+        evaluate_func(expr, selector, within)
       end
     end
   end

lib/ferrum/frame/runtime.rb

@@ -3,36 +3,19 @@
 require "singleton"
 
 module Ferrum
+  class CyclicObject
+    include Singleton
+
+    def inspect
+      %(#<#{self.class} JavaScript object that cannot be represented in Ruby>)
+    end
+  end
+
   class Frame
     module Runtime
       INTERMITTENT_ATTEMPTS = ENV.fetch("FERRUM_INTERMITTENT_ATTEMPTS", 6).to_i
       INTERMITTENT_SLEEP = ENV.fetch("FERRUM_INTERMITTENT_SLEEP", 0.1).to_f
 
-      EXECUTE_OPTIONS = {
-        returnByValue: true,
-        functionDeclaration: %(function() { %s })
-      }.freeze
-      DEFAULT_OPTIONS = {
-        functionDeclaration: %(function() { return %s })
-      }.freeze
-      EVALUATE_ASYNC_OPTIONS = {
-        awaitPromise: true,
-        functionDeclaration: %(
-          function() {
-           return new Promise((__resolve, __reject) => {
-             try {
-               arguments[arguments.length] = r => __resolve(r);
-               arguments.length = arguments.length + 1;
-               setTimeout(() => __reject(new Error("timed out promise")), %s);
-               %s
-             } catch(error) {
-               __reject(error);
-             }
-           });
-          }
-        )
-      }.freeze
-
       SCRIPT_SRC_TAG = <<~JS
         const script = document.createElement("script");
         script.src = arguments[0];
@@ -63,37 +46,45 @@ module Runtime
       JS
 
       def evaluate(expression, *args)
-        call(*args, expression: expression)
+        expression = "function() { return %s }" % expression
+        call(expression: expression, arguments: args)
       end
 
-      def evaluate_async(expression, wait_time, *args)
-        call(*args, expression: expression, wait_time: wait_time * 1000, **EVALUATE_ASYNC_OPTIONS)
+      def evaluate_async(expression, wait, *args)
+        template = <<~JS
+          function() {
+            return new Promise((__f, __r) => {
+              try {
+                arguments[arguments.length] = r => __f(r);
+                arguments.length = arguments.length + 1;
+                setTimeout(() => __r(new Error("timed out promise")), %s);
+                %s
+              } catch(error) {
+                __r(error);
+              }
+            });
+          }
+        JS
+
+        expression = template % [wait * 1000, expression]
+        call(expression: expression, arguments: args, awaitPromise: true)
       end
 
       def execute(expression, *args)
-        call(*args, expression: expression, handle: false, **EXECUTE_OPTIONS)
+        expression = "function() { %s }" % expression
+        call(expression: expression, arguments: args, handle: false, returnByValue: true)
         true
       end
 
-      def evaluate_on(node:, expression:, by_value: true, wait: 0)
-        errors = [NodeNotFoundError, NoExecutionContextError]
-        attempts, sleep = INTERMITTENT_ATTEMPTS, INTERMITTENT_SLEEP
-
-        Ferrum.with_attempts(errors: errors, max: attempts, wait: sleep) do
-          response = @page.command("DOM.resolveNode", nodeId: node.node_id)
-          object_id = response.dig("object", "objectId")
-          options = DEFAULT_OPTIONS.merge(objectId: object_id)
-          options[:functionDeclaration] = options[:functionDeclaration] % expression
-          options.merge!(returnByValue: by_value)
-
-          response = @page.command("Runtime.callFunctionOn",
-                                   wait: wait, slowmoable: true,
-                                   **options)
-          handle_error(response)
-          response = response["result"]
+      def evaluate_func(expression, *args, on: nil)
+        call(expression: expression, arguments: args, on: on)
+      end
 
-          by_value ? response.dig("value") : handle_response(response)
-        end
+      def evaluate_on(node:, expression:, by_value: true, wait: 0)
+        options = { handle: true }
+        expression = "function() { return %s }" % expression
+        options = { handle: false, returnByValue: true } if by_value
+        call(expression: expression, on: node, wait: wait, **options)
       end
 
       def add_script_tag(url: nil, path: nil, content: nil, type: "text/javascript")
@@ -126,27 +117,32 @@ def add_style_tag(url: nil, path: nil, content: nil)
 
       private
 
-      def call(*args, expression:, wait_time: nil, handle: true, **options)
+      def call(expression:, arguments: [], on: nil, wait: 0, handle: true, **options)
+        params = options.dup
         errors = [NodeNotFoundError, NoExecutionContextError]
         attempts, sleep = INTERMITTENT_ATTEMPTS, INTERMITTENT_SLEEP
 
         Ferrum.with_attempts(errors: errors, max: attempts, wait: sleep) do
-          arguments = prepare_args(args)
-          params = DEFAULT_OPTIONS.merge(options)
-          expression = [wait_time, expression] if wait_time
-          params[:functionDeclaration] = params[:functionDeclaration] % expression
-          params = params.merge(arguments: arguments)
-          unless params[:executionContextId]
-            params = params.merge(executionContextId: execution_id)
+          if on
+            response = @page.command("DOM.resolveNode", nodeId: on.node_id)
+            object_id = response.dig("object", "objectId")
+            params.merge!(objectId: object_id)
+          elsif params[:executionContextId].nil?
+            params.merge!(executionContextId: execution_id)
+          else
+            # executionContextId is passed, nop
           end
 
+          params.merge!(functionDeclaration: expression,
+                        arguments: prepare_args(arguments))
+
           response = @page.command("Runtime.callFunctionOn",
-                                   slowmoable: true,
+                                   wait: wait, slowmoable: true,
                                    **params)
           handle_error(response)
           response = response["result"]
 
-          handle ? handle_response(response) : response
+          handle ? handle_response(response) : response.dig("value")
         end
       end
 
@@ -265,12 +261,4 @@ def cyclic_object
       end
     end
   end
-
-  class CyclicObject
-    include Singleton
-
-    def inspect
-      %(#<#{self.class} JavaScript object that cannot be represented in Ruby>)
-    end
-  end
 end

lib/ferrum/page.rb

@@ -32,7 +32,7 @@ def reset
     extend Forwardable
     delegate %i[at_css at_xpath css xpath
                 current_url current_title url title body doctype set_content
-                execution_id evaluate evaluate_on evaluate_async execute
+                execution_id evaluate evaluate_on evaluate_async execute evaluate_func
                 add_script_tag add_style_tag] => :main_frame
 
     include Frames, Screenshot

spec/browser_spec.rb

@@ -143,15 +143,6 @@ module Ferrum
       expect(browser.body).to include("x: 100, y: 150")
     end
 
-    it "supports executing multiple lines of javascript" do
-      browser.execute <<-JS
-        var a = 1
-        var b = 2
-        window.result = a + b
-      JS
-      expect(browser.evaluate("window.result")).to eq(3)
-    end
-
     it "supports stopping the session", skip: Ferrum.windows? do
       browser = Browser.new
       pid = browser.process.pid
@@ -474,99 +465,6 @@ module Ferrum
       expect(value).to be_nil
     end
 
-    context "evaluate" do
-      it "can return an element" do
-        browser.go_to("/ferrum/type")
-        element = browser.evaluate(%(document.getElementById("empty_input")))
-        expect(element).to eq(browser.at_css("#empty_input"))
-      end
-
-      it "can return structures with elements" do
-        browser.go_to("/ferrum/type")
-        result = browser.evaluate <<~JS
-          {
-            a: document.getElementById("empty_input"),
-            b: { c: document.querySelectorAll("#empty_textarea, #filled_textarea") }
-          }
-        JS
-
-        expect(result).to eq(
-          "a" => browser.at_css("#empty_input"),
-          "b" => {
-            "c" => browser.css("#empty_textarea, #filled_textarea")
-          }
-        )
-      end
-    end
-
-    context "evaluate_async" do
-      it "handles evaluate_async value properly" do
-        expect(browser.evaluate_async("arguments[0](null)", 5)).to be_nil
-        expect(browser.evaluate_async("arguments[0](false)", 5)).to be false
-        expect(browser.evaluate_async("arguments[0](true)", 5)).to be true
-        expect(browser.evaluate_async(%(arguments[0]({foo: "bar"})), 5)).to eq("foo" => "bar")
-      end
-
-      it "will timeout" do
-        expect {
-          browser.evaluate_async("var callback=arguments[0]; setTimeout(function(){callback(true)}, 4000)", 1)
-        }.to raise_error(Ferrum::ScriptTimeoutError)
-      end
-    end
-
-    it "handles evaluate values properly" do
-      expect(browser.evaluate("null")).to be_nil
-      expect(browser.evaluate("false")).to be false
-      expect(browser.evaluate("true")).to be true
-      expect(browser.evaluate("undefined")).to eq(nil)
-
-      expect(browser.evaluate("3;")).to eq(3)
-      expect(browser.evaluate("31337")).to eq(31337)
-      expect(browser.evaluate(%("string"))).to eq("string")
-      expect(browser.evaluate(%({foo: "bar"}))).to eq("foo" => "bar")
-
-      expect(browser.evaluate("new Object")).to eq({})
-      expect(browser.evaluate("new Date(2012, 0).toDateString()")).to eq("Sun Jan 01 2012")
-      expect(browser.evaluate("new Object({a: 1})")).to eq({"a" => 1})
-      expect(browser.evaluate("new Array")).to eq([])
-      expect(browser.evaluate("new Function")).to eq({})
-
-      expect {
-        browser.evaluate(%(throw "smth"))
-      }.to raise_error(Ferrum::JavaScriptError)
-    end
-
-    context "cyclic structure" do
-      context "ignores seen" do
-        let(:code) {
-          <<~JS
-            (function() {
-              var a = {};
-              var b = {};
-              var c = {};
-              c.a = a;
-              a.a = a;
-              a.b = b;
-              a.c = c;
-              return %s;
-            })()
-          JS
-        }
-
-        it "objects" do
-          expect(browser.evaluate(code % "a")).to eq(CyclicObject.instance)
-        end
-
-        it "arrays" do
-          expect(browser.evaluate(code % "[a]")).to eq([CyclicObject.instance])
-        end
-      end
-
-      it "backtracks what it has seen" do
-        expect(browser.evaluate("(function() { var a = {}; return [a, a] })()")).to eq([{}, {}])
-      end
-    end
-
     it "synchronizes page loads properly" do
       browser.go_to("/ferrum/index")
       browser.at_xpath("//a[text() = 'JS redirect']").click