Made network interception threads fail silently

[jlucartc]

When Thread.abort_on_exception is true, multiple threads may cause the application using Selenium's network interception to terminate without any means of dealing with the exception, which may leave the application in an inconsistent state. Therefore, this option is being set to false.

This change will affect only callback_thread method in websocket_connection.rb since the socket thread created in attach_socket_listener, being only one thread, causes no harm to exception handling, and thus can safely raise exceptions. Also, the rescue clause was removed from callback_thread since it won't be raising exceptions anymore.

Fixes #12220

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.

[CLAassistant]

All committers have signed the CLA.

[p0deje]

Ok, I can now see why we needed to abort the thread on exception - because we have a case where we'd like to raise from event:

selenium/rb/spec/integration/selenium/webdriver/devtools_spec.rb

Lines 48 to 56 in a04a22f

	it 'propagates errors in events', except: {browser: :firefox,
	reason: 'https://bugzilla.mozilla.org/show_bug.cgi?id=1819965'} do
	expect {
	driver.devtools.page.enable
	driver.devtools.page.on(:load_event_fired) { raise 'This is fine!' }
	driver.navigate.to url_for('xhtmlTest.html')
	sleep 0.5
	}.to raise_error(RuntimeError, 'This is fine!')
	end

Any ideas how do we suppress internal CDP errors while preserving this behavior?

[jlucartc]

I believe that something like this could solve this case:

def callback_thread(params)
  Thread.new do
    # Threads will not abort silently, so that the exception may be handled
    Thread.current.abort_on_exception = true

    # We might end up blocked forever when we have an error in event.
    # For example, if network interception event raises error,
    # the browser will keep waiting for the request to be proceeded
    # before returning back to the original thread. In this case,
    # we should at least print the error.
    Thread.current.report_on_exception = true

    yield params
 # We're gonna need a wider clause for exception handling, or else
 # the IntercepID error will not be captured and will be raised in the main thread.
  rescue Exception => e
    Thread.stop
    # No exception can be raised here, because it will lead to
    # unsafe thread exception handling. Therefore, the only way
    # to make the exception known is by informing the main thread directly.

    # If the error is a connection error, set a flag that signals the main thread.
    # Else, ignore the exception
    lock.synchronize{
        kill_connection(e.class)  if CONNECTION_ERRORS.include?(e.class) and is_connection_alive?
    }
  end
end

# Since the error capture will be syncronized, I don't think that these two methods need to be inside
# a 'lock.syncronize' block also.
def kill_connection(error_class)
    @keep_connection = false
    @connection_error_class = error_class
end

def is_connection_alive?
    @keep_connection
end

Then, the websocket_connection may try to verify @keep_connection right before each incoming message received in attach_socket_listener.

[p0deje]

Hm, maybe we simply need to do the following

      def callback_thread(params)
        Thread.new do
          Thread.current.abort_on_exception = true

          # We might end up blocked forever when we have an error in event.
          # For example, if network interception event raises error,
          # the browser will keep waiting for the request to be proceeded
          # before returning back to the original thread. In this case,
          # we should at least print the error.
          Thread.current.report_on_exception = true

          yield params
        rescue Error::WebDriverError, *CONNECTION_ERRORS
          Thread.stop
        end
      end

[jlucartc]

But how is the main thread gonna know that the exception was raised? Doesn't the test case need a raised exception?

[p0deje]

In this case, it won't know for WebDriver errors, but that's exactly the point of this PR, isn't it?

[jlucartc]

If all the tests need is a RuntimeError, then your suggestion would work. I was thinking that somehow the main thread would need to know when *CONNECTION_ERRORS were raised.

[p0deje]

I was thinking that somehow the main thread would need to know when *CONNECTION_ERRORS were raised.

No, that's why we rescue them.

[jlucartc]

I got it. Then I'm gonna send the commit with this version:

      def callback_thread(params)
        Thread.new do
          Thread.current.abort_on_exception = true

          # We might end up blocked forever when we have an error in event.
          # For example, if network interception event raises error,
          # the browser will keep waiting for the request to be proceeded
          # before returning back to the original thread. In this case,
          # we should at least print the error.
          Thread.current.report_on_exception = true

          yield params
        rescue Error::WebDriverError, *CONNECTION_ERRORS
          Thread.stop
        end
      end