Hrm...Curiously the path that takes a cookie line uses the scheme of the URL to determine if it should find the ETLD+1 of the host.  SetCanonicalCookieAsync doesn't have this information, so if we have an extensions scheme, weird things could happen.

(this was somewhat misdirected to the possible leak bug previously, so I am repeating myself a bit here)

So I think this may be more important than previously thought, as the following websites I tried all hit SetCanonicalCookieAsync:
cnn.com, google.com(!), news.bbc.co.uk; I think google analytics also uses document.cookie --- and a lot of sites use google analytics.

So with respect to the scheme and the like... Right, SetCanonicalCookieAsync doesn't have a scheme to pass 
to cookie_util::GetEffectiveDomain like DoCookieCallbackForURL ... but... neither does the store when loading
the cookies and figuring out which host keys correspond to which arguments to PersistentCookieStore::LoadCookiesForKey 
--- it just does this (with error handling cut out):

  sql::Statement smt(
      db_->GetUniqueStatement("SELECT DISTINCT host_key FROM cookies"));

  while (smt.Step())
    host_keys.push_back(smt.ColumnString(0));

  for (size_t idx = 0; idx < host_keys.size(); ++idx) {
    const std::string& domain = host_keys[idx];
    std::string key = registry_controlled_domains::GetDomainAndRegistry(
        domain, registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);

    keys_to_load_[key].insert(domain);
  }


... so given std::unique_ptr<CanonicalCookie> cookie, wouldn't 
registry_controlled_domains::GetDomainAndRegistry(
        cookie->Domain(), registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
be the right key to use? 

If so, I can make a CL, though this feels a little brittle/poorly abstracted; it's actually 
hard to convince myself that the existing URL -> key derivation is correct; though that we only ever 
seem to use chrome-extension as a non-standard cookieable scheme likely reduces what matters in practice.

Per comment on the other bug, extensions can set cookies using the extension scheme, and I'm not sure it's safe to call GetDomainAndRegistry on extension "domains".

If my code reading + unit testing is right, I think it will produce "", which is what you would want to pass to the present implementation of sqllite store to get it to load in those cookies; I think what cookie_util::GetEffectiveDomain computes is different, and not actually what the backend expects in this case, so we may have some weird bugs around not loading extension cookies right (which may be masked by this very bug).

.... And of course SetCanonicalCookie and URL methods have to be consistent, too, or one gets data coherence bugs, which try bots helpfully pointed at my first try of only changing SetCanonicalCookie.

I should probably make a test extension or something, though.

I don't think we want it to be "" - different extensions should not share cookies, right?  Or am I misunderstanding.

It just means they get loaded together, that's all. To be specific, here is the extension cookie jar after a tiny test extension added a cookie:

$ sqlite3 -cmd ".dump cookies"  ~/.config/chromium/Default/Extension\ Cookies
...
INSERT INTO cookies VALUES(13164315862949421,'aknmeieabphheepdkibhjbpoagohaido','','','/',0,0,0,13164315862949421,0,0,1,X'763131c78fafdc7dd41a51fd08957b10bd332e',0);
...

aknmeieabphheepdkibhjbpoagohaido is the extension ID, and it's in the host_key column, where domain or host for normal cookies would go.

When this is opened by SQLitePersistentCookieStore::Backend::InitializeDatabase, it will fill in this structure:

// Map of domain keys(eTLD+1) to domains/hosts that are to be loaded from DB.
std::map<std::string, std::set<std::string>> keys_to_load_;

using what was pseudocoded in comment #7; which means for this example you'll end up with 
"" => { "aknmeieabphheepdkibhjbpoagohaido" }

For something more normal, you may end up with something like

"google.com" => { ".chrome.google.com", ".google.com", ".www.google.com" }
"cnn.com" => { ".cnn.com", "www.cnn.com" }

If you call SQLitePersistentCookieStore::LoadCookiesForKey("google.com") it would then fetch all the rows where the host_key field 
matches one of .chrome.google.com, .google.com or .www.google.com, and then it would be up to the CookieMonster to actually figure out 
what to do with them.

Similarly, "" is what SQLitePersistentCookieStore::LoadCookiesForKey would expect for key to load cookies for that extension --- not 
"aknmeieabphheepdkibhjbpoagohaido" which what the present use of cookie_util::GetEffectiveDomain in DoCookieCallbackForURL computes AFAICT incorrectly.

Ultimately I think we want some function that can be computed off both CanonicalCookie::Domain() and off URLs and produce consistent results.

Sketch of fix: https://chromium-review.googlesource.com/c/chromium/src/+/940141

Needs a bunch of comment updates, and a testcase for the extensions thing... which is turning out tricky since the background fetch of all the rows basically
seems to fetch those relevant to extensions first anyway (making the side-bug 
likely latent in practice).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/323efafeada25cc73ca76d4a1d6e00bd7bff42d4

commit 323efafeada25cc73ca76d4a1d6e00bd7bff42d4
Author: Maks Orlovich <morlovich@chromium.org>
Date: Tue Mar 06 02:56:39 2018

CookieMonster: Don't load entire cookie jar on SetCanonicalCookieAsync

Before this change, the SQLite store and the cookie
monster had different notions of how exactly |key| passed to
PersistentStore::LoadCookiesForKey should be computed[1], and the
version used by CookieMonster was not really computable from a
CanonicalCookie.  This changes both of these to use
CookieMonster::GetKey, which is already used with both
CanonicalCookie's and URLs, and can therefore be easily called from
SetCanonicalCookieAsync as well.

[1] In particular they disagreed on chrome-extension:// cookies, but
that appeared to have been deterministically masked by implementation
details of the SQLite cookie store, so doesn't seem to have been a bug
that actually affected our users.

Bug:  813141 
Change-Id: Iaa3840a1e1e28992c99413142a62fddff97c7f78
Reviewed-on: https://chromium-review.googlesource.com/940141
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541036}
[modify] https://crrev.com/323efafeada25cc73ca76d4a1d6e00bd7bff42d4/net/cookies/cookie_monster.cc
[modify] https://crrev.com/323efafeada25cc73ca76d4a1d6e00bd7bff42d4/net/cookies/cookie_monster.h
[modify] https://crrev.com/323efafeada25cc73ca76d4a1d6e00bd7bff42d4/net/cookies/cookie_monster_unittest.cc
[modify] https://crrev.com/323efafeada25cc73ca76d4a1d6e00bd7bff42d4/net/extras/sqlite/sqlite_persistent_cookie_store.cc
[modify] https://crrev.com/323efafeada25cc73ca76d4a1d6e00bd7bff42d4/net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc

I would like to request backport of this to M66, since I think document.cookie setting is extremely common, and because the long-tail delays of loading the full cookie jar are quite bad.

mmenke@, do you agree? Anyone else feel like objecting or supporting?

I think that's reasonable, particularly given how recently M66 was cut.

Pls apply appropriate OSs label.

I tried to do the clicking, in the interest of saving roundtrip time.

As far as I understand, we don't handle document.cookies on iOS -- the WebView there does. Wo do expose SetCanonicalCookieAsync on iOS, so the performance fix is still relevant, even though it might have a smaller impact.

(Thanks!)
Isn't iOS CookieStoreIOS and subclasses rather than CookieMonster, though?

CookieStoreIOS and CookieStoreIOSPersistent both include a CookieMonster. One of them uses the system cookie store, so it doesn't block on CookieMonster operations, but both of them run the CookieMonster code.

If the OS box means "merging this code will change the code we run in the iOS build", I think the answer is yes. If it means "we're solving a problem in the iOS build" the answer is no. WDYT?

Your change meets the bar and is auto-approved for M66. Please go ahead and merge the CL to branch 3359 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/50a9255cfb4a934fc9ce25769f4cbda2386249c9

commit 50a9255cfb4a934fc9ce25769f4cbda2386249c9
Author: Maks Orlovich <morlovich@chromium.org>
Date: Sun Mar 11 12:12:18 2018

CookieMonster: Don't load entire cookie jar on SetCanonicalCookieAsync

Before this change, the SQLite store and the cookie
monster had different notions of how exactly |key| passed to
PersistentStore::LoadCookiesForKey should be computed[1], and the
version used by CookieMonster was not really computable from a
CanonicalCookie.  This changes both of these to use
CookieMonster::GetKey, which is already used with both
CanonicalCookie's and URLs, and can therefore be easily called from
SetCanonicalCookieAsync as well.

[1] In particular they disagreed on chrome-extension:// cookies, but
that appeared to have been deterministically masked by implementation
details of the SQLite cookie store, so doesn't seem to have been a bug
that actually affected our users.

Bug:  813141 
Change-Id: Iaa3840a1e1e28992c99413142a62fddff97c7f78
Reviewed-on: https://chromium-review.googlesource.com/940141
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#541036}(cherry picked from commit 323efafeada25cc73ca76d4a1d6e00bd7bff42d4)
Reviewed-on: https://chromium-review.googlesource.com/958601
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#149}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/50a9255cfb4a934fc9ce25769f4cbda2386249c9/net/cookies/cookie_monster.cc
[modify] https://crrev.com/50a9255cfb4a934fc9ce25769f4cbda2386249c9/net/cookies/cookie_monster.h
[modify] https://crrev.com/50a9255cfb4a934fc9ce25769f4cbda2386249c9/net/cookies/cookie_monster_unittest.cc
[modify] https://crrev.com/50a9255cfb4a934fc9ce25769f4cbda2386249c9/net/extras/sqlite/sqlite_persistent_cookie_store.cc
[modify] https://crrev.com/50a9255cfb4a934fc9ce25769f4cbda2386249c9/net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc