Delete cookies for site when deleting locally stored data.

Delete cookies for site when deleting locally stored data.

And, in the process, fix a bug in the CookieTreeModel, where it was prematurely sending BatchEnded events.

BUG=445962, 447395
Committed: https://crrev.com/bf8a48a2dfb9c0d16103ae8228798f5cfdaf343f
Cr-Commit-Position: refs/heads/master@{#313902}

[Finnur, 2015-01-19 14:50:55]

finnur@chromium.org changed reviewers:
+ mkwst@chromium.org, newt@chromium.org

[Finnur, 2015-01-19 14:51:48]

Newt, can you please review the first three files?
Mike, can you please review the last four files?

[Mike West, 2015-01-20 07:55:19]

LGTM, thanks for the fix. :)

[newt (away), 2015-01-20 18:35:10]

https://codereview.chromium.org/863503002/diff/20001/chrome/android/java/stri...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/863503002/diff/20001/chrome/android/java/stri...
chrome/android/java/strings/android_chrome_strings.grd:331: Are you sure you
want to clear all data for this website (including cookies) and reset all its
permissions?
I believe the bug called for commas instead of parentheses:

    ..., including cookies, ...

https://codereview.chromium.org/863503002/diff/20001/chrome/browser/android/p...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/863503002/diff/20001/chrome/browser/android/p...
chrome/browser/android/preferences/website_preference_bridge.cc:290: class
SiteDataDeleteHelper :
How does desktop delete cookies and clear site data? Seems like we should be
sharing that code, rather than reinventing this rather complicated machinery.

[Finnur, 2015-01-21 14:53:06]

https://codereview.chromium.org/863503002/diff/20001/chrome/android/java/stri...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/863503002/diff/20001/chrome/android/java/stri...
chrome/android/java/strings/android_chrome_strings.grd:331: Are you sure you
want to clear all data for this website (including cookies) and reset all its
permissions?
On 2015/01/20 18:35:10, newt wrote:
> I believe the bug called for commas instead of parentheses:
> 
>     ..., including cookies, ...

Done.

https://codereview.chromium.org/863503002/diff/20001/chrome/browser/android/p...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/863503002/diff/20001/chrome/browser/android/p...
chrome/browser/android/preferences/website_preference_bridge.cc:290: class
SiteDataDeleteHelper :
> How does desktop delete cookies and clear site data? Seems like we should be
> sharing that code, rather than reinventing this rather complicated machinery.

Yes, I had the same thought initially. "There's got to be a way to just say: Go
off (asynchronously) and delete all cookies for this one URL". Right?

There's a couple of things I found out, though.

1) Desktop is built around the idea of setting up a tree in-memory and keeping
it alive, while showing the user a list-representation of it and accept requests
to delete by node-id. In other words, Desktop has no need to look for data from
one site by url and delete it. What Desktop does is iterate multiple times over
the tree, once to send down the data and once each time the user deletes
(looking for the id of the node). We only need one pass (already know what we
want to delete).

2) The LocalDataContainer uses helper objects to gather the data and not all
helper objects are applicable across Android and Desktop. For example, Desktop
uses BrowsingDataFlashLSOHelper, which isn't needed on Android. My point is
this: Even the construction of the tree isn't going to apply cleanly across.

But, the machinery (as you call it) :) -- while being a little wordy, looks more
complex than it actually is. We're simply creating a LocalDataContainer object
along with some helper objects (that will do the actual fetch work) and simply
wait for them to come back with the dataset. Much of the complexity is just
boilerplate to keep the delete request alive while we wait for the workers to
finish and notify us. Which is code that we'd need anyway because we're
performing an async operation from a static function - right?

So... I'm not sure what makes sense to share/simplify here, but if you have
ideas I'm open to suggestions.

[Finnur, 2015-01-22 10:44:10]

Newton: WDYT?

[newt (away), 2015-01-23 02:23:15]

Could you get an OWNER of chrome/browser/browsing_data to look at
website_preference_bridge.cc? The JNI bits look fine, but I'm certainly not
qualified to review to the Cookie deletion code.

https://codereview.chromium.org/863503002/diff/40001/chrome/browser/android/p...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/863503002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/website_preference_bridge.cc:345: void
TreeModelEndBatch(CookiesTreeModel* model) override {
It's not clear to me that TreeModelEndBatch is guaranteed to be called exactly
once, but that's mainly because I don't know anything about CookiesTreeModel.

https://codereview.chromium.org/863503002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/website_preference_bridge.cc:363: for (int i
= node->child_count(); i > 0; --i)
Why can't you move this for loop up to before the first if statement?

https://codereview.chromium.org/863503002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/website_preference_bridge.cc:364:
RecursivelyFindSiteAndDelete(node->GetChild(i - 1));
Can COOKIE nodes contain other COOKIE nodes?

[Finnur, 2015-01-23 16:02:57]

Mike, can you look at website_preference_bridge.cc as well?

Newton: Good points, I'll address your review comments in an upcoming CL
update (you don't need to look again at the moment).


On Fri, Jan 23, 2015 at 2:23 AM, <newt@chromium.org> wrote:

> Could you get an OWNER of chrome/browser/browsing_data to look at
> website_preference_bridge.cc? The JNI bits look fine, but I'm certainly not
> qualified to review to the Cookie deletion code.
>
>
> https://codereview.chromium.org/863503002/diff/40001/
> chrome/browser/android/preferences/website_preference_bridge.cc
> File chrome/browser/android/preferences/website_preference_bridge.cc
> (right):
>
> https://codereview.chromium.org/863503002/diff/40001/
> chrome/browser/android/preferences/website_preference_bridge.cc#newcode345
> chrome/browser/android/preferences/website_preference_bridge.cc:345:
> void TreeModelEndBatch(CookiesTreeModel* model) override {
> It's not clear to me that TreeModelEndBatch is guaranteed to be called
> exactly once, but that's mainly because I don't know anything about
> CookiesTreeModel.
>
> https://codereview.chromium.org/863503002/diff/40001/
> chrome/browser/android/preferences/website_preference_bridge.cc#newcode363
> chrome/browser/android/preferences/website_preference_bridge.cc:363: for
> (int i = node->child_count(); i > 0; --i)
> Why can't you move this for loop up to before the first if statement?
>
> https://codereview.chromium.org/863503002/diff/40001/
> chrome/browser/android/preferences/website_preference_bridge.cc#newcode364
> chrome/browser/android/preferences/website_preference_bridge.cc:364:
> RecursivelyFindSiteAndDelete(node->GetChild(i - 1));
> Can COOKIE nodes contain other COOKIE nodes?
>
> https://codereview.chromium.org/863503002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Finnur, 2015-01-26 10:47:03]

Mike: ping
Newton: Code updated, PTAL.

[Finnur, 2015-01-26 10:47:22]

Ah... and now with comments...

https://codereview.chromium.org/863503002/diff/40001/chrome/browser/android/p...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/863503002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/website_preference_bridge.cc:345: void
TreeModelEndBatch(CookiesTreeModel* model) override {
Incidentally, I've become intimately familiar with that logic, since it was
designed to send only one event (with DCHECKs in the Desktop UI) but due to a
bug (that I'm also fixing in this CL and Mike reviewed), it wasn't working (was
sending the event twice).

https://codereview.chromium.org/863503002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/website_preference_bridge.cc:363: for (int i
= node->child_count(); i > 0; --i)
On 2015/01/23 02:23:14, newt wrote:
> Why can't you move this for loop up to before the first if statement?

At one point I needed to delete last, but then managed to simplify the code and
get rid of that requirement. Never revisited this, but now I have. Thanks! This
simplifies things a bit.

https://codereview.chromium.org/863503002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/website_preference_bridge.cc:364:
RecursivelyFindSiteAndDelete(node->GetChild(i - 1));
I don't believe so. What I see is a tree that looks like this:

Root
  - Host
    - Cookies
      - Cookie A
      - Cookie B
      - Cookie C
  - Host
    - Cookies
      - Cookie D
      - Cookie E
      - Cookie F
  - Host
    - ...

There always seems to be a COOKIES (plural) node, containing leaf nodes of type
COOKIE (singular). This is pretty typical for all the data, AppCache has
AppCaches, LocalStorage has LocalStorages, etc.

[Mike West, 2015-01-26 13:05:55]

website_preference_bridge LGTM.

[newt (away), 2015-01-26 19:24:22]

https://codereview.chromium.org/863503002/diff/80001/chrome/browser/android/p...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/863503002/diff/80001/chrome/browser/android/p...
chrome/browser/android/preferences/website_preference_bridge.cc:362:
domain_.DomainIs(info.cookie->Domain().c_str()))
Is this the correct way to check for cookies that should be deleted?

GURL("http://maps.google.com").DomainIs("google.com") will return true, but you
don't want to delete the cookies for maps.google.com when deleting cookies for
google.com, I believe.

Also, should we delete HTTPS cookies when the user requests to delete cookies
for an HTTP origin?

[Mike West, 2015-01-28 09:39:54]

On 2015/01/26 19:24:22, newt wrote:
>
https://codereview.chromium.org/863503002/diff/80001/chrome/browser/android/p...
> File chrome/browser/android/preferences/website_preference_bridge.cc (right):
> 
>
https://codereview.chromium.org/863503002/diff/80001/chrome/browser/android/p...
> chrome/browser/android/preferences/website_preference_bridge.cc:362:
> domain_.DomainIs(info.cookie->Domain().c_str()))
> Is this the correct way to check for cookies that should be deleted?
> 
> GURL("http://maps.google.com").DomainIs("google.com") will return true, but
you
> don't want to delete the cookies for http://maps.google.com when deleting
cookies for
> http://google.com, I believe.
>
> Also, should we delete HTTPS cookies when the user requests to delete cookies
> for an HTTP origin?

Cookies don't map to the same-origin model: they ignore both scheme and port,
and are purely domain-bound. Sometimes they're marked as "secure", but that
doesn't change the cookie's ownership, it just means that they're only sent when
the scheme is HTTPS.

Cookies from `maps.google.com` can also be set for `google.com`, and cookies
from `google.com` are sent when you visit `maps.google.com`. eTLD+1 is about as
good as we can do here. Because cookies are a terrible storage mechanism in
practically every way. :)

[Finnur, 2015-01-28 11:33:48]

https://codereview.chromium.org/863503002/diff/80001/chrome/browser/android/p...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/863503002/diff/80001/chrome/browser/android/p...
chrome/browser/android/preferences/website_preference_bridge.cc:362:
domain_.DomainIs(info.cookie->Domain().c_str()))
Thanks Mike.

I convinced myself that the review I sent out was correct, but started doubting
myself upon reading Newton's reply. But now I'm coming back to thinking it was
correct after all. I'm not 100% sure, though, so I'll explain my reasoning and
we can discuss.

As Mike points out, the cookie model and they way we present sites to the user
are not consistent. We might show two sites for www.google.is (one with https
and one with http) in the sites list, but under the hood the cookies that get
created when I access https://www.google.is (note the https) look something akin
to this:

A) HOST  Domain: www.google.is 
B) DOMAIN  Domain: .google.is 
C) DOMAIN  Domain: .google.is (Accessible to javascript only)

None of those cookies have the secure_ flag set to true (a flag that means
cookies are only sent over https). 
Side question: Mike, does secure_ == true mean that the cookie is not visible on
the site when accessed over http?

Anyway, regardless of that, two things are clear to me:
1) We have no good way of deleting only http or only https cookies (as the three
cookies above show). This is what Mike touched upon, they are purely domain
bound.
2) We'll have a variety of cookies that can affect a site, some specifying exact
hosts (cookie A above), some casting a wider net (the other two), with no good
way of knowing how they were created or how many sites within the domain use it.
So (in my mind) the only thing the implementation can do is ask the question:
Does this cookie affect the site being deleted? If so, delete it. And that where
the DomainIs call comes in.

That means we're likely to delete a cookie that is shared by other sites on the
domain, so we can satisfy the requirement to delete the cookies for the site
being deleted. To be specific -- and using Newtons example that if you ask to
delete all data related to maps.google.com, you have to delete the cookie that
applies to the .google.com domain.

Right?

[newt (away), 2015-01-28 17:43:59]

lgtm. Thanks!

https://codereview.chromium.org/863503002/diff/80001/chrome/browser/android/p...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/863503002/diff/80001/chrome/browser/android/p...
chrome/browser/android/preferences/website_preference_bridge.cc:362:
domain_.DomainIs(info.cookie->Domain().c_str()))
On 2015/01/28 11:33:48, Finnur wrote:
> Thanks Mike.
> 
> I convinced myself that the review I sent out was correct, but started
doubting
> myself upon reading Newton's reply. But now I'm coming back to thinking it was
> correct after all. I'm not 100% sure, though, so I'll explain my reasoning and
> we can discuss.
> 
> As Mike points out, the cookie model and they way we present sites to the user
> are not consistent. We might show two sites for http://www.google.is (one with
https
> and one with http) in the sites list, but under the hood the cookies that get
> created when I access https://www.google.is (note the https) look something
akin
> to this:
> 
> A) HOST  Domain: http://www.google.is 
> B) DOMAIN  Domain: .google.is 
> C) DOMAIN  Domain: .google.is (Accessible to javascript only)
> 
> None of those cookies have the secure_ flag set to true (a flag that means
> cookies are only sent over https). 
> Side question: Mike, does secure_ == true mean that the cookie is not visible
on
> the site when accessed over http?
> 
> Anyway, regardless of that, two things are clear to me:
> 1) We have no good way of deleting only http or only https cookies (as the
three
> cookies above show). This is what Mike touched upon, they are purely domain
> bound.
> 2) We'll have a variety of cookies that can affect a site, some specifying
exact
> hosts (cookie A above), some casting a wider net (the other two), with no good
> way of knowing how they were created or how many sites within the domain use
it.
> So (in my mind) the only thing the implementation can do is ask the question:
> Does this cookie affect the site being deleted? If so, delete it. And that
where
> the DomainIs call comes in.
> 
> That means we're likely to delete a cookie that is shared by other sites on
the
> domain, so we can satisfy the requirement to delete the cookies for the site
> being deleted. To be specific -- and using Newtons example that if you ask to
> delete all data related to http://maps.google.com, you have to delete the
cookie that
> applies to the .google.com domain.
> 
> Right?

Ok, I'm convinced. Thanks for walking me through the reasoning :)

[markusheintz_, 2015-01-29 10:14:58]

On 2015/01/28 17:43:59, newt wrote:
> lgtm. Thanks!
> 
>
https://codereview.chromium.org/863503002/diff/80001/chrome/browser/android/p...
> File chrome/browser/android/preferences/website_preference_bridge.cc (right):
> 
>
https://codereview.chromium.org/863503002/diff/80001/chrome/browser/android/p...
> chrome/browser/android/preferences/website_preference_bridge.cc:362:
> domain_.DomainIs(info.cookie->Domain().c_str()))
> On 2015/01/28 11:33:48, Finnur wrote:
> > Thanks Mike.
> > 
> > I convinced myself that the review I sent out was correct, but started
> doubting
> > myself upon reading Newton's reply. But now I'm coming back to thinking it
was
> > correct after all. I'm not 100% sure, though, so I'll explain my reasoning
and
> > we can discuss.
> > 
> > As Mike points out, the cookie model and they way we present sites to the
user
> > are not consistent. We might show two sites for http://www.google.is (one
with
> https
> > and one with http) in the sites list, but under the hood the cookies that
get
> > created when I access https://www.google.is (note the https) look something
> akin
> > to this:
> > 
> > A) HOST  Domain: http://www.google.is 
> > B) DOMAIN  Domain: .google.is 
> > C) DOMAIN  Domain: .google.is (Accessible to javascript only)
> > 
> > None of those cookies have the secure_ flag set to true (a flag that means
> > cookies are only sent over https). 
> > Side question: Mike, does secure_ == true mean that the cookie is not
visible
> on
> > the site when accessed over http?
> > 
> > Anyway, regardless of that, two things are clear to me:
> > 1) We have no good way of deleting only http or only https cookies (as the
> three
> > cookies above show). This is what Mike touched upon, they are purely domain
> > bound.
> > 2) We'll have a variety of cookies that can affect a site, some specifying
> exact
> > hosts (cookie A above), some casting a wider net (the other two), with no
good
> > way of knowing how they were created or how many sites within the domain use
> it.
> > So (in my mind) the only thing the implementation can do is ask the
question:
> > Does this cookie affect the site being deleted? If so, delete it. And that
> where
> > the DomainIs call comes in.
> > 
> > That means we're likely to delete a cookie that is shared by other sites on
> the
> > domain, so we can satisfy the requirement to delete the cookies for the site
> > being deleted. To be specific -- and using Newtons example that if you ask
to
> > delete all data related to http://maps.google.com, you have to delete the
> cookie that
> > applies to the .google.com domain.
> > 
> > Right?
> 
> Ok, I'm convinced. Thanks for walking me through the reasoning :)

LGTM

Following up on an off cl discussion.
I agree with the conclusion here.

[Finnur, 2015-01-30 11:45:53]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2015-01-30 11:46:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/863503002/100001

[commit-bot: I haz the power, 2015-01-30 12:28:44]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-30 12:29:43]

Patchset 6 (id:??) landed as
https://crrev.com/bf8a48a2dfb9c0d16103ae8228798f5cfdaf343f
Cr-Commit-Position: refs/heads/master@{#313902}