New issue
Advanced search Search tips

Issue 796151 link

Starred by 1 user

Issue metadata

Status: Fixed
Merged: issue 793630
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Storage deletion is not visible

Project Member Reported by dullweber@chromium.org, Dec 19 2017

Issue description

Chrome Version: 64.0.3282.12
OS: Android

What steps will reproduce the problem?
(1) Visit some site, e.g. facebook.com
(2) Go to Settings/Site settings/Storage
(3) Clear Storage

What is the expected result?
All entries should be gone.

What happens instead?
The list is still there and you need to go back and reopen storage to see that it was actually deleted.

 

Comment 1 Deleted

Comment 2 Deleted

oops, I merged this incorrectly
Owner: mek@chromium.org
Status: Assigned (was: Untriaged)
The following is happening (https://crrev.com/c/833928): 

CLEAR  JNI_WebsitePreferenceBridge_ClearLocalStorageData
CLEAR  DOMStorageContextWrapper::DeleteLocalStorageForPhysicalOrigin
CLEAR  LocalStorageContextMojo::DeleteStorageForPhysicalOrigin
CLEAR  LocalStorageContextMojo::RetrieveStorageUsage (DeleteStorageForPhysicalOrigin)
FETCH  JNI_WebsitePreferenceBridge_FetchLocalStorageInfo
FETCH  BrowsingDataLocalStorageHelper::StartFetching
FETCH  DOMStorageContextWrapper::GetLocalStorageUsage
FETCH  LocalStorageContextMojo::RetrieveStorageUsage (GetStorageUsage)
CLEAR  LocalStorageContextMojo::OnGotStorageUsageForDeletePhysicalOrigin
CLEAR  LocalStorageContextMojo::DeleteStorage

DOMStorageContextWrapper::DeleteLocalStorageForPhysicalOrigin returns immediately and the UI fetches localstorage again before the deletion is finished.

mek@: You worked on the mojo changes for localstorage. Do you have an idea how this could be fixed? Could we add a callback to LocalStorageContextMojo::DeleteStorageForPhysicalOrigin to wait until the deletion has actually happened?

Comment 5 by mek@chromium.org, Dec 20 2017

Cc: dmu...@chromium.org
Yeah, we'd need to add some kind of callback to be informed when deleting localstorage (or session storage, I think other storages already do have callback) is finished.
Is someone already working on this issue? It would be great if we could get it fixed for 65, it's quite confusing at the moment. Otherwise I could take this and add the callback.

Comment 7 by mek@chromium.org, Jan 3 2018

I'm not working on it, since this has been like this since forever it didn't seem like a very high priority to me. Happy to review code if you want to take it on.
Cc: mek@chromium.org
Owner: dullweber@chromium.org
Status: Started (was: Assigned)
Ok. It's not too urgent as the deletion actually happens but we just cleaned up lots of bugs in data deletion and I would like to have this solved as well :)
Project Member

Comment 9 by bugdroid1@chromium.org, Feb 1 2018

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

commit de325b648d67f46268f9f7fdf1ea998bfd98e193
Author: Christian Dullweber <dullweber@chromium.org>
Date: Thu Feb 01 10:01:20 2018

Add callback for LocalStorage deletion

Deletions in the Storage UI on Android report as finished before
LocalStorage has actually been deleted. The UI then incorrectly shows
storage that should have been deleted.
This CL adds a callback for LocalStorage deletions and waits until the
callback is called before the UI is updated.

Bug:  796151 
Change-Id: I936a0bc4cbc189702aaeffa0dd63cd1fbf0e60c7
Reviewed-on: https://chromium-review.googlesource.com/850214
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Dave Schuyler <dschuyler@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533616}
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/LocalStorageInfo.java
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferenceBridge.java
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/chrome/browser/android/preferences/website_preference_bridge.cc
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/chrome/browser/browsing_data/browsing_data_local_storage_helper.cc
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/chrome/browser/browsing_data/browsing_data_local_storage_helper.h
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/chrome/browser/browsing_data/browsing_data_local_storage_helper_browsertest.cc
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/chrome/browser/browsing_data/browsing_data_local_storage_helper_unittest.cc
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/chrome/browser/browsing_data/cookies_tree_model.cc
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/chrome/browser/browsing_data/mock_browsing_data_local_storage_helper.cc
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/chrome/browser/browsing_data/mock_browsing_data_local_storage_helper.h
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/chrome/browser/sessions/session_data_deleter.cc
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/chrome/browser/ui/webui/settings/site_settings_handler.cc
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/chrome/browser/ui/webui/settings/site_settings_handler.h
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/content/browser/dom_storage/dom_storage_browsertest.cc
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/content/browser/dom_storage/dom_storage_context_wrapper.cc
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/content/browser/dom_storage/dom_storage_context_wrapper.h
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/content/browser/dom_storage/local_storage_context_mojo.cc
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/content/browser/dom_storage/local_storage_context_mojo.h
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/content/browser/dom_storage/local_storage_context_mojo_unittest.cc
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/content/browser/storage_partition_impl.cc
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/content/public/browser/dom_storage_context.h
[modify] https://crrev.com/de325b648d67f46268f9f7fdf1ea998bfd98e193/content/public/browser/session_storage_usage_info.h

Project Member

Comment 10 by bugdroid1@chromium.org, Feb 1 2018

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

commit 9a5b79ceeccc1b5f20057354241e96c8ddeb96b7
Author: Christian Dullweber <dullweber@chromium.org>
Date: Thu Feb 01 17:06:05 2018

Let StoragePartition wait for localstorage deletions

This CL changes StoragePartition to wait for localstorage deletions
to finish before calling its callback.

Some extension tests can leak memory because they don't wait until
StorageParitition deletions are finished. This CL adds a method to
StoragePartition to check if it is done and changes extension tests
to always wait for StoragePartition.

Bug:  796151 
Change-Id: Ica6f6ef47457eb34f82923ec6ae144f6b5b67fda
Reviewed-on: https://chromium-review.googlesource.com/852234
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533707}
[modify] https://crrev.com/9a5b79ceeccc1b5f20057354241e96c8ddeb96b7/chrome/browser/extensions/extension_gcm_app_handler_unittest.cc
[modify] https://crrev.com/9a5b79ceeccc1b5f20057354241e96c8ddeb96b7/chrome/browser/extensions/extension_service_test_base.cc
[modify] https://crrev.com/9a5b79ceeccc1b5f20057354241e96c8ddeb96b7/chrome/browser/extensions/extension_service_test_base.h
[modify] https://crrev.com/9a5b79ceeccc1b5f20057354241e96c8ddeb96b7/content/browser/storage_partition_impl.cc
[modify] https://crrev.com/9a5b79ceeccc1b5f20057354241e96c8ddeb96b7/content/browser/storage_partition_impl.h
[modify] https://crrev.com/9a5b79ceeccc1b5f20057354241e96c8ddeb96b7/content/public/browser/storage_partition.h
[modify] https://crrev.com/9a5b79ceeccc1b5f20057354241e96c8ddeb96b7/content/public/test/test_storage_partition.cc
[modify] https://crrev.com/9a5b79ceeccc1b5f20057354241e96c8ddeb96b7/content/public/test/test_storage_partition.h

Project Member

Comment 11 by bugdroid1@chromium.org, Feb 1 2018

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

commit 2768203553deae4587be1b6840b4e06f02f8c870
Author: Tommy Steimel <steimel@chromium.org>
Date: Thu Feb 01 23:24:39 2018

Revert "Let StoragePartition wait for localstorage deletions"

This reverts commit 9a5b79ceeccc1b5f20057354241e96c8ddeb96b7.

Reason for revert: Causing failures on the Linux Chromium OS ASan LSan Tests bot (94% confidence from FindIt):

https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/25968

Failures:
ExtensionAppModelBuilderTest.Uninstall
ExtensionAppModelBuilderTest.UninstallTerminatedApp
DualBadgeMapTest.ArcAppToExtensionMapTest

Example error message:
==21659==ERROR: LeakSanitizer: detected memory leaks

Original change's description:
> Let StoragePartition wait for localstorage deletions
> 
> This CL changes StoragePartition to wait for localstorage deletions
> to finish before calling its callback.
> 
> Some extension tests can leak memory because they don't wait until
> StorageParitition deletions are finished. This CL adds a method to
> StoragePartition to check if it is done and changes extension tests
> to always wait for StoragePartition.
> 
> Bug:  796151 
> Change-Id: Ica6f6ef47457eb34f82923ec6ae144f6b5b67fda
> Reviewed-on: https://chromium-review.googlesource.com/852234
> Commit-Queue: Christian Dullweber <dullweber@chromium.org>
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Reviewed-by: Ken Rockot <rockot@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#533707}

TBR=rockot@chromium.org,mek@chromium.org,jochen@chromium.org,dullweber@chromium.org

Change-Id: I2e7dd6dc7d88964b88963278daeb40cc33f6c0b2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  796151 
Reviewed-on: https://chromium-review.googlesource.com/898462
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Commit-Queue: Tommy Steimel <steimel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533851}
[modify] https://crrev.com/2768203553deae4587be1b6840b4e06f02f8c870/chrome/browser/extensions/extension_gcm_app_handler_unittest.cc
[modify] https://crrev.com/2768203553deae4587be1b6840b4e06f02f8c870/chrome/browser/extensions/extension_service_test_base.cc
[modify] https://crrev.com/2768203553deae4587be1b6840b4e06f02f8c870/chrome/browser/extensions/extension_service_test_base.h
[modify] https://crrev.com/2768203553deae4587be1b6840b4e06f02f8c870/content/browser/storage_partition_impl.cc
[modify] https://crrev.com/2768203553deae4587be1b6840b4e06f02f8c870/content/browser/storage_partition_impl.h
[modify] https://crrev.com/2768203553deae4587be1b6840b4e06f02f8c870/content/public/browser/storage_partition.h
[modify] https://crrev.com/2768203553deae4587be1b6840b4e06f02f8c870/content/public/test/test_storage_partition.cc
[modify] https://crrev.com/2768203553deae4587be1b6840b4e06f02f8c870/content/public/test/test_storage_partition.h

Project Member

Comment 12 by bugdroid1@chromium.org, Feb 2 2018

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

commit 64d38d08942413ac2ecb5451683fd99d04390c3f
Author: Christian Dullweber <dullweber@chromium.org>
Date: Fri Feb 02 14:06:31 2018

Reland "Let StoragePartition wait for localstorage deletions"

This is a reland of 9a5b79ceeccc1b5f20057354241e96c8ddeb96b7.

Original change's description:
> Let StoragePartition wait for localstorage deletions
>
> This CL changes StoragePartition to wait for localstorage deletions
> to finish before calling its callback.
>
> Some extension tests can leak memory because they don't wait until
> StorageParitition deletions are finished. This CL adds a method to
> StoragePartition to check if it is done and changes extension tests
> to always wait for StoragePartition.
>
> Bug:  796151 
> Change-Id: Ica6f6ef47457eb34f82923ec6ae144f6b5b67fda
> Reviewed-on: https://chromium-review.googlesource.com/852234
> Commit-Queue: Christian Dullweber <dullweber@chromium.org>
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Reviewed-by: Ken Rockot <rockot@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#533707}

TBR=rockot@chromium.org,mek@chromium.org,jochen@chromium.org

Bug:  796151 
Change-Id: I2fa54361fe4066266d59108513a0f4e284bea161
Reviewed-on: https://chromium-review.googlesource.com/897763
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534043}
[modify] https://crrev.com/64d38d08942413ac2ecb5451683fd99d04390c3f/chrome/browser/chromeos/extensions/gfx_utils_unittest.cc
[modify] https://crrev.com/64d38d08942413ac2ecb5451683fd99d04390c3f/chrome/browser/extensions/extension_gcm_app_handler_unittest.cc
[modify] https://crrev.com/64d38d08942413ac2ecb5451683fd99d04390c3f/chrome/browser/extensions/extension_service_test_base.cc
[modify] https://crrev.com/64d38d08942413ac2ecb5451683fd99d04390c3f/chrome/browser/extensions/extension_service_test_base.h
[modify] https://crrev.com/64d38d08942413ac2ecb5451683fd99d04390c3f/chrome/browser/extensions/external_provider_impl_chromeos_unittest.cc
[modify] https://crrev.com/64d38d08942413ac2ecb5451683fd99d04390c3f/chrome/browser/ui/app_list/arc/arc_app_unittest.cc
[modify] https://crrev.com/64d38d08942413ac2ecb5451683fd99d04390c3f/chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc
[modify] https://crrev.com/64d38d08942413ac2ecb5451683fd99d04390c3f/content/browser/storage_partition_impl.cc
[modify] https://crrev.com/64d38d08942413ac2ecb5451683fd99d04390c3f/content/browser/storage_partition_impl.h
[modify] https://crrev.com/64d38d08942413ac2ecb5451683fd99d04390c3f/content/public/browser/storage_partition.h
[modify] https://crrev.com/64d38d08942413ac2ecb5451683fd99d04390c3f/content/public/test/test_storage_partition.cc
[modify] https://crrev.com/64d38d08942413ac2ecb5451683fd99d04390c3f/content/public/test/test_storage_partition.h

Components: -Blink>Storage Blink>Storage>DOMStorage
Done? (and thanks for tackling this!)
Status: Fixed (was: Started)
Yes!

Sign in to add a comment