Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: backport patch that ensures that cookie store is always created #15836

Merged
merged 6 commits into from Nov 29, 2018
Merged

fix: backport patch that ensures that cookie store is always created #15836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
af3529d
fix: backport patch that ensures that cookie store is always created
brenca Nov 26, 2018
24a0e59
fix: disable cookie encryption
deepak1556 Nov 26, 2018
9fc265e
Update patches/common/chromium/ensure_cookie_store.patch
nornagon Nov 26, 2018
9522ea4
fix: flush the cookie store when NetworkContext shuts down
brenca Nov 26, 2018
20896fe
test: add test for cookie store persistance
brenca Nov 26, 2018
776811c
Update patches/common/chromium/ensure_cookie_store.patch
nornagon Nov 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions atom/browser/net/url_request_context_getter.cc
View file
Open in desktop
Expand Up @@ -188,6 +188,9 @@ URLRequestContextGetter::Handle::CreateNetworkContextParams() {
base_path.Append(chrome::kChannelIDFilename);
network_context_params->restore_old_session_cookies = false;
network_context_params->persist_session_cookies = false;
// TODO(deepak1556): Matches the existing behavior https://git.io/fxHMl,
// enable encryption as a followup.
network_context_params->enable_encrypted_cookies = false;
}

// TODO(deepak1556): Decide the stand on chrome ct policy and
Expand Down Expand Up @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deepak1556 @brenca is this ready to merge or do more changes need to be made?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am good with the current patch, needs to be rebased to fix conflicts before merge.

}

context_shutting_down_ = true;
resource_context.reset();
net::URLRequestContextGetter::NotifyContextShuttingDown();
Expand Down
1 change: 1 addition & 0 deletions patches/common/chromium/.patches
View file
Open in desktop
Expand Up @@ -76,3 +76,4 @@ cross_site_document_resource_handler.patch
content_allow_embedder_to_prevent_locking_scheme_registry.patch
fix_trackpad_scrolling.patch
mac_fix_form_control_rendering_on_10_14_mojave.patch
ensure_cookie_store.patch