Issue 13357004: Clear browsing data clears data for type kStorageTypeTemporary but not for kStorageTypeSyncable.

Issue 13357004: Clear browsing data clears data for type kStorageTypeTemporary but not for kStorageTypeSyncable. (Closed)

Created:
7 years, 8 months ago by Ramya

Modified:
7 years, 8 months ago

Reviewers:
michaeln, Jói, michaelbai, jam, Evan Stade, kinuko

CC:
chromium-reviews, joi+watch-content_chromium.org, jam, markusheintz_, darin-cc_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Clear browsing data clears data for type kStorageTypeTemporary but not for kStorageTypeSyncable. This bug is visible in Android for mail.google.com Even after Clearing all the data, the syncable data was left behind. This CL should fix this bug. BUG=180249 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193738

Patch Set 1 #

Patch Set 2 : Line lengths #

Total comments: 14

Patch Set 3 : Changes based on comments #

Total comments: 10

Patch Set 4 : Some updates based on comments #

Patch Set 5 : Revertuing a file #

Patch Set 6 : Updates after CL review #

Total comments: 10

Patch Set 7 : Updates based on recent comments #

Total comments: 2

Patch Set 8 : Updates from comments #

Total comments: 2

Patch Set 9 : Fixed indent #

Patch Set 10 : Rebase #

Patch Set 11 : Rebase again #

Created: 7 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+118 lines, -36 lines)			Patch
M	chrome/browser/browsing_data/browsing_data_file_system_helper.h	View	1 2 3 4 5	2 chunks	+7 lines, -1 line	0 comments	Download
M	chrome/browser/browsing_data/browsing_data_file_system_helper.cc	View	1 2 3 4 5 6	5 chunks	+20 lines, -5 lines	0 comments	Download
M	chrome/browser/browsing_data/browsing_data_quota_helper.h	View		2 chunks	+3 lines, -1 line	0 comments	Download
M	chrome/browser/browsing_data/browsing_data_quota_helper.cc	View		3 chunks	+12 lines, -5 lines	0 comments	Download
M	chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc	View	1 2 3 4 5 6	3 chunks	+15 lines, -3 lines	0 comments	Download
M	chrome/browser/browsing_data/browsing_data_quota_helper_unittest.cc	View	1 2	4 chunks	+6 lines, -4 lines	0 comments	Download
M	chrome/browser/browsing_data/browsing_data_remover.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+8 lines, -0 lines	0 comments	Download
M	chrome/browser/browsing_data/mock_browsing_data_file_system_helper.h	View	1 2 3 4 5	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/browsing_data/mock_browsing_data_file_system_helper.cc	View	1 2 3 4 5	1 chunk	+6 lines, -5 lines	0 comments	Download
M	chrome/browser/browsing_data/mock_browsing_data_quota_helper.h	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/browsing_data/mock_browsing_data_quota_helper.cc	View		1 chunk	+6 lines, -4 lines	0 comments	Download
M	chrome/browser/ui/webui/quota_internals_proxy.cc	View		2 chunks	+7 lines, -1 line	0 comments	Download
M	content/browser/renderer_host/quota_dispatcher_host.cc	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	content/browser/storage_partition_impl.cc	View	1 2 3 4 5 6 7 8	2 chunks	+10 lines, -0 lines	0 comments	Download
M	content/public/browser/storage_partition.h	View	1 2 3 4 5 6	1 chunk	+3 lines, -0 lines	0 comments	Download
M	webkit/appcache/appcache_quota_client.cc	View	1 2 3 4 5	3 chunks	+3 lines, -3 lines	0 comments	Download
M	webkit/fileapi/local_file_system_test_helper.cc	View	1 2 3 4 5	1 chunk	+2 lines, -1 line	0 comments	Download
M	webkit/fileapi/sandbox_mount_point_provider.cc	View	1 2 3 4 5 6 7 8 9	2 chunks	+4 lines, -0 lines	0 comments	Download

Messages

Total messages: 32 (0 generated)

Expand Messages | Collapse Messages

michaelbai

I didn't fully understand the whole stuff, but I think you could send it out ...

7 years, 8 months ago (2013-03-29 23:28:51 UTC) #1

Ramya

PTAL for OWNERS lgtm estade@ chrome/browser/ui/webui/ michaeln@ chrome/browser/browsing_data/ content/browser/in_process_webkit/ webkit/appcache/ webkit/database/ webkit/database/ sievers@ content/browser/renderer_host/ sky@ ...

7 years, 8 months ago (2013-03-30 00:07:11 UTC) #2

michaeln

I'm going to defer to kinuko on this one. At a glance, having each storage ...

7 years, 8 months ago (2013-04-01 20:10:34 UTC) #3

kinuko

The quota plumbing for syncable storage type was not really comprehensive and thanks for working ...

7 years, 8 months ago (2013-04-02 03:23:04 UTC) #4

kinuko

(Forgot to publish comments) https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc File chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc (right): https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc#newcode101 chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc:101: quota::kStorageTypeSyncable, kStorageTypeTemporary ? https://codereview.chromium.org/13357004/diff/8001/content/browser/in_process_webkit/indexed_db_context_impl.cc File ...

7 years, 8 months ago (2013-04-02 03:23:44 UTC) #5

no sievers

On 2013/03/30 00:07:11, Ramya wrote: > PTAL for OWNERS lgtm > > sievers@ content/browser/renderer_host/ -sievers, ...

7 years, 8 months ago (2013-04-02 03:26:09 UTC) #6

Ramya

PTAL https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc File chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc (right): https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc#newcode101 chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc:101: quota::kStorageTypeSyncable, As per like 80, GetOriginsModifiedSince for temporary ...

7 years, 8 months ago (2013-04-03 00:21:38 UTC) #7

kinuko

https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc File chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc (right): https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc#newcode101 chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc:101: quota::kStorageTypeSyncable, On 2013/04/03 00:21:38, Ramya wrote: > As per ...

7 years, 8 months ago (2013-04-03 12:42:15 UTC) #8

https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc (right):

https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc:101:
quota::kStorageTypeSyncable,
On 2013/04/03 00:21:38, Ramya wrote:
> As per like 80, GetOriginsModifiedSince for temporary has already been called.
> So if the type here is temporary, then we need to call GetOriginsModifiedSince
> for syncable and after that for persistent (similar to logic that existed
> previously).

Ah ok, I must have looked carefully. Can we comment to make it clear that we're
calling GetOriginsModifiedSince for all types by chaining callbacks?

Also could we make the order of calling GetOriginsModifiedSince as well as that
of type enum (i.e. temporary, persistent, syncable):

if (type == Temporary) {
  GetOriginsModifiedSince(Persistent, ...);
} else if (type == persistent) {
  GetOriginsModifiedSince(Syncable, ...);
} else {
  DCHECK(type == Syncable);
}

https://codereview.chromium.org/13357004/diff/8001/webkit/appcache/appcache_s...
File webkit/appcache/appcache_storage.cc (right):

https://codereview.chromium.org/13357004/diff/8001/webkit/appcache/appcache_s...
webkit/appcache/appcache_storage.cc:113: origin, quota::kStorageTypeSyncable,
On 2013/04/03 00:21:38, Ramya wrote:
> I am not too familiar with how appcache works, but while trying to fix the bug
I
> noticed that adding NotifyStorageModified for syncable data type fixed it.
(The
> bug is that after logging into http://gmail.com and clearing browsing data,
out of
> 3.5MB data 1.5MB is always left behind). Adding NotifyStorageModified to this
> file fixed the issue. Maybe kinuko can help me understand what is going on
here.

Hmm actually I don't get it either... I guess this does nothing to do with
AppCache but maybe adding similar line in any storage API instead of here might
possibly fix the problem?

Is it possible for you to see what's left behind under "File System" profile
directory without these lines?  Thanks so much.

Ramya

On 2013/04/03 12:42:15, kinuko wrote: > https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc > File chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc (right): > > https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc#newcode101 > ...

7 years, 8 months ago (2013-04-03 22:16:15 UTC) #10

On 2013/04/03 12:42:15, kinuko wrote:
>
https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_da...
> File chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc (right):
> 
>
https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_da...
> chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc:101:
> quota::kStorageTypeSyncable,
> On 2013/04/03 00:21:38, Ramya wrote:
> > As per like 80, GetOriginsModifiedSince for temporary has already been
called.
> > So if the type here is temporary, then we need to call
GetOriginsModifiedSince
> > for syncable and after that for persistent (similar to logic that existed
> > previously).
> 
> Ah ok, I must have looked carefully. Can we comment to make it clear that
we're
> calling GetOriginsModifiedSince for all types by chaining callbacks?
> 
> Also could we make the order of calling GetOriginsModifiedSince as well as
that
> of type enum (i.e. temporary, persistent, syncable):
> 
> if (type == Temporary) {
>   GetOriginsModifiedSince(Persistent, ...);
> } else if (type == persistent) {
>   GetOriginsModifiedSince(Syncable, ...);
> } else {
>   DCHECK(type == Syncable);
> }
> 
>
https://codereview.chromium.org/13357004/diff/8001/webkit/appcache/appcache_s...
> File webkit/appcache/appcache_storage.cc (right):
> 
>
https://codereview.chromium.org/13357004/diff/8001/webkit/appcache/appcache_s...
> webkit/appcache/appcache_storage.cc:113: origin, quota::kStorageTypeSyncable,
> On 2013/04/03 00:21:38, Ramya wrote:
> > I am not too familiar with how appcache works, but while trying to fix the
bug
> I
> > noticed that adding NotifyStorageModified for syncable data type fixed it.
> (The
> > bug is that after logging into http://gmail.com and clearing browsing data,
> out of
> > 3.5MB data 1.5MB is always left behind). Adding NotifyStorageModified to
this
> > file fixed the issue. Maybe kinuko can help me understand what is going on
> here.
> 
> Hmm actually I don't get it either... I guess this does nothing to do with
> AppCache but maybe adding similar line in any storage API instead of here
might
> possibly fix the problem?
> 
> Is it possible for you to see what's left behind under "File System" profile
> directory without these lines?  Thanks so much.

As you mentioned, changing the client from AppCache to FileSystem also did the
trick. Unfortunately, I am not what is causing the need to add those lines in
appcache_storage.cc. If I remove them, I am back with the same old bug. Can you
explain what you mean by "see what's left behind under "File System" profile"? I
am sure I understand what I need to check. I did add the missing cases for
kFileSystemTypeSyncable, but not sure if it is useful at all.

kinuko

On 2013/04/03 22:16:15, Ramya wrote: > On 2013/04/03 12:42:15, kinuko wrote: > > > https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc ...

7 years, 8 months ago (2013-04-04 05:12:13 UTC) #11

On 2013/04/03 22:16:15, Ramya wrote:
> On 2013/04/03 12:42:15, kinuko wrote:
> >
>
https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_da...
> > File chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc
(right):
> > 
> >
>
https://codereview.chromium.org/13357004/diff/8001/chrome/browser/browsing_da...
> > chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc:101:
> > quota::kStorageTypeSyncable,
> > On 2013/04/03 00:21:38, Ramya wrote:
> > > As per like 80, GetOriginsModifiedSince for temporary has already been
> called.
> > > So if the type here is temporary, then we need to call
> GetOriginsModifiedSince
> > > for syncable and after that for persistent (similar to logic that existed
> > > previously).
> > 
> > Ah ok, I must have looked carefully. Can we comment to make it clear that
> we're
> > calling GetOriginsModifiedSince for all types by chaining callbacks?
> > 
> > Also could we make the order of calling GetOriginsModifiedSince as well as
> that
> > of type enum (i.e. temporary, persistent, syncable):
> > 
> > if (type == Temporary) {
> >   GetOriginsModifiedSince(Persistent, ...);
> > } else if (type == persistent) {
> >   GetOriginsModifiedSince(Syncable, ...);
> > } else {
> >   DCHECK(type == Syncable);
> > }
> > 
> >
>
https://codereview.chromium.org/13357004/diff/8001/webkit/appcache/appcache_s...
> > File webkit/appcache/appcache_storage.cc (right):
> > 
> >
>
https://codereview.chromium.org/13357004/diff/8001/webkit/appcache/appcache_s...
> > webkit/appcache/appcache_storage.cc:113: origin,
quota::kStorageTypeSyncable,
> > On 2013/04/03 00:21:38, Ramya wrote:
> > > I am not too familiar with how appcache works, but while trying to fix the
> bug
> > I
> > > noticed that adding NotifyStorageModified for syncable data type fixed it.
> > (The
> > > bug is that after logging into http://gmail.com and clearing browsing
data,
> > out of
> > > 3.5MB data 1.5MB is always left behind). Adding NotifyStorageModified to
> this
> > > file fixed the issue. Maybe kinuko can help me understand what is going on
> > here.
> > 
> > Hmm actually I don't get it either... I guess this does nothing to do with
> > AppCache but maybe adding similar line in any storage API instead of here
> might
> > possibly fix the problem?
> > 
> > Is it possible for you to see what's left behind under "File System" profile
> > directory without these lines?  Thanks so much.
> 
> As you mentioned, changing the client from AppCache to FileSystem also did the
> trick. Unfortunately, I am not what is causing the need to add those lines in
> appcache_storage.cc. If I remove them, I am back with the same old bug. Can
you
> explain what you mean by "see what's left behind under "File System" profile"?
I
> am sure I understand what I need to check. I did add the missing cases for
> kFileSystemTypeSyncable, but not sure if it is useful at all.

Looking into the issue page (http://crbug.com/180249) it seems to tell this
doesn't happen with cold start-- is it true?  If it's true it might mean we'd
made some bug in previous version and left some data for 'syncable' type which
cannot be deleted by the current ClearBrowsingData code?  I tested the repro
steps on my Galaxy Nexus (running Chrome 25.0.1364.169) but I cannot repro this.

On the phone where the bug is happening is it possible to look into what data is
actually remaining on the disk?? (E.g. On desktop chrome the data is usually
stored in \Users\username\AppData\Local\Google\Chrome\User Data\Default\. Does
Chrome Android do the same?)

michaeln

I think i see what might be one source of syncable confusion? In appcache_quota_client.cc there ...

7 years, 8 months ago (2013-04-05 00:54:14 UTC) #13

michaeln

> It's possible that other quota clients were coded in this way too assuming the ...

7 years, 8 months ago (2013-04-05 01:15:28 UTC) #14

kinuko

On 2013/04/05 00:54:14, michaeln wrote: > I think i see what might be one source ...

7 years, 8 months ago (2013-04-05 02:30:45 UTC) #15

Ramya

Thanks michaeln@ and kinuko@ for all the feedback. I made changes to appcache_quota_client and the ...

7 years, 8 months ago (2013-04-05 19:21:12 UTC) #16

michaeln

https://codereview.chromium.org/13357004/diff/36001/chrome/browser/browsing_data/browsing_data_file_system_helper.cc File chrome/browser/browsing_data/browsing_data_file_system_helper.cc (right): https://codereview.chromium.org/13357004/diff/36001/chrome/browser/browsing_data/browsing_data_file_system_helper.cc#newcode143 chrome/browser/browsing_data/browsing_data_file_system_helper.cc:143: fileapi::kFileSystemTypeSyncable); nit: indent these params like the others above

7 years, 8 months ago (2013-04-05 20:04:10 UTC) #19

kinuko

Glad to hear that the issue has been fixed, thanks for working on this!! https://codereview.chromium.org/13357004/diff/36001/content/browser/fileapi/fileapi_message_filter.cc ...

7 years, 8 months ago (2013-04-06 04:36:19 UTC) #20

Ramya

Thanks for all the feedback. Comments addressed. jam@ content/ OWNERS lgtm kinuko@ - overall lgtm ...

7 years, 8 months ago (2013-04-08 22:24:56 UTC) #21

kinuko

lgtm https://codereview.chromium.org/13357004/diff/47001/webkit/quota/quota_manager.cc File webkit/quota/quota_manager.cc (right): https://codereview.chromium.org/13357004/diff/47001/webkit/quota/quota_manager.cc#newcode1283 webkit/quota/quota_manager.cc:1283: if ((type == kStorageTypeTemporary || type == kStorageTypeSyncable) ...

7 years, 8 months ago (2013-04-09 13:37:51 UTC) #22

Ramya

jam@ Can you PTAL for content/ OWNERS lgtm https://codereview.chromium.org/13357004/diff/47001/webkit/quota/quota_manager.cc File webkit/quota/quota_manager.cc (right): https://codereview.chromium.org/13357004/diff/47001/webkit/quota/quota_manager.cc#newcode1283 webkit/quota/quota_manager.cc:1283: if ...

7 years, 8 months ago (2013-04-09 17:30:14 UTC) #23

jam

content lgtm https://codereview.chromium.org/13357004/diff/63001/content/browser/storage_partition_impl.cc File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/13357004/diff/63001/content/browser/storage_partition_impl.cc#newcode113 content/browser/storage_partition_impl.cc:113: quota::kStorageTypeSyncable, base::Time(), nit: indent again

7 years, 8 months ago (2013-04-09 23:15:35 UTC) #24

Ramya

https://codereview.chromium.org/13357004/diff/63001/content/browser/storage_partition_impl.cc File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/13357004/diff/63001/content/browser/storage_partition_impl.cc#newcode113 content/browser/storage_partition_impl.cc:113: quota::kStorageTypeSyncable, base::Time(), On 2013/04/09 23:15:35, jam wrote: > nit: ...

7 years, 8 months ago (2013-04-10 16:58:29 UTC) #25