Local storage deletion is flaky |
|||||
Issue description
Tested on 61.0.3137.0:
Steps:
1. Open a random page (e.g. localhost)
2. Add a few local storage entries (e.g. by executing 'localStorage["key"] = "value";' in the DevTools console
3. Verify that they have been added (e.g. by executing 'localStorage.length > 0' in the DevTools console, which should return 'true')
4. Open 'chrome://settings/clearBrowserData' in a new tab
5. Clear 'Cookies and other site data'
6. Verify that local storage is empty ('!!localStorage.length')
Expected:
7. Local storage is empty ('true')
Actual:
7. Local storage is not empty ('false')
,
Jun 20 2017
I verified that the call reached StoragePartitionImpl::ClearLocalStorageOnUIThread(), so the bug is somewhere lower than that.
,
Jun 20 2017
DomStorageContextImpl::DeleteLocalStorage and/or DomStorageContextImpl::DeleteLocalStorageForPhysicalOrigin are not called in cases of unsuccessful deletion. So the bug is somewhere in StoragePartitionImpl.
,
Jun 20 2017
StoragePartitionImpl::OnLocalStorageUsageInfo doesn't see the origin in question. The data is not deleted, because the storage partition doesn't know it's there. ...even though the DevTools and the origin info bubble do.
,
Jun 20 2017
So the bug is in DOMStorageContextImpl::GetLocalStorageUsage. Passing to owners for triage.
,
Jun 20 2017
Anything related to DOMStorageContextImpl is likely a red herring. In M61 localstorage is no longer implemented in DOMStorageContextImpl but in LocalStorageContextMojo instead. Having said that, this still sounds like a bug of course.
,
Jun 20 2017
What is probably happening is that the data hasn't been committed to disk yet, which causes the new implementation of GetLocalStorageUsage to not include the origin at all (the old implementation would generally not count uncommitted data as usage either, but since it would have an empty sqlite database for the origin, that would be included).
,
Jun 20 2017
,
Jun 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8309dd9de3baa6a3897d3b1fe094e26846f9bad8 commit 8309dd9de3baa6a3897d3b1fe094e26846f9bad8 Author: Marijn Kruisselbrink <mek@chromium.org> Date: Thu Jun 22 19:57:44 2017 Make GetStorageUsage include orgins with only uncommitted data. In general GetStorageUsage only returns usage as it is on disk, which means it only returns committed data. But the same API is also used to figure out which origins have localstorage data in general, so make sure to always include all origins that haven't been committed to disk yet as well. In the old code this worked automatically since opening localstorage would cause an empty sqlite database to be created for that origin. Bug: 735040 Change-Id: Idc5410f30404c8ba6d5110416cb019d5d4c415ea Reviewed-on: https://chromium-review.googlesource.com/541498 Commit-Queue: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Reviewed-by: Theresa <twellington@chromium.org> Reviewed-by: Martin Šrámek <msramek@chromium.org> Cr-Commit-Position: refs/heads/master@{#481638} [modify] https://crrev.com/8309dd9de3baa6a3897d3b1fe094e26846f9bad8/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java [modify] https://crrev.com/8309dd9de3baa6a3897d3b1fe094e26846f9bad8/content/browser/dom_storage/local_storage_context_mojo.cc [modify] https://crrev.com/8309dd9de3baa6a3897d3b1fe094e26846f9bad8/content/browser/dom_storage/local_storage_context_mojo_unittest.cc
,
Jun 26 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by msramek@chromium.org
, Jun 20 2017