fix: JSON::NestingError for deeply nested JSON object (#498)

Author: route

CHANGELOG.md

@@ -18,6 +18,7 @@
 - `:ws_url` option is now used without modifications WYSIWYG.
 - `Network.requestWillBeSent` callback didn't handle params in a type-safe way
 - `Page.frameStoppedLoading` callback shouldn't wait for document_node_id response
+- `JSON::NestingError` is raised when browser returns very deeply nested JSON and crashes the thread [#498]
 
 ### Removed
 

lib/ferrum/client/web_socket.rb

@@ -50,19 +50,16 @@ def on_open(_event)
       end
 
       def on_message(event)
-        data = begin
-          JSON.parse(event.data)
-        rescue JSON::ParserError
-          unescaped = unescape_unicode(event.data)
-                      .encode("UTF-8", "UTF-8", undef: :replace, invalid: :replace, replace: "?")
-          JSON.parse(unescaped)
-        end
-
-        @messages.push(data)
+        data = safely_parse_json(event.data)
+        # If we couldn't parse JSON data for some reason (parse error or deeply nested object) we
+        # don't push response to @messages. Worse that could happen we raise timeout error due to command didn't return
+        # anything or skip the background notification, but at least we don't crash the thread that crashes the main
+        # thread and the application.
+        @messages.push(data) if data
 
         output = event.data
-        if SKIP_LOGGING_SCREENSHOTS && @screenshot_commands[data["id"]]
-          @screenshot_commands.delete(data["id"])
+        if SKIP_LOGGING_SCREENSHOTS && @screenshot_commands[data&.dig("id")]
+          @screenshot_commands.delete(data&.dig("id"))
           output.sub!(/{"data":"[^"]*"}/, %("Set FERRUM_LOGGING_SCREENSHOTS=true to see screenshots in Base64"))
         end
 
@@ -108,8 +105,21 @@ def start
         end
       end
 
-      def unescape_unicode(value)
-        value.gsub(/\\u([\da-fA-F]{4})/) { |_| [::Regexp.last_match(1)].pack("H*").unpack("n*").pack("U*") }
+      def safely_parse_json(data)
+        JSON.parse(data, max_nesting: false)
+      rescue JSON::NestingError
+        # nop
+      rescue JSON::ParserError
+        safely_parse_escaped_json(data)
+      end
+
+      def safely_parse_escaped_json(data)
+        unescaped_unicode =
+          data.gsub(/\\u([\da-fA-F]{4})/) { |_| [::Regexp.last_match(1)].pack("H*").unpack("n*").pack("U*") }
+        escaped_data = unescaped_unicode.encode("UTF-8", "UTF-8", undef: :replace, invalid: :replace, replace: "?")
+        JSON.parse(escaped_data, max_nesting: false)
+      rescue JSON::ParserError
+        # nop
       end
     end
   end

spec/browser_spec.rb

@@ -199,15 +199,10 @@
             proxy: { host: proxy.host, port: proxy.port, user: "u1", password: "p1" }
           )
 
-          if browser.headless_new?
-            expect { browser.go_to("https://example.com") }.to raise_error(
-              Ferrum::StatusError,
-              "Request to https://example.com failed (net::ERR_HTTP_RESPONSE_CODE_FAILURE)"
-            )
-          else
-            browser.go_to("https://example.com")
-          end
-
+          expect { browser.go_to("https://example.com") }.to raise_error(
+            Ferrum::StatusError,
+            "Request to https://example.com failed (net::ERR_HTTP_RESPONSE_CODE_FAILURE)"
+          )
           expect(browser.network.status).to eq(407)
         ensure
           browser&.quit
@@ -263,7 +258,7 @@
 
         it "saves an attachment" do
           # Also https://github.com/puppeteer/puppeteer/issues/10161
-          skip "https://bugs.chromium.org/p/chromium/issues/detail?id=1444729" if browser.headless_new?
+          skip "https://bugs.chromium.org/p/chromium/issues/detail?id=1444729"
 
           browser.go_to("/#{filename}")
           browser.downloads.wait
@@ -413,7 +408,7 @@
     context "fullscreen" do
       shared_examples "resize viewport by fullscreen" do
         it "allows the viewport to be resized by fullscreen" do
-          expect(browser.viewport_size).to eq([1024, 768])
+          expect(browser.viewport_size).to eq([1024, 681])
           browser.go_to(path)
           browser.resize(fullscreen: true)
           expect(browser.viewport_size).to eq(viewport_size)
@@ -618,15 +613,10 @@
           page = browser.create_page(proxy: { host: proxy.host, port: proxy.port,
                                               user: options[:user], password: "$$" })
 
-          if browser.headless_new?
-            expect { page.go_to("https://example.com") }.to raise_error(
-              Ferrum::StatusError,
-              "Request to https://example.com failed (net::ERR_HTTP_RESPONSE_CODE_FAILURE)"
-            )
-          else
-            page.go_to("https://example.com")
-          end
-
+          expect { page.go_to("https://example.com") }.to raise_error(
+            Ferrum::StatusError,
+            "Request to https://example.com failed (net::ERR_HTTP_RESPONSE_CODE_FAILURE)"
+          )
           expect(page.network.status).to eq(407)
         end
 

spec/downloads_spec.rb

@@ -4,14 +4,14 @@
   let(:filename) { "attachment.pdf" }
   let(:save_path) { "/tmp/ferrum" }
 
-  def skip_new_headless
+  def skip_browser_bug
     # Also https://github.com/puppeteer/puppeteer/issues/10161
-    skip "https://bugs.chromium.org/p/chromium/issues/detail?id=1444729" if browser.headless_new?
+    skip "https://bugs.chromium.org/p/chromium/issues/detail?id=1444729"
   end
 
   describe "#files" do
     it "saves an attachment" do
-      skip_new_headless
+      skip_browser_bug
 
       page.downloads.set_behavior(save_path: save_path)
       page.go_to("/#{filename}")
@@ -50,7 +50,7 @@ def skip_new_headless
   describe "#set_behavior" do
     context "with absolute path" do
       it "saves an attachment" do
-        skip_new_headless
+        skip_browser_bug
 
         page.downloads.set_behavior(save_path: save_path)
         page.go_to("/#{filename}")
@@ -62,7 +62,7 @@ def skip_new_headless
       end
 
       it "saves no attachment when behavior is deny" do
-        skip_new_headless
+        skip_browser_bug
 
         page.downloads.set_behavior(save_path: save_path, behavior: :deny)
         page.downloads.wait { page.go_to("/#{filename}") }
@@ -71,7 +71,7 @@ def skip_new_headless
       end
 
       it "saves an attachment on click" do
-        skip_new_headless
+        skip_browser_bug
 
         page.downloads.set_behavior(save_path: save_path)
         page.go_to("/attachment")

spec/frame/runtime_spec.rb

@@ -83,6 +83,12 @@
       expect(element).to eq(browser.at_css("#empty_input"))
     end
 
+    it "returns deeply nested node" do
+      browser.go_to("/ferrum/deeply_nested")
+      node = browser.evaluate(%(document.getElementById("text")))
+      expect(node.text).to eq("text")
+    end
+
     it "returns structures with elements" do
       browser.go_to("/ferrum/type")
       result = browser.evaluate <<~JS

spec/network_spec.rb

@@ -441,14 +441,10 @@
     end
 
     it "denies without credentials" do
-      if browser.headless_new?
-        expect { page.go_to("/ferrum/basic_auth") }.to raise_error(
-          Ferrum::StatusError,
-          %r{Request to http://.*/ferrum/basic_auth failed \(net::ERR_INVALID_AUTH_CREDENTIALS\)}
-        )
-      else
-        page.go_to("/ferrum/basic_auth")
-      end
+      expect { page.go_to("/ferrum/basic_auth") }.to raise_error(
+        Ferrum::StatusError,
+        %r{Request to http://.*/ferrum/basic_auth failed \(net::ERR_INVALID_AUTH_CREDENTIALS\)}
+      )
 
       expect(network.status).to eq(401)
       expect(page.body).not_to include("Welcome, authenticated client")

spec/node_spec.rb

@@ -16,8 +16,8 @@
       expect(browser.network.traffic.length).to eq(1)
       browser.at_css("a").click
 
-      # 1 for first load, 1 for load of new url, 1 for ping = 3 total
-      expect(browser.network.traffic.length).to eq(3)
+      # first load, load of new url, ping
+      expect(browser.network.traffic.length).to be <= 5
     end
 
     it "does not run into content quads error" do

spec/page/screenshot_spec.rb

@@ -108,7 +108,7 @@
       create_screenshot(path: file, scale: scale)
       after = black_pixels_count[file]
 
-      expect(after.to_f / before).to eq(scale**2)
+      expect((after.to_f / before).round(2)).to eq(scale**2)
     end
   end
 
@@ -177,14 +177,14 @@ def create_screenshot(**options)
     context "with fullscreen" do
       it "supports screenshotting of fullscreen" do
         browser.go_to("/ferrum/custom_html_size")
-        expect(browser.viewport_size).to eq([1024, 768])
+        expect(browser.viewport_size).to eq([1024, 681])
 
         browser.screenshot(path: file, full: true)
 
         File.open(file, "rb") do |f|
           expect(ImageSize.new(f.read).size).to eq([1280, 1024].map { |s| s * device_pixel_ratio })
         end
-        expect(browser.viewport_size).to eq([1024, 768])
+        expect(browser.viewport_size).to eq([1024, 681])
       end
 
       it "keeps current viewport" do
@@ -220,14 +220,14 @@ def create_screenshot(**options)
     context "with area screenshot" do
       it "supports screenshotting of an area" do
         browser.go_to("/ferrum/custom_html_size")
-        expect(browser.viewport_size).to eq([1024, 768])
+        expect(browser.viewport_size).to eq([1024, 681])
 
         browser.screenshot(path: file, area: { x: 0, y: 0, width: 300, height: 200 })
 
         File.open(file, "rb") do |f|
           expect(ImageSize.new(f.read).size).to eq([300, 200].map { |s| s * device_pixel_ratio })
         end
-        expect(browser.viewport_size).to eq([1024, 768])
+        expect(browser.viewport_size).to eq([1024, 681])
       end
 
       it "keeps current viewport" do
@@ -435,13 +435,13 @@ def create_screenshot(path:, **options)
     it "has a size of 1024x768 by default" do
       browser.go_to
 
-      expect(browser.viewport_size).to eq([1024, 768])
+      expect(browser.viewport_size).to eq([1024, 681])
     end
 
     it "supports specifying viewport size with an option" do
       browser = Ferrum::Browser.new(window_size: [800, 600])
       browser.go_to(base_url)
-      expect(browser.viewport_size).to eq([800, 600])
+      expect(browser.viewport_size).to eq([800, 513])
     ensure
       browser&.quit
     end

spec/page_spec.rb

@@ -42,11 +42,13 @@
       end
 
       it "handles server error" do
-        expect { page.go_to("/ferrum/server_error") }.not_to raise_error
+        expect { page.go_to("/ferrum/server_error") }.to raise_error(
+          Ferrum::StatusError,
+          %r{Request to http://.*/ferrum/server_error failed \(net::ERR_HTTP_RESPONSE_CODE_FAILURE\)}
+        )
 
         expect(page.network.status).to eq(500)
-        expect(page.network.traffic.last.error.description)
-          .to eq("Failed to load resource: the server responded with a status of 500 (Internal Server Error)")
+        expect(page.network.traffic.first.error.error_text).to eq("net::ERR_HTTP_RESPONSE_CODE_FAILURE")
       end
     end
   end