fix: backport patch that ensures that cookie store is always created #15836
Conversation
|
@deepak1556 added a test for this |
brenca
force-pushed
the
brenca/cookie-store-backport
branch
from
aacb472
to
3d180a7
Compare
brenca
force-pushed
the
brenca/cookie-store-backport
branch
from
3d180a7
to
76baa64
Compare
| @@ -259,6 +262,11 @@ void URLRequestContextGetter::NotifyContextShuttingDown( | |||
| std::unique_ptr<ResourceContext> resource_context) { | |||
| DCHECK_CURRENTLY_ON(BrowserThread::IO); | |||
|
|
|||
| // todo(brenca): remove once C70 lands | |||
| if (url_request_context_ && url_request_context_->cookie_store()) { | |||
| url_request_context_->cookie_store()->FlushStore(base::NullCallback()); | |||
There was a problem hiding this comment.
Is this necessary ? I was able to confirm that cookies are persisted without any external flushing. My test scenario was, from a clean app state.
- visit https://github.com and sign-in
- exit app and restart
- account remains signed-in
There was a problem hiding this comment.
Alright, not sure what's going on here - the test that I added passes without the explicit flush, but the test app I used doesn't keep the cookies. Can you check if this works on your machine? https://gist.github.com/brenca/205ea1876c844b4a6eec58ee735f6537
There was a problem hiding this comment.
Hmm if the app is quit immediately the cookie is not persisted, must be the persistent db write period https://cs.chromium.org/chromium/src/net/extras/sqlite/sqlite_persistent_cookie_store.cc?l=1277, seems good to have an external flush.
There was a problem hiding this comment.
@deepak1556 @brenca is this ready to merge or do more changes need to be made?
There was a problem hiding this comment.
I am good with the current patch, needs to be rebased to fix conflicts before merge.
|
Looking forward to this fix! Will this trigger a new beta release? 🙂 |
Co-Authored-By: brenca <benecene@gmail.com>
Co-Authored-By: brenca <benecene@gmail.com>
brenca
force-pushed
the
brenca/cookie-store-backport
branch
from
c6f22b4
to
776811c
Compare
|
We're already working on a new beta release, actually. There have been other tooling issues that are holding that up. |
|
Release Notes Persisted
|
|
I was unable to backport this PR to "4-0-x" cleanly; |
|
Looks like this change didn't make it into v4.0.0-beta.8, correct? |
|
Yepp, should be in the next beta. |
Description of Change
Fixes #15365, but the issue actually affected all cookies. This patch ensures that the cookie store is created properly.
Checklist
npm testpassesRelease Notes
Notes: Fixed a bug that made all cookies non persistent.