-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Session cookies are persisted on session restore #28379
Comments
Note for @brave/qa-team - Need to run test cases 1, 2, 4 on Linux. Test case 3 is N/A. Should run 1, 2, 3, 4 on Windows and macOS. Test case 1 (quit+open)
Test case 2 (brave://restart)
Test case 3 (browser update)
Test case 4 (disabled feature, old behavior)
|
Verified with
Verified test plan from #28379 (comment). Test Case 1 (quit + open) - PASSED
Note, confirmed with @goodov that
Test case 2 (brave://restart) - PASSED
Test case 3 (browser update) - FAILED, follow up issue logged
Test case 4 (disabled feature, old behavior) - PASSED
Logs from step 5:
Regression set 1 - "Open the New Tab page" setting - PASSEDCase 1:
Case 2:
Case 3:
Regression set 2 - "Open a specific page or set of pages" setting - PASSEDCase 1:
Case 2:
Case 3:
|
I dig a little and found a PR which tries to fix the restart on MacOS. Right now it doesn't work as expected, instead it's pretty crazy how the "update restart" currently actually works on MacOS (in a bad way). https://github.com/brave/brave-core/pull/15257/files#r1140262599 |
Verification
Verified test plan from #28379 (comment). Test Case 1 (quit + open) - PASSED1. Install 1.50.101 2. launch Brave 3. visit `brave://settings` and confirm "Continue where you left off" is enabled 4. Open https://bayden.com/test/sessions/ and choose `lightcyan` from selector. 5. Ensure `sessionCookie says color is: lightcyan`. 6. Fully quit browser. 7. Open browser. 8. Expect the session is restored and the website is still opened. 9. Refresh the page (Ctrl/Cmd+R). 10. Expect `sessionCookie` **has changed to** `undefined`.Note, confirmed with @goodov that
Test Case 2 (Exit + open) - PASSED
Note, confirmed with @goodov that
Test case 3 (brave://restart) - PASSED
Test case 4 (browser update) - PASSED
Test case 5 (disabled feature, old behavior) - PASSED
Regression set 1 - "Open the New Tab page" setting - PASSEDCase 1: -
Case 2: -
Case 3: -
Regression set 2 - "Open a specific page or set of pages" setting - PASSEDCase 1:
Case 2:
Case 3:
|
that's weird. Can you please check this is what I got: vBKXxaCsSt.mp4
The failed regression cases where you use MacOS as a baseline are initially not exactly correct. The browser should restore an old session after a restart triggered by an update no matter what the current restore mode is selected. The reason why the browser doesn't do it on MacOS is because of this. So this inconsistency with MacOS is currently expected, but we should use Windows version as the baseline in the update-restart test cases. The browser should restore the active session during update-restart no matter what the session restore option is selected. That's the thing we currently miss on MacOS and is visible only when "Continue where you left off" is not selected. |
@goodov 2023-03-28_09h04_06.mp4 |
Thank you for the video! I've reproduced it, but it's not related to the new feature. For some reason, session cookies set in the very first launch are not preserved on a first restart. This behavior is also exists in the current Release builds ( Please rerun the test case, but do a restart before running the test case to ensure that it's not affected by this first run issue. Please create the issue for this bug in |
Verification PASSED on
Verified test plan from #28379 (comment). Test Case 1 (quit + open) - PASSED
Note, confirmed with @goodov that
Test case 2 (brave://restart) - PASSED
Test case 4 (disabled feature, old behavior) - Encountered #29392
|
@goodov are session cookies expected to persist between browser restarts on MacOS? I have |
Screen.Recording.2023-07-17.at.5.58.07.PM.mov |
yes. We need to fix the update process on MacOS for this feature to work properly. #25576 |
That issue deals with the case where there's a pending browser update though right? I don't have a pending browser update. I'm able to repro with Brave Release as well. |
@ShivanKaul afaik this issue was not implemented on macOS due to the issue I faced in #29135 which I believe is related to what @goodov noted in #28379 (comment). There was a griffin PR to disable this feature on macOS - brave/brave-variations#557, meaning the old (previous) behavior is expected on macOS, but not Win/Linux. @goodov please correct me if I am not remembering this correctly. |
That's v helpful, thanks @LaurenWags! I've updated this issue's description to make that clearer. |
good call, thanks @ShivanKaul. We should have clarified what happened previously 👍🏻 |
We don't enable the feature because it breaks the update-restart case where session cookies should be kept. |
I was looking into this change, as it breaks my organisation's use of Brave, and wanted to control it via policy. I found https://support.brave.com/hc/en-us/articles/360039248271-Group-Policy but it seems that this specific setting is not exposed via a policy ? |
@Geod24 can you create a new issue? It would be great if you can also mention what platforms you're interested in, and any other details. |
Chromium implements session cookie restore to persist them during updates (browser restart), but this is also applied when "Continue where you left off" is enabled. It's unclear if this is intentional, but this in fact allows these cookies to live much longer than other "persisted" cookie variants.
You can try this test https://bayden.com/test/sessions/
All Chromium browsers persists session cookies (but not sessionStorage) across page closes and browser resets in "Continue where you left off" mode.
An article from Eric Lawrence which also covers this topic:
https://textslashplain.com/2019/06/24/surprise-undead-session-cookies/
Note (by @ShivanKaul): this issue was fixed by brave/brave-core#17122 for all platforms except MacOS.
The text was updated successfully, but these errors were encountered: