Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't manipulate exception backtrace. #13222

Closed
wants to merge 1 commit into from

Conversation

ioquatix
Copy link

@ioquatix ioquatix commented Nov 30, 2023

Using set_backtrace causes Exception#backtrace_locations to become nil which is a great loss for accurate error reporting. From what I've seen very little is gained by adding the backtrace from the server. I don't feel that this is a breaking change as no one could reasonably depend on this information anyway. However, by removing this, not only is the code simpler, test frameworks will be able to better consume the backtrace. For example, sus uses this to highlight the error in VSCode.

If you did decide you want to preserve the error, maybe a more elaborate approach is warranted. I would suggest the following:

  1. Try to extract the call stack from the browser, however if no methods resolve, exit here.
  2. Once at least some methods have resolved to something meaningful, append that to the message, e.g. klass.new(messsage + "\n\n#{remote_backtrace}).

Alternatively, you could extend the error classes to have the remote/server backtrace as an ivar, e.g.

class Selenium::WebDriver::RemoteError
  def initialize(message, remote_backtrace: nil)
    super(message)
    @remote_backtrace = remote_backtrace
  end

  attr :remote_backtrace
end

Then you would subclass all your existing errors from that, or something to that effect. This would preserve the normal Ruby backtrace_locations behaviour while still exposing the remote backtrace to those who wanted to consume it.

It also occurred to me there is one other potential option: Create two exception objects, one with the remote backtrace, and then use klass.new(..., cause: remote_error). This would also hide the remote backtrace unless the code specifically looked for it. I still don't recommend sticking non-Ruby backtrace data in backtrace though.

Fixes #13221

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Using `set_backtrace` causes `Exception#backtrace_locations` to become `nil` which is a great loss for accurate error reporting. From what I've seen very little is gained by adding the backtrace from the server.
@CLAassistant
Copy link

CLAassistant commented Nov 30, 2023

CLA assistant check
All committers have signed the CLA.

@titusfortner titusfortner requested a review from p0deje November 30, 2023 16:55
@p0deje
Copy link
Member

p0deje commented Nov 30, 2023

There is some useful information when it's used with Grid as it provides more detailed backtrack. I think that using cause probably makes more sense, but this would be a backwards incompatible change. Let's maybe do some cleanup to strip down useless lines in the backtrace like you suggest in:

Try to extract the call stack from the browser, however if no methods resolve, exit here.

@titusfortner
Copy link
Member

and just as context, the current implementation was created to solve #3683

@ioquatix
Copy link
Author

ioquatix commented Nov 30, 2023

I respect your position that using cause can be a compatibility issue. However, most loggers (including those from test runners) usually follow the causality chain when printing exceptions, i.e. something like:

def print(exception)
  $stderr.puts exception.detailed_message
  if cause = exception.cause
    $stderr.puts "Caused by"
    print(cause)
  end
end

Therefore, in terms of visibility, the information is usually still presented. Using cause also makes it easier to distinguish between the remote error and the local error.

I'm happy to update then PR to use cause if that's the direction you want to go in. LMK.

@p0deje
Copy link
Member

p0deje commented Dec 1, 2023

I can see that at least IRB and RSpec properly shows exception cause:

irb(main):001:0> foo = StandardError.new
=> #<StandardError: StandardError>
irb(main):002:0> foo = StandardError.new("FOO")
=> #<StandardError: FOO>
irb(main):003:0> bar = StandardError.new("BAR", cause: foo)
(irb):3:in `initialize': wrong number of arguments (given 2, expected 0..1) (ArgumentError)
	from (irb):3:in `new'
	from (irb):3:in `<main>'
	from /Users/p0deje/.asdf/installs/ruby/3.0.4/lib/ruby/gems/3.0.0/gems/irb-1.5.0/exe/irb:11:in `<top (required)>'
	from /Users/p0deje/.asdf/installs/ruby/3.0.4/bin/irb:25:in `load'
	from /Users/p0deje/.asdf/installs/ruby/3.0.4/bin/irb:25:in `<main>'
irb(main):004:0> bar = RuntimeError.new("BAR")
=> #<RuntimeError: BAR>
irb(main):005:1* begin
irb(main):006:1*   raise foo
irb(main):007:1* rescue
irb(main):008:1*   raise bar
irb(main):009:0> end
(irb):8:in `rescue in <top (required)>': BAR (RuntimeError)
	from (irb):5:in `<main>'
	from /Users/p0deje/.asdf/installs/ruby/3.0.4/lib/ruby/gems/3.0.0/gems/irb-1.5.0/exe/irb:11:in `<top (required)>'
	from /Users/p0deje/.asdf/installs/ruby/3.0.4/bin/irb:25:in `load'
	from /Users/p0deje/.asdf/installs/ruby/3.0.4/bin/irb:25:in `<main>'
(irb):6:in `<main>': FOO (StandardError)
	from /Users/p0deje/.asdf/installs/ruby/3.0.4/lib/ruby/gems/3.0.0/gems/irb-1.5.0/exe/irb:11:in `<top (required)>'
	from /Users/p0deje/.asdf/installs/ruby/3.0.4/bin/irb:25:in `load'
	from /Users/p0deje/.asdf/installs/ruby/3.0.4/bin/irb:25:in `<main>'
it 'foo' do
  foo = StandardError.new("FOO")
  bar = RuntimeError.new("BAR")
  begin
    raise foo
  rescue
    raise bar
  end
end

# Produces:
#  Failure/Error: raise bar
#
#  RuntimeError:
#    BAR
#  # /Users/p0deje/Development/SeleniumHQ/selenium/rb/spec/unit/selenium/devtools_spec.rb:51:in `block in Selenium'
#  # /private/var/tmp/_bazel_p0deje/d9f7ca256f773f0ccaaf86cfc9fd2df8/external/bundle/jruby/3.1.0/gems/webmock-3.19.1/lib/webmock/rspec.rb:39:in `block in <main>'
#  # ------------------
#  # --- Caused by: ---
#  # StandardError:
#  #   FOO
#  #   /Users/p0deje/Development/SeleniumHQ/selenium/rb/spec/unit/selenium/devtools_spec.rb:49:in `block in Selenium'

With that said, I propose to rework the exception handling and make remote/driver error a cause of a usual exception.

@pujagani pujagani added the C-rb label Dec 6, 2023
@titusfortner
Copy link
Member

@ioquatix is there a change that needs to be made still?

@ioquatix
Copy link
Author

ioquatix commented Dec 31, 2023

Did the exception backtrace get fixed? Not sure if this PR is still relevant but not sure if the original problem is fixed.

@titusfortner
Copy link
Member

No, I don't think we've done anything in the past month. I'm just going through all the open issues and trying to figure out what needs to happen on each of them.

With that said, I propose to rework the exception handling and make remote/driver error a cause of a usual exception

Are you interested in updating the PR to do what Alex suggested?

@diemol
Copy link
Member

diemol commented Feb 16, 2024

@ioquatix would you like to continue with this PR?

@ioquatix
Copy link
Author

I would like to, but I do not have time right now. Feel free to rework the PR if you want. Otherwise, I may try to revisit it in the future. Sorry about that!

@aguspe
Copy link
Contributor

aguspe commented Jun 22, 2024

Hi @ioquatix I hope it's okay that I tried to give it a go at this to see if we can get it fixed, if you will keep working on this I will gladly roll back my PR

@p0deje
Copy link
Member

p0deje commented Jul 16, 2024

Replaced by #14170

@p0deje p0deje closed this Jul 16, 2024
@ioquatix ioquatix deleted the patch-1 branch July 17, 2024 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Fairly meaningless backtrace information is added at the expense of backtrace locations.
7 participants