search

GoogleChromeLabs / chromium-bidi

Implementation of WebDriver BiDi for Chromium
https://googlechromelabs.github.io/chromium-bidi/
Apache License 2.0
90 stars 30 forks source link

fix: do not throw an error when unsubscribing from event if the CDP session is closed #2606

Closed sadym-chromium closed 3 weeks ago

sadym-chromium commented 3 weeks ago

This causes a false-negative WPT tests in case of testdriverJS BiDi.

Lightning00Blade commented 3 weeks ago

I don't think this is the way we want to go, we would want to create a property on the CDP target to know what the status update or not, we already do that for Cache and Network, and Fetch, we should do it here as well. I guess we are sending a DeviceAccess.disable and that is causing the error, having a cached prevent from sending it.

sadym-chromium commented 3 weeks ago

I don't think this is the way we want to go, we would want to create a property on the CDP target to know what the status update or not, we already do that for Cache and Network, and Fetch, we should do it here as well. I guess we are sending a DeviceAccess.disable and that is causing the error, having a cached prevent from sending it.

If the Target.detachedFromTarget is received BEFORE the command, the browsing context is not iterated. The problem happens when there is a race condition between command and Target.detachedFromTarget event. E.g. in this case the timing is the following:

  1. Send command DeviceAccess.disable.
  2. Received event Target.detachedFromTarget.
  3. Received command response "Session with given id not found".

Having that, we can either add a heuristic checking the exception, or add a client status. But in the second option we will have to make the check AFTER the command is finished. So basically we will end up with the same heuristic, as we don't want to silence other kind of exceptions.

Lightning00Blade commented 3 weeks ago

Can you take a look at this - https://github.com/GoogleChromeLabs/chromium-bidi/pull/2609 I think this will solve the problem you are trying to fix here, but as it prevents sending usless command to CDP.

Lightning00Blade commented 3 weeks ago

If we want we should have a general solution to the problem of unsubscribing as there are other commands that have similar behaviour.

sadym-chromium commented 3 weeks ago

Can you take a look at this - #2609 I think this will solve the problem you are trying to fix here, but as it prevents sending usless command to CDP.

I added a commit with test failing but expecting to pass. The PR does not address the race condition.