-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Fix Slow logout on Chrome-like browsers #42544
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the HTTPS check :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with the changes by @artonge
I signed-off your commit. @artonge |
Signed-off-by: Gaspard d'Hautefeuille <github@dhautefeuille.eu>
nextcloud#41196 + keep https check Co-authored-by: Louis <louis@chmn.me> Signed-off-by: Gaspard d'Hautefeuille <github@dhautefeuille.eu>
807ff54
to
08ff644
Compare
Drone is not triggered. Merging. |
Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 |
Drone is actually red due to this PR:
Drone is configured to no longer work on forks due to some security implications. It is recommended to fork the PR into our org for now when they are good. Can you follow up to make CI green again? |
/backport to stable28 |
if ( | ||
$this->request->getServerProtocol() === 'https' && | ||
!$this->request->isUserAgent([Request::USER_AGENT_CHROME, Request::USER_AGENT_ANDROID_MOBILE_CHROME]) | ||
) { | ||
$response->addHeader('Clear-Site-Data', '"cache", "storage"'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should have been fixed in the master pr. Instead of excluding Chrome just drop the cache part.
Thats what slows chrome down, because of a 7 years old chrome bug...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
Summary
It cancels PR #37405, removes all the regression code.
This issue is it has been 9 months than the logout takes 30 seconds on Chrome-like browsers since this PR.
If anything was interesting in the highly damaging PR #37405, I highly recommend to add any feature-related PR in a separate PR, as this one is only meant to fix a critical bug, which makes Nextcloud unusable.
Checklist
@karlitschek