fix(runtime): raise JavaScriptError for any thrown value, not only Error instances

CHANGELOG.md

@@ -41,6 +41,8 @@
   `429 Too Many Requests` for a full remote browser service, and exposes it as `#status`
 
 ### Fixed
+- `evaluate`, `execute` and friends raise `JavaScriptError` when JS throws something other than an `Error`, e.g.
+  `throw "smth"`, instead of returning it. Returning an `Error` object no longer crashes with `NoMethodError`
 - `workers` skips detached workers, `target.worker` with no session raises `NoSuchTargetError` instead of a CDP error
 - `attach_target` reuses a target's session and returns the target, instead of opening one more that reruns extensions
 - Service workers registered by a page stayed attached and alive for the browser's lifetime, now Chrome can stop them

lib/ferrum/errors.rb

@@ -216,7 +216,7 @@ class JavaScriptError < BrowserError
 
     def initialize(response)
       @class_name, @message = response["exception"]&.values_at("className", "description")
-      @message ||= response["text"]
+      @message ||= [response["text"], response.dig("exception", "value")].compact.join(" ")
       @stack_trace = response["stackTrace"]
       super(response.merge("message" => @message))
     end

lib/ferrum/frame/runtime.rb

@@ -171,14 +171,14 @@ def call(expression:, arguments: [], on: nil, wait: 0, handle: true, **options)
 
       # FIXME: We should have a central place to handle all type of errors
       def handle_error(response)
-        result = response["result"]
-        return if result["subtype"] != "error"
+        details = response["exceptionDetails"]
+        return unless details
 
-        case result["description"]
+        case details.dig("exception", "description")
         when /\AError: timed out promise/
           raise ScriptTimeoutError
         else
-          raise JavaScriptError, response["exceptionDetails"]
+          raise JavaScriptError, details
         end
       end
 

spec/frame/runtime_spec.rb

@@ -2,6 +2,10 @@
 
 describe Ferrum::Frame::Runtime do
   describe "#execute" do
+    it "raises when a string is thrown" do
+      expect { browser.execute(%(throw "smth")) }.to raise_error(Ferrum::JavaScriptError, "Uncaught smth")
+    end
+
     it "executes multiple lines of javascript" do
       browser.execute <<-JS
         var a = 1
@@ -128,6 +132,22 @@
       end.to raise_error(Ferrum::JavaScriptError)
     end
 
+    it "raises when a string is thrown" do
+      expect do
+        browser.evaluate(%((() => { throw "smth" })()))
+      end.to raise_error(Ferrum::JavaScriptError, "Uncaught smth")
+    end
+
+    it "raises when an object is thrown" do
+      expect do
+        browser.evaluate("(() => { throw { code: 1 } })()")
+      end.to raise_error(Ferrum::JavaScriptError) { |e| expect(e.class_name).to eq("Object") }
+    end
+
+    it "returns an Error object instead of raising" do
+      expect(browser.evaluate(%(new Error("smth")))).to eq({})
+    end
+
     context "when cyclic structure is returned" do
       context "ignores seen" do
         let(:code) do
@@ -219,9 +239,24 @@
         browser.evaluate_async("var callback=arguments[0]; setTimeout(function(){callback(true)}, 4000)", 1)
       end.to raise_error(Ferrum::ScriptTimeoutError)
     end
+
+    it "raises when a string is thrown" do
+      expect do
+        browser.evaluate_async(%(throw "smth"), 5)
+      end.to raise_error(Ferrum::JavaScriptError, "Uncaught (in promise) smth")
+    end
   end
 
   describe "#evaluate_on" do
+    it "raises when a string is thrown" do
+      browser.go_to("/table")
+      node = browser.at_xpath(".//td")
+
+      expect do
+        browser.evaluate_on(node: node, expression: %((() => { throw "smth" })()))
+      end.to raise_error(Ferrum::JavaScriptError, "Uncaught smth")
+    end
+
     it "does not retry if node is not around anymore" do
       stub_const("Ferrum::Frame::Runtime::INTERMITTENT_SLEEP", 5)