Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2110483002:
Remove use of CanonicalCookie::Source() from browsing_data. (Closed)
DescriptionRemove use of CanonicalCookie::Source() from browsing_data.
Source() is only non-NULL on cookies that have been set since the
browser started. Given this weird behavior, we should remove the
field entirely, to avoid bugs (See, for instance,
https://crbug.com/624004).
The main change is that CookieTreeHostNode will now group cookies
by the domain they're set on instead of the scheme+domain+port that
set them. This was already the behavior for cookies that were loaded
from the cookie store, rather than being set since the last browser
start, so should not make much difference (Also worth nothing scheme
was effectively always http before, since https was replaced with
http, setting cookies on file URLs is not allowed, and extension
cookies use a different cookie store that bypasses this code).
BUG=620770
Committed: https://crrev.com/a1663cdfc27bb767da47200b6093fbccd57ce0b1
Cr-Commit-Position: refs/heads/master@{#404932}
Patch Set 1 #Patch Set 2 : Update test, too #Patch Set 3 : Fix #Patch Set 4 : Merge #
Total comments: 6
Patch Set 5 : Response to comments #Patch Set 6 : Missed some #Patch Set 7 : Fix android #Patch Set 8 : Really fix android #Patch Set 9 : Really really fix android? #
Total comments: 2
Patch Set 10 : Fix GURL init #Patch Set 11 : Merge #Patch Set 12 : Fix merge #Dependent Patchsets: Messages
Total messages: 56 (25 generated)
Description was changed from ========== Remove use of CanonicalCookie::Source() from CookieTreeHostNode. Source() is only non-NULL on cookies that have been set since the browser started. Given this weird behavior, we should remove the field entirely, to avoid bugs (See, for instance, https://crbug.com/624004). This will cause cookies set since last browser start to be displayed under the domain that they're set on, rather than the URL that actually set them, but this is already the case for cookies loaded from file, so should not be a major issue. BUG=620770 ========== to ========== Remove use of CanonicalCookie::Source() from browsing_data. Source() is only non-NULL on cookies that have been set since the browser started. Given this weird behavior, we should remove the field entirely, to avoid bugs (See, for instance, https://crbug.com/624004). The main change is that CookieTreeHostNode will now group cookies by the domain they're set on instead of the scheme+domain+port that set them. This was already the behavior for cookies that were loaded from the cookie store, rather than being set since the last browser start, so should not make much difference (Also worth nothing scheme was effectively always http before, since https was replaced with http, setting cookies on file URLs is not allowed, and extension cookies use a different cookie store that bypasses this code). BUG=620770 ==========
mmenke@chromium.org changed reviewers: + mkwst@chromium.org
I don't think this breaks anything too badly. I'm going to follow up with a CL that does the rest, this is just the only part that has a potentially controversial change, so thought I'd send it out alone.
+msramek, just in case, as I'm going to be OOO shortly. https://codereview.chromium.org/2110483002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/cookies_tree_model.cc (left): https://codereview.chromium.org/2110483002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/cookies_tree_model.cc:1223: if (source.is_empty() || !group_by_cookie_source_) { I think this is the only spot where `group_by_cookie_source_` is used. If we stop using it here, we can stop using it everywhere. The only place it seems to be set to true is in the CookiesTreeModel created in `LocalSharedObjectsContainer::CreateCookiesTreeModel`, but I don't think it has any substantial effect on things (especially given the `source()` weirdness you note. Would you mind removing it in this patch? I hope it doesn't explode the size; it looks pretty straightforward. https://codereview.chromium.org/2110483002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/cookies_tree_model.cc:1232: if (!source.SchemeIsHTTPOrHTTPS()) 1. Oh how I wish we didn't need to support WebView's crazy use of cookies on `file:` URLs. :( 2. This patch is going to lump all `file:` cookies together rather than skipping them. It would be lovely if we had any tests at all that verified that behavior. But since we don't, it must not be important, right? https://codereview.chromium.org/2110483002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/cookies_tree_model.cc (right): https://codereview.chromium.org/2110483002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/cookies_tree_model.cc:1211: // Secure cookies are treated just the same as normal ones. Since you're touching it anyway, how about "// Cookies ignore schemes, so group all HTTPS and HTTP cookies together."?
I'll respond to your other comments later today. https://codereview.chromium.org/2110483002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/cookies_tree_model.cc (left): https://codereview.chromium.org/2110483002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/cookies_tree_model.cc:1232: if (!source.SchemeIsHTTPOrHTTPS()) On 2016/06/29 05:58:42, Mike West (OOO through 4th) wrote: > 1. Oh how I wish we didn't need to support WebView's crazy use of cookies on > `file:` URLs. :( > > 2. This patch is going to lump all `file:` cookies together rather than skipping > them. It would be lovely if we had any tests at all that verified that behavior. > But since we don't, it must not be important, right? We're *already* doing that, for cookies loaded from disk. The persistent cookie store doesn't save or load source GURLs. So trying to improve file cookie behavior at this layer seems a futile endeavor. See https://cs.chromium.org/chromium/src/net/extras/sqlite/sqlite_persistent_cook... The cookie store doesn't seem to separate out file cookies in any way (Which is why it has the scary warning out it). Is this code even used by WebView? I assumed since it was in Chrome, it was not.
https://codereview.chromium.org/2110483002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/cookies_tree_model.cc (left): https://codereview.chromium.org/2110483002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/cookies_tree_model.cc:1223: if (source.is_empty() || !group_by_cookie_source_) { On 2016/06/29 05:58:42, Mike West (OOO through 4th) wrote: > I think this is the only spot where `group_by_cookie_source_` is used. If we > stop using it here, we can stop using it everywhere. The only place it seems to > be set to true is in the CookiesTreeModel created in > `LocalSharedObjectsContainer::CreateCookiesTreeModel`, but I don't think it has > any substantial effect on things (especially given the `source()` weirdness you > note. > > Would you mind removing it in this patch? I hope it doesn't explode the size; it > looks pretty straightforward. Done. https://codereview.chromium.org/2110483002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/cookies_tree_model.cc (right): https://codereview.chromium.org/2110483002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/cookies_tree_model.cc:1211: // Secure cookies are treated just the same as normal ones. On 2016/06/29 05:58:42, Mike West (OOO through 4th) wrote: > Since you're touching it anyway, how about "// Cookies ignore schemes, so group > all HTTPS and HTTP cookies together."? Done.
mmenke@chromium.org changed reviewers: + bauerb@chromium.org, dbeam@chromium.org
[+bauerb]: Please review chrome/browser/content_settings/ and chrome/browser/android/preferences/ [+dbeam]: Please review chrome/browser/ui/webui/options/ Note that there's no rush on either of these.
lgtm but did you run either the new or old settings UI to see if this affects content/site settings?
On 2016/06/30 22:30:05, Dan Beam wrote: > lgtm but did you run either the new or old settings UI to see if this affects > content/site settings? I haven't, but it does. Before, if cookies were set since the last chrome start, they will be grouped by the server that set them (And if set by file URLs (WebView only, though I don't think WebView uses this code?), they would include the "file" scheme as well. If they were loaded from the cookie store on start, they were grouped solely by the domain they were set on. Now, they're always grouped by the domain they were set on, regardless of whether they were loaded from the cookie store or set since last restart. That inconsistency is the main reason for this CL. No realizing loaded cookies have no source url has led to at least one bug as well.
On 2016/06/30 23:33:44, mmenke wrote: > On 2016/06/30 22:30:05, Dan Beam wrote: > > lgtm but did you run either the new or old settings UI to see if this affects > > content/site settings? > > I haven't, but it does. Before, if cookies were set since the last chrome > start, they will be grouped by the server that set them (And if set by file URLs > (WebView only, though I don't think WebView uses this code?), they would include > the "file" scheme as well. If they were loaded from the cookie store on start, > they were grouped solely by the domain they were set on. > > Now, they're always grouped by the domain they were set on, regardless of > whether they were loaded from the cookie store or set since last restart. That > inconsistency is the main reason for this CL. No realizing loaded cookies have > no source url has led to at least one bug as well. Well, actually, I have going to the the cookies used by this page window, and verified things looked fine. I just haven't exhaustively compared it with and without this CL.
LGTM, thanks! https://codereview.chromium.org/2110483002/diff/160001/chrome/browser/browsin... File chrome/browser/browsing_data/cookies_tree_model.cc (right): https://codereview.chromium.org/2110483002/diff/160001/chrome/browser/browsin... chrome/browser/browsing_data/cookies_tree_model.cc:1210: GURL source = GURL(std::string(url::kHttpScheme) + Nit: You could directly initialize |source| here.
LGTM. Thanks for taking another pass!
https://codereview.chromium.org/2110483002/diff/160001/chrome/browser/browsin... File chrome/browser/browsing_data/cookies_tree_model.cc (right): https://codereview.chromium.org/2110483002/diff/160001/chrome/browser/browsin... chrome/browser/browsing_data/cookies_tree_model.cc:1210: GURL source = GURL(std::string(url::kHttpScheme) + On 2016/07/01 13:30:15, Bernhard Bauer wrote: > Nit: You could directly initialize |source| here. Done.
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mkwst@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2110483002/#ps180001 (title: "Fix GURL init")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mkwst@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2110483002/#ps200001 (title: "Merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mkwst@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2110483002/#ps220001 (title: "Fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove use of CanonicalCookie::Source() from browsing_data. Source() is only non-NULL on cookies that have been set since the browser started. Given this weird behavior, we should remove the field entirely, to avoid bugs (See, for instance, https://crbug.com/624004). The main change is that CookieTreeHostNode will now group cookies by the domain they're set on instead of the scheme+domain+port that set them. This was already the behavior for cookies that were loaded from the cookie store, rather than being set since the last browser start, so should not make much difference (Also worth nothing scheme was effectively always http before, since https was replaced with http, setting cookies on file URLs is not allowed, and extension cookies use a different cookie store that bypasses this code). BUG=620770 ========== to ========== Remove use of CanonicalCookie::Source() from browsing_data. Source() is only non-NULL on cookies that have been set since the browser started. Given this weird behavior, we should remove the field entirely, to avoid bugs (See, for instance, https://crbug.com/624004). The main change is that CookieTreeHostNode will now group cookies by the domain they're set on instead of the scheme+domain+port that set them. This was already the behavior for cookies that were loaded from the cookie store, rather than being set since the last browser start, so should not make much difference (Also worth nothing scheme was effectively always http before, since https was replaced with http, setting cookies on file URLs is not allowed, and extension cookies use a different cookie store that bypasses this code). BUG=620770 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Remove use of CanonicalCookie::Source() from browsing_data. Source() is only non-NULL on cookies that have been set since the browser started. Given this weird behavior, we should remove the field entirely, to avoid bugs (See, for instance, https://crbug.com/624004). The main change is that CookieTreeHostNode will now group cookies by the domain they're set on instead of the scheme+domain+port that set them. This was already the behavior for cookies that were loaded from the cookie store, rather than being set since the last browser start, so should not make much difference (Also worth nothing scheme was effectively always http before, since https was replaced with http, setting cookies on file URLs is not allowed, and extension cookies use a different cookie store that bypasses this code). BUG=620770 ========== to ========== Remove use of CanonicalCookie::Source() from browsing_data. Source() is only non-NULL on cookies that have been set since the browser started. Given this weird behavior, we should remove the field entirely, to avoid bugs (See, for instance, https://crbug.com/624004). The main change is that CookieTreeHostNode will now group cookies by the domain they're set on instead of the scheme+domain+port that set them. This was already the behavior for cookies that were loaded from the cookie store, rather than being set since the last browser start, so should not make much difference (Also worth nothing scheme was effectively always http before, since https was replaced with http, setting cookies on file URLs is not allowed, and extension cookies use a different cookie store that bypasses this code). BUG=620770 Committed: https://crrev.com/a1663cdfc27bb767da47200b6093fbccd57ce0b1 Cr-Commit-Position: refs/heads/master@{#404932} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/a1663cdfc27bb767da47200b6093fbccd57ce0b1 Cr-Commit-Position: refs/heads/master@{#404932} |
