Gracefully handle errors returned by chrome

[tonnenpinguin]

Hi!

While playing around with this I noticed that if I pass some arguably invalid options like in the following example, chromic ignores the error sent by Chrome and instead waits for the timeout and then fails with a generic message.

ChromicPDF.print_to_pdf({:html, "<h1>Hello,world!</h1>"}, print_to_pdf: %{pageRanges: "2-3"})

In the logs you can see that Chrome immediately tells us that the page range argument was faulty.

18:12:15.260 [debug] [ChromicPDF] msg out: %{"id" => 23, "method" => "Page.printToPDF", "params" => %{"footerTemplate" => "", "headerTemplate" => "", "pageRanges" => "2-3"}, "sessionId" => "CE865B72566BD21A8272A3F7EEF30EBE"}
18:12:15.262 [debug] [ChromicPDF] msg in: %{"error" => %{"code" => -32000, "message" => "Page range exceeds page count"}, "id" => 23, "sessionId" => "CE865B72566BD21A8272A3F7EEF30EBE"}
18:12:15.262 [debug] [ChromicPDF] msg in: %{"method" => "Page.frameResized", "params" => %{}, "sessionId" => "CE865B72566BD21A8272A3F7EEF30EBE"}
[... about 5 seconds later the error message appears]
** (ChromicPDF.Browser.ExecutionError) Timeout in Channel.run_protocol/3!

The underlying GenServer.call/3 exited with a timeout. This happens when the browser was
not able to complete the current operation (= PDF print job) within the configured
5000 milliseconds.

If you are printing large PDFs and expect long processing times, please consult the
documentation for the `timeout` option of the session pool.

If you are *not* printing large PDFs but your print jobs still time out, this is likely a
bug in ChromicPDF. Please open an issue on the issue tracker.

---

Current protocol:

%ChromicPDF.Protocol{
  steps: [
    await: &ChromicPDF.PrintToPDF.printed/2,
    call: &ChromicPDF.ResetTarget.reset_history/2,
    await: &ChromicPDF.ResetTarget.history_reset/2,
    call: &ChromicPDF.ResetTarget.blank/2,
    await: &ChromicPDF.ResetTarget.blanked/2,
    await: &ChromicPDF.ResetTarget.fsl_after_blank/2,
    output: &ChromicPDF.PrintToPDF.output/1
  ],
  state: %{
    :__protocol__ => ChromicPDF.PrintToPDF,
    :capture_screenshot => %{},
    :html => "[FILTERED]",
    :ignore_certificate_errors => false,
    :last_call_id => 23,
    :name => "[FILTERED]",
    :offline => false,
    :print_to_pdf => %{
      "footerTemplate" => "[FILTERED]",
      "headerTemplate" => "[FILTERED]",
      "pageRanges" => "2-3"
    },
    :source_type => :html,
    :unhandled_runtime_exceptions => "[FILTERED]",
    "frameId" => "43E1D2D96EBAEDF4BB82A95B74DE397A",
    "sessionId" => "CE865B72566BD21A8272A3F7EEF30EBE"
  }
}

    (chromic_pdf 1.8.1) lib/chromic_pdf/pdf/browser/channel.ex:23: ChromicPDF.Browser.Channel.run_protocol/3
    (chromic_pdf 1.8.1) lib/chromic_pdf/pdf/browser/session_pool.ex:115: ChromicPDF.Browser.SessionPool.do_run_protocol/4
    (nimble_pool 1.0.0) lib/nimble_pool.ex:349: NimblePool.checkout!/4
    (chromic_pdf 1.8.1) lib/chromic_pdf/pdf/browser/session_pool.ex:55: ChromicPDF.Browser.SessionPool.run_protocol/3
    (chromic_pdf 1.8.1) lib/chromic_pdf/api.ex:67: anonymous fn/3 in ChromicPDF.API.chrome_export/4
    (chromic_pdf 1.8.1) lib/chromic_pdf/api/telemetry.ex:10: anonymous fn/2 in ChromicPDF.Telemetry.with_telemetry/3
    (telemetry 1.1.0) /Users/bernhardhackl/src/chromic_pdf/deps/telemetry/src/telemetry.erl:320: :telemetry.span/3
    iex:4: (file)
18:12:20.252 [debug] [ChromicPDF] msg out: %{"id" => 24, "method" => "Target.closeTarget", "params" => %{targetId: "43E1D2D96EBAEDF4BB82A95B74DE397A"}}

This PR fixes this and instead of waiting for a timeout handles errors sent via rpc right away. There's one failing test, but as far as I can tell we should rewrite the test as it basically tests for the behaviour that I fixed here.

[maltoe]

Hey @tonnenpinguin,

thanks for the contribution :purple_heart:! Looks good and is clearly a way to do this, though I'd prefer a different solution. Let me know what you think:

• Side-note on intercept_exception_thrown: I've recently added this handler before every await_notification function to look for a specific error notification which may occur at any time. This could eventually be made more generic to intercept other failed notifications, though we do receive a bunch of them that from ChromicPDF's POV are fine - as in, they don't matter for PDF printing. So for now this is specifically for the JS exceptions.
• The error you're after is a response error, i.e. it carries the id field matching it to a call we sent previously. This we can better handle inside the normal await_response handler. In fact, there is already some code that does handle errors in responses. See this mechanism https://github.com/bitcrowd/chromic_pdf/blob/4df426e7f14ce26edb01bab2777d6ff8ace80101/lib/chromic_pdf/pdf/protocol_macros.ex#L193-L197 in ProtocolMacros, and this use of it in the Navigate protocol (where for some reason we only treat messages with errorText != nil as an error :shrug: ).

So, I'd suggest to do the following:

• extend JsonRPC.is_response? to also return true for error responses (= id matches and result or error key present)
• add a default implementation for the await_response callback, in the lines I linked in ProtocolMacros. In other words, if the protocol does not export a callback, use a default one. You can probably get away with a cond instead of the if and make JsonRPC.is_error? the second condition.
• error handling should then already be implemented, i.e. you can probably drop your error_returned extras in ChromeError.

Benefits of this approach

• It leaves us the option to ignore errors, or only treat them as errors under certain conditions.
• We don't run this code for every message we receive. Now, this also means that we won't see (yet unknown) errors for calls that don't define a await_response, but that's a trade-off between performance and developer experience where I'd lean towards performance (= reducing needlessly executed code). Just consider that after you've dealt with your "page range exceeds page count" error, it is unlikely to happen ever again. At least in our experience, we usually set up a template and once it works, it works.

[maltoe]

Moin moin nach Hamburg im Übrigen

[tonnenpinguin]

Moin @maltoe!

Thanks for the detailed feedback, that helps a lot when playing around with a new codebase :)

I applied the suggested changes, let me know what you think! :)

A couple random things I noticed

• ChromicPDF.Browser.Channel.warn_on_inspector_crash!/1 has an trailing bang even though there's no assertions in the function body
• ChromicPDF.Connection.JsonRPC.is_response?/2 et al have a is_ prefix even though they are not valid in guard clauses -> should be response? or maybe has_response?

[maltoe]

A couple random things I noticed

Feel free to fix these, if you like.

[maltoe]

merged to main manually, thanks again :purple_heart: