From 02add6cc6b5dbfca18d53d25e864a6bf12b07f7a Mon Sep 17 00:00:00 2001 From: Dmitry Vorotilin Date: Tue, 22 Oct 2024 11:27:31 +0300 Subject: [PATCH 1/2] fix: JSON::NestingError for deeply nested JSON object --- CHANGELOG.md | 1 + lib/ferrum/client/web_socket.rb | 36 +-- spec/frame/runtime_spec.rb | 6 + spec/support/views/deeply_nested.erb | 316 +++++++++++++++++++++++++++ 4 files changed, 346 insertions(+), 13 deletions(-) create mode 100644 spec/support/views/deeply_nested.erb diff --git a/CHANGELOG.md b/CHANGELOG.md index 0436134d..829808f7 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/ferrum/client/web_socket.rb b/lib/ferrum/client/web_socket.rb index c3d7119e..e88d73bc 100644 --- a/lib/ferrum/client/web_socket.rb +++ b/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 diff --git a/spec/frame/runtime_spec.rb b/spec/frame/runtime_spec.rb index b6934e1c..9d95c08a 100644 --- a/spec/frame/runtime_spec.rb +++ b/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 diff --git a/spec/support/views/deeply_nested.erb b/spec/support/views/deeply_nested.erb new file mode 100644 index 00000000..0ef26f4e --- /dev/null +++ b/spec/support/views/deeply_nested.erb @@ -0,0 +1,316 @@ + + + + + + + + + +
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+

text

+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+ + From fac65e82efd2b6ca4177110440d7f96b5a1b94a7 Mon Sep 17 00:00:00 2001 From: Dmitry Vorotilin Date: Tue, 22 Oct 2024 12:51:52 +0300 Subject: [PATCH 2/2] fix: tests --- spec/browser_spec.rb | 30 ++++++++++-------------------- spec/downloads_spec.rb | 12 ++++++------ spec/network_spec.rb | 12 ++++-------- spec/node_spec.rb | 4 ++-- spec/page/screenshot_spec.rb | 14 +++++++------- spec/page_spec.rb | 8 +++++--- 6 files changed, 34 insertions(+), 46 deletions(-) diff --git a/spec/browser_spec.rb b/spec/browser_spec.rb index 6856f5c3..0e0c944a 100644 --- a/spec/browser_spec.rb +++ b/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 diff --git a/spec/downloads_spec.rb b/spec/downloads_spec.rb index ce3829d8..83158c2b 100644 --- a/spec/downloads_spec.rb +++ b/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") diff --git a/spec/network_spec.rb b/spec/network_spec.rb index 4d8d16a9..2fe49023 100644 --- a/spec/network_spec.rb +++ b/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") diff --git a/spec/node_spec.rb b/spec/node_spec.rb index a4d2af8b..f7d752ba 100644 --- a/spec/node_spec.rb +++ b/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 diff --git a/spec/page/screenshot_spec.rb b/spec/page/screenshot_spec.rb index aebd8674..e2a1d7ee 100644 --- a/spec/page/screenshot_spec.rb +++ b/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 diff --git a/spec/page_spec.rb b/spec/page_spec.rb index 8fc9be58..037ad8c6 100644 --- a/spec/page_spec.rb +++ b/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