1617611 - Fix all the tests broken by "cookies sameSite=lax by default"

Status: RESOLVED WONTFIX

(Core :: Networking: Cookies, task, P3)

Description

[Andrea Marchesini [:baku]]

Bug 1604212 disables "cookie sameSite=lax by default" feature in mochitests, and xpcshell-tests. Before enabling this feature in release, we need to fix the tests.

Comment 1

[Shane Caraveo (:mixedpuppy)]

This priority level concerns me. Tests were broken by bug 1604212, and fixed by prefing off that change for those tests. The last time a feature was turned on, but disabled in our tests, it broke an api. That caused me to start watching all changes to any extension related code in phab.

I'd prefer to see this as a P1, at least to the extent that we verify no extension api is broken.

Comment 2

[Dragana Damjanovic [:dragana]]

Sorry this is my bad, I sometime put work by other teams to be P3 for necko, but this one is not that.

Comment 3

[Dragana Damjanovic [:dragana]]

Assigning this to you. If this is a meta bug please mark as a [meta]. If someone else is going to work on it please reassign. If it should not be P1 please reclassify it.

(This is a P1 bug so we need an assignee).

Comment 4

[Niklas]

Attachment: Bug 1617611: Enable SameSite=Lax by default in mochitests. r=ckerschb!

Comment 5

[Niklas]

Attachment: Bug 1617611: Annotate each failing test individually. r=ckerschb!

Annotating each test individually lets us avoid introducing new failing tests
while we go through the backlog of failing tests.

Depends on D129162

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c3352f4b3681
Enable SameSite=Lax by default in mochitests. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/fdb2c6bfc0c6
Annotate each failing test individually. r=webdriver-reviewers,necko-reviewers,ckerschb,whimboo,valentin

Comment 7

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing mochitest failures on storageAccessWithDynamicFpi.js

Backout link : https://hg.mozilla.org/integration/autoland/rev/bb05a9c04907014611396dec1fbfe65b0eba8295

Push with failures : https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=fdb2c6bfc0c6fc1d2511a5878ace937d397cfe61&selectedTaskRun=bXykI63mRsiLuaV0oaecKg.0

Link to failure log : https://treeherder.mozilla.org/logviewer?job_id=358163496&repo=autoland&lineNumber=5282

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/849b1fc5760f
Enable SameSite=Lax by default in mochitests. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/fb19eba784ee
Annotate each failing test individually. r=webdriver-reviewers,necko-reviewers,ckerschb,whimboo,valentin

Comment 9

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/849b1fc5760f
https://hg.mozilla.org/mozilla-central/rev/fb19eba784ee

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0e09e99a2fd6
Port bug 1617611 - Annotate the tests broken by "cookies sameSite=lax by default". rs=bustage-fix

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The tests haven't been fixed yet, but specific preferences for cookies have been set for individual tests that are failing with lax by default. As such I assume this bug should not be closed yet.

Comment 12

[Niklas]

Attachment: Bug 1617611: Enable SameSite=Lax by default in xpcshell tests. r=ckerschb!

Comment 13

[Niklas]

Attachment: Bug 1617611: Annotate each failing xpcshell test individually. r=ckerschb!

Depends on D131491

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1ef9a4454c3
Enable SameSite=Lax by default in xpcshell tests. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/dbfe6b7dcadc
Annotate each failing xpcshell test individually. r=ckerschb,extension-reviewers,robwu

Comment 15

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/b1ef9a4454c3
https://hg.mozilla.org/mozilla-central/rev/dbfe6b7dcadc

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

With all tests being correctly annotated and all patches belonging to this bug landed (see comment 9 and comment 15). I am removing the leave-open keyword and mark this bug as fixed.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Which bug will be used to actually fix the broken tests? When I was asking to add a comment next to the disabled Remote Protocol browser chrome tests this bug was mentioned. So I'm wondering about the next steps. Thanks.

Comment 18

[Niklas]

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

Sorry for the back and forth, we will keep this bug open to track the fixing. As you mentioned all the comments added in the patches reference this bug, so it makes sense to keep it open.

Comment 19

[Niklas]

Attachment: Bug 1617611: Fix dom/security/test/general and dom/security/test/https-first tests. r=freddyb!,ckerschb

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:dragana, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P1'.
:dragana, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 22

[Dragana Damjanovic [:dragana]]

Christoph, can you find someone to finish this?

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Dragana Damjanovic [:dragana] from comment #22)

Christoph, can you find someone to finish this?

Tom, is this something we even want to land after all? Or can we close this one out?

Comment 24

[Tom Schuster (MoCo)]

This doesn't seem close to being done at all to me. There are still at least 50 tests that disable laxByDefault. For a lot of these however we already disable other cookie prefs as well, so fixing just laxByDefault doesn't seem too important to me. We can keep this in the backlog for now.

Comment 25

[Tom Schuster (MoCo)]

Attachment: Bug 1617611 - Fix all tests using laxByDefault=false under browser/. r?freddyb

This turned out to be more complicated than expected, because:

• SameSite=None requires https://
• Many test were using the mochi.test domain, which is not reachable via https

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/776e34d36cb6
Fix all tests using laxByDefault=false under browser/. r=freddyb

Comment 27

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/776e34d36cb6

Comment 28

[Valentin Gosu [:valentin] (he/him)]

Is there anything else we need to do here? Does this still need to be a P1?

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

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

Is there anything else we need to do here? Does this still need to be a P1?

I don't think so, though Freddy is the better person to answer here.

Comment 30

[Frederik Braun [:freddy]]

No. We are not going to ship "lax by default" anytime soon. OK to close this or down-prioritize.