Don't manipulate exception backtrace.

[ioquatix]

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 https://github.com/SeleniumHQ/selenium/issues/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

• [x] 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

• [x] 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.

[CLAassistant]

All committers have signed the CLA.

[p0deje]

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]

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

[ioquatix]

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]

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.

[titusfortner]

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

[ioquatix]

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

[titusfortner]

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]

@ioquatix would you like to continue with this PR?

[ioquatix]

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]

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]

Replaced by #14170