SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
30.18k stars 8.11k forks source link

[🚀 Feature]: Remove CDP support for Firefox browser #11736

Open pujagani opened 1 year ago

pujagani commented 1 year ago

Feature and motivation

Firefox's implementation of ChromeDevTools protocol (CDP) is half-baked and not complete. This leads to errors when trying to use certain CDP APIs and tends to cause CI failures. Firefox is focused on implementing the W3C-compliant BiDi protocol. https://wpt.fyi/results/webdriver/tests/bidi?label=experimental&label=master&aligned&view=subtest is a good way to track the progress. The BiDi protocol has the capability to provide the same real-world use cases described in https://www.selenium.dev/documentation/webdriver/bidirectional/bidi_api/ as CDP provides currently. Selenium has started working towards implementing BiDi in different languages. Since BiDi is Firefox's focus and Selenium's long-term focus (since all major browser vendors are working towards implementing the BiDi), transitioning to BiDi only for Firefox seems apt.

Usage example

The removal of CDP support for Firefox will follow Selenium's way of deprecating for at least 2 versions and then removing the support completely.
BiDi usage is documented at https://www.selenium.dev/documentation/webdriver/bidirectional/bidirectional_w3c/.

github-actions[bot] commented 1 year ago

@pujagani, thank you for creating this issue. We will troubleshoot it as soon as we can.


Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

titusfortner commented 8 months ago

@diemol based on your comment here — https://github.com/SeleniumHQ/selenium/pull/12270#pullrequestreview-1502735591

I think we should close this issue. What do you think?

whimboo commented 8 months ago

@diemol based on your comment here — #12270 (review)

Note that we are most likely going to remove our partial CDP support before WebDriver BiDi is completely implemented. It may even happen later this year. As such leaving support around in Selenium might break users.

diemol commented 8 months ago

@whimboo do you have an approximate date for that to happen? So we can synchronize.

whimboo commented 8 months ago

There is no clear date yet given that we still have a lot of work to do, but not before the next ESR 128 release. So at earliest for version 129.

whimboo commented 3 months ago

There is no clear date yet given that we still have a lot of work to do, but not before the next ESR 128 release. So at earliest for version 129.

We are in-par with our plan and the releases as mentioned above should hold. That means with Firefox 129 the CDP protocol will not be enabled by default anymore. For details see the following blog post:

https://fxdx.dev/deprecating-cdp-support-in-firefox-embracing-the-future-with-webdriver-bidi/

titusfortner commented 3 months ago

Would be really nice if we could have the network/logging/script BiDi pieces implemented in Selenium before this happens

whimboo commented 3 months ago

Please note that this is not the complete removal. You would just have to set a preference for the default profile as in use for Firefox. Then CDP will be enabled. I assume that this could be implemented easily in necessary bindings?

titusfortner commented 3 months ago

Maybe Selenium should toggle it on by default until we get bidi working? Depends on timing maybe

whimboo commented 3 months ago

That's probably a good idea. Do you or @pujagani know which APIs are still lacking WebDriver BiDi support and were implemented via CDP before?

titusfortner commented 3 months ago

So at the Dev Summit a few weeks ago we agreed on what the High Level API should look like for BiDi. We haven't implemented it in any of the bindings yet. We have low level implementations of BiDi in some of the languages. I'm ok with not toggling CDP to enabled for Firefox once we have the High Level API implemented in the language.

pujagani commented 3 months ago

I think till we get to the point of implementing the High-Level APIs that were decided. We should probably toggle the CDP support on and keep. @whimboo I think the missing piece was allowing modifying the outgoing response body https://w3c.github.io/webdriver-bidi/#command-network-provideResponse. That is one that we need to implement NetworkInterceptor that uses CDP currently.

whimboo commented 3 months ago

I see. So @juliandescottes is currently working on these pieces and we hope that network.provideResponse will be available soon. AFAIK Chrome already supports that so that you potentially could start implementing it already. Given that we have a spec it should then not require any work (beside switching to bidi for sure) to enable it for Firefox as well.

juliandescottes commented 3 months ago

@pujagani hi!

As @whimboo said, I am currently working on support for network.provideResponse, hopefully it will be available in the coming weeks.

But considering that this feature was not available in Firefox with CDP at all, I don't understand why this is a blocker to stop using CDP for Firefox?

pujagani commented 3 months ago

Thank you for the update. That's great! @juliandescottes You have a valid concern. It is not a blocker to stop using CDP with Firefox. But if we were to deprecate/remove support for CDP in Firefox, we would want to point users to the new BiDi APIs. This change will probably need a blog post to redirect users to the new API. However, what we want to do is create and redesign all the high-level APIs listed here to use BiDi. So we will need them to be fully working with the browsers, else I am sure a lot of user might report errors back to Selenium even if the browser doesn't support it. Also, the NetworkInterceptor is the one of the most popular ones. I would prefer for it be fully baked it. Again, I understand the timelines and resources available so there is no urgency. It just the missing piece we are waiting for while we figure out the rest.

whimboo commented 3 months ago

@pujagani I assume that in that case one of you will take care of getting this preference set to the required value in one of the upcoming Selenium releases? Otherwise users, who are using CDP, will have to do it on their own when creating a new session.

pujagani commented 3 months ago

Sure. We can take care of it. Any documentation link that I can refer to regarding which preference needs to be set?

whimboo commented 3 months ago

Sure. We can take care of it. Any documentation link that I can refer to regarding which preference needs to be set?

Just check the Blog post that I referenced at https://github.com/SeleniumHQ/selenium/issues/11736#issuecomment-2142936951. Thanks!

pujagani commented 3 months ago

Sorry, I missed that. Thank you!

pujagani commented 3 months ago

@whimboo "Defaults to 3 (WebDriver BiDi and CDP)" - This is mentioned in https://firefox-source-docs.mozilla.org/remote/Prefs.html. Can we use that once CDP is deprecated? To ensure both protocols are active.

whimboo commented 3 months ago

Oh yes, you should definitely use 3 for that preference to have both CDP AND WebDriver BiDi enabled. I've updated our blog post to mention that as well. Thanks for noticing!

pujagani commented 3 months ago

Great! Thank you!

pujagani commented 3 months ago

Also, since it defaults to 3 (and that's what Selenium wants to use), do we need to explicitly set the preference?

whimboo commented 3 months ago

As mentioned on the blog post starting with Firefox 129 the preference will default to 1 which means WebDriver BiDi only.

whimboo commented 3 months ago

@pujagani and the merged PR is busted because it will only enable CDP and not BiDi. So please update it to 3.

whimboo commented 3 months ago

@pujagani and the merged PR is busted because it will only enable CDP and not BiDi. So please update it to 3.

Sorry, just noticed that this linked commit above is outdated and you already updated it on https://github.com/SeleniumHQ/selenium/pull/14091.

pujagani commented 3 months ago

Okay, got it. Thank you so much! I need to fix some tests in the PRs and will then merge it.