Closed Bug 1703805 Opened 1 year ago Closed 1 year ago

Remove FTP proxying support

Categories

(Testing :: Marionette, task)

Other Branch
task

Tracking

(firefox90 fixed)

RESOLVED FIXED
90 Branch
Tracking Status
firefox90 --- fixed

People

(Reporter: valentin, Assigned: whimboo)

References

Details

Attachments

(2 files, 1 obsolete file)

Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED

When removing the support for FTP proxies we should return a proper InvalidArgument error for the New Session command, and note that this kind of proxy is no longer available.

As of now I assume we don't have to do anything for geckodriver given that we want to keep backward compatibility. But maybe we could add a comment in which Firefox release the support has been removed, so that we can eventually already return that error on the driver side without having to launch Firefox. Or we could already check for the version?

James, is there anything else that would need to be done that I missed above?

Flags: needinfo?(james)

That sounds right. On the GeckoDriver side we currently allow any proxy settings and rely on marionette to interpret them: https://searchfox.org/mozilla-central/source/testing/geckodriver/src/capabilities.rs#177 We could make that check for an ftp* field and perform a maximum version check, but I suspect this isn't used enough to make it worthwhile.

Flags: needinfo?(james)

Valentin, does the landing of this patch block all the other patches in the stack? Maybe we should uncouple it for now? I might not have time to do a further check before next week.

Flags: needinfo?(valentin.gosu)

The test will fail if I land it without this.
I can land the test changes if that's OK, followed by removing the proxy settings support.

Flags: needinfo?(valentin.gosu)

(In reply to Valentin Gosu [:valentin] (he/him) from comment #5)

The test will fail if I land it without this.
I can land the test changes if that's OK, followed by removing the proxy settings support.

I think that' fine. Can you please move the test removal patch over to the other bug then? That way we can keep this bug for the ftp proxy removal, and we will take care of the remaining bits hopefully soon. Thanks.

Assignee: valentin.gosu → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(valentin.gosu)

Comment on attachment 9217329 [details]
Bug 1703805 - Don't check marionette FTP proxy support in test_session.js r=whimboo

Revision D112906 was moved to bug 1574475. Setting attachment 9217329 [details] to obsolete.

Attachment #9217329 - Attachment is obsolete: true
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #7)

(In reply to Valentin Gosu [:valentin] (he/him) from comment #5)

The test will fail if I land it without this.
I can land the test changes if that's OK, followed by removing the proxy settings support.

I think that' fine. Can you please move the test removal patch over to the other bug then? That way we can keep this bug for the ftp proxy removal, and we will take care of the remaining bits hopefully soon. Thanks.

Thanks!

Assignee: valentin.gosu → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(valentin.gosu)
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED

As discussed I will take it.

Assignee: valentin.gosu → hskupin

Valentin, did we actually get rid of any FTP related code? We also still have the following ftp schema specific code for handling cookies:
https://searchfox.org/mozilla-central/rev/716d7cd80b3dcd81c005ad13b51d3e6a85bd7e73/testing/marionette/driver.js#1964

I assume this also needs an update?

Flags: needinfo?(valentin.gosu)

Yes, the FTP code was removed in bug 1574475.
There are a few follow-ups needed. That one should not cause any problems, but should be fixed either way.

Flags: needinfo?(valentin.gosu)

Thanks. As it looks like this check is wrong anyway because the HTML spec says:

A Document object that falls into one of the following conditions is a cookie-averse Document object:

    A Document object whose browsing context is null.
    A Document whose URL's scheme is not an HTTP(S) scheme.

So FTP should have never been accepted anyway, and as such can easily be removed.

Attachment #9214374 - Attachment description: Bug 1703805 - Remove marionette FTP proxying support r=whimboo → Bug 1703805 - Remove marionette FTP proxying support.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf0e44b8205e
Remove marionette FTP proxying support. r=marionette-reviewers,webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/644af69790cb
[wdspec] Added "Add Cookie" WebDriver tests for invalid cookie domain. r=webdriver-reviewers,jdescottes
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28975 for changes under testing/web-platform/tests
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.