LocalStorage migration code can leave DB in inconsistent state, resulting in misbehaving removeItem
Reported by
thin...@gmail.com,
Sep 15 2017
|
||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.91 Safari/537.36 Steps to reproduce the problem: 1. In the console of the browser type localStorage 2. pick a key from it, and then remove it by localStorage.removeItem(YOUR_KEY) 3. type localStorage again, it's shown that YOUR_KEY has been removed 4. Close the browser down using (Command-Q) 5. Open the browser up and go to the same URL and open Console and type localStorage again, YOUR_KEY is now there What is the expected behavior? YOUR_KEY should have been gone from the localStorage What went wrong? YOUR_KEY is still present in localStorage Did this work before? N/A Does this work in other browsers? Yes Chrome version: 61.0.3163.91 Channel: stable OS Version: OS X 10.12.6 Flash Version: It seems to only happen for certain websites/domains
,
Sep 15 2017
Rechecked the issue using Stable version 61.0.3163.91 on MAC 10.12.6. Below were the steps followed: 1) Navigated to www.google.com 2) On dev console typed "localStorage" 3) Unable to identify any specific key for the output generated. (Screen shot attached) @ thinh76: request you to please take a look at the screen shot attached and let us know what exactly could be a key here. This will help us to triage the issue. Also if this website does not generate any keys, can you please suggest any sample website when we can get it. Thanks.!
,
Sep 15 2017
Please see attachments. The first screenshot "remove_one_key.png" is the step that I removed the key, then I quit the browser (Command-Q) and then open up the browser again and you can see the key I removed still remained there (key_still_remain). The key I removed was "FeatureTag.IDAM_AUTH" This behaviour started to happen yesterday the 14th of Sep and somehow these keys got stored in the Default profile (I am assuming) and couldn't remove them individually. However, if I did localStorage.clear() and closed browser, re-open browser it worked as expected. I am assuming it's the glitch between updating Chrome version from the latest 60 to 61? (I updated it sometimes yesterday).
,
Sep 15 2017
Thank you for providing more feedback. Adding requester "ranjitkan@chromium.org" to the cc list and removing "Needs-Feedback" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 15 2017
,
Sep 15 2017
,
Sep 18 2017
,
Sep 20 2017
Can confirm bug is present in Version 61.0.3163.91 (Official Build) (64-bit) Windows 10. Also on same version on MacOS. Evem when overwriting value does not help in my case. Still after restart of browser old data is back.
,
Sep 21 2017
Hmm, that's not good... There definitely seems to be something fishy going on with removeItem (maybe with other parts as well, but I haven't gotten any clear reports for that yet). Somebody else reported removeItem in one renderer process not propagating to other renderer processes (or maybe not even to the browser process) which sounds like it could be either the same bug as this, or at least related...
,
Sep 21 2017
I investigated my leveldb local storage folder and find there are 2 nearly identical keys. If I run localStorage.getItem then I get the value of key 2, but if when I delete the key using localStorage.removeItem, I can still do localStorage.getItem and get the value of key 1 (older key) _http://localhost:8080aHR0cDovL2xvY2FsaG9zdDo4MDgw_stargazer_token [95 104 116 116 112 58 47 47 108 111 99 97 108 104 111 115 116 58 56 48 56 48 0 0 97 0 72 0 82 0 48 0 99 0 68 0 111 0 118 0 76 0 50 0 120 0 118 0 89 0 50 0 70 0 115 0 97 0 71 0 57 0 122 0 100 0 68 0 111 0 52 0 77 0 68 0 103 0 119 0 95 0 115 0 116 0 97 0 114 0 103 0 97 0 122 0 101 0 114 0 95 0 116 0 111 0 107 0 101 0 110 0] eyJhbGciOiJIUzUxMiJ9.eyJib2R5Ijoie1widXNlcklkXCI6e1widXNlcklkJGFjY2VzcyQwXCI6XCJoYXJ5LndpbGxpYW1zLmljQGdtYWlsLmNvbVwifSxcImNvb2tpZVwiOlwiZjBjMmExNjFjZDExMGQzODEzNjE4YzQ4MTc3MmQ1MzMxYWFlM2U3Y1wiLFwic2d3VXJsXCI6XCJodHRwOi8vc3luYy1nYXRld2F5LmdvbmRvci5zdmMua3ViZS1sb2NhbC5pbzo4MC9zeW5jX2dhdGV3YXlcIn0iLCJleHAiOjE1MDUyODA0MjYsImlhdCI6MTUwNTE5NDAyNiwianRpIjoiIiwiaXNzIjoiYXV0aG8ifQ.c5x0ZTYUpFyghiZUoYs7DE5ezzuFDbRvoSPaU5_qYPW6aB6Zi14zQJtzMzUxNLFNczv8w6Cb3Qa2DM4uq-w9Ow [0 101 0 121 0 74 0 104 0 98 0 71 0 99 0 105 0 79 0 105 0 74 0 73 0 85 0 122 0 85 0 120 0 77 0 105 0 74 0 57 0 46 0 101 0 121 0 74 0 105 0 98 0 50 0 82 0 53 0 73 0 106 0 111 0 105 0 101 0 49 0 119 0 105 0 100 0 88 0 78 0 108 0 99 0 107 0 108 0 107 0 88 0 67 0 73 0 54 0 101 0 49 0 119 0 105 0 100 0 88 0 78 0 108 0 99 0 107 0 108 0 107 0 74 0 71 0 70 0 106 0 89 0 50 0 86 0 122 0 99 0 121 0 81 0 119 0 88 0 67 0 73 0 54 0 88 0 67 0 74 0 111 0 89 0 88 0 74 0 53 0 76 0 110 0 100 0 112 0 98 0 71 0 120 0 112 0 89 0 87 0 49 0 122 0 76 0 109 0 108 0 106 0 81 0 71 0 100 0 116 0 89 0 87 0 108 0 115 0 76 0 109 0 78 0 118 0 98 0 86 0 119 0 105 0 102 0 83 0 120 0 99 0 73 0 109 0 78 0 118 0 98 0 50 0 116 0 112 0 90 0 86 0 119 0 105 0 79 0 108 0 119 0 105 0 90 0 106 0 66 0 106 0 77 0 109 0 69 0 120 0 78 0 106 0 70 0 106 0 90 0 68 0 69 0 120 0 77 0 71 0 81 0 122 0 79 0 68 0 69 0 122 0 78 0 106 0 69 0 52 0 89 0 122 0 81 0 52 0 77 0 84 0 99 0 51 0 77 0 109 0 81 0 49 0 77 0 122 0 77 0 120 0 89 0 87 0 70 0 108 0 77 0 50 0 85 0 51 0 89 0 49 0 119 0 105 0 76 0 70 0 119 0 105 0 99 0 50 0 100 0 51 0 86 0 88 0 74 0 115 0 88 0 67 0 73 0 54 0 88 0 67 0 74 0 111 0 100 0 72 0 82 0 119 0 79 0 105 0 56 0 118 0 99 0 51 0 108 0 117 0 89 0 121 0 49 0 110 0 89 0 88 0 82 0 108 0 100 0 50 0 70 0 53 0 76 0 109 0 100 0 118 0 98 0 109 0 82 0 118 0 99 0 105 0 53 0 122 0 100 0 109 0 77 0 117 0 97 0 51 0 86 0 105 0 90 0 83 0 49 0 115 0 98 0 50 0 78 0 104 0 98 0 67 0 53 0 112 0 98 0 122 0 111 0 52 0 77 0 67 0 57 0 122 0 101 0 87 0 53 0 106 0 88 0 50 0 100 0 104 0 100 0 71 0 86 0 51 0 89 0 88 0 108 0 99 0 73 0 110 0 48 0 105 0 76 0 67 0 74 0 108 0 101 0 72 0 65 0 105 0 79 0 106 0 69 0 49 0 77 0 68 0 85 0 121 0 79 0 68 0 65 0 48 0 77 0 106 0 89 0 115 0 73 0 109 0 108 0 104 0 100 0 67 0 73 0 54 0 77 0 84 0 85 0 119 0 78 0 84 0 69 0 53 0 78 0 68 0 65 0 121 0 78 0 105 0 119 0 105 0 97 0 110 0 82 0 112 0 73 0 106 0 111 0 105 0 73 0 105 0 119 0 105 0 97 0 88 0 78 0 122 0 73 0 106 0 111 0 105 0 89 0 88 0 86 0 48 0 97 0 71 0 56 0 105 0 102 0 81 0 46 0 99 0 53 0 120 0 48 0 90 0 84 0 89 0 85 0 112 0 70 0 121 0 103 0 104 0 105 0 90 0 85 0 111 0 89 0 115 0 55 0 68 0 69 0 53 0 101 0 122 0 122 0 117 0 70 0 68 0 98 0 82 0 118 0 111 0 83 0 80 0 97 0 85 0 53 0 95 0 113 0 89 0 80 0 87 0 54 0 97 0 66 0 54 0 90 0 105 0 49 0 52 0 122 0 81 0 74 0 116 0 122 0 77 0 122 0 85 0 120 0 78 0 76 0 70 0 78 0 99 0 122 0 118 0 56 0 119 0 54 0 67 0 98 0 51 0 81 0 97 0 50 0 68 0 77 0 52 0 117 0 113 0 45 0 119 0 57 0 79 0 119 0] _http://localhost:8080aHR0cDovL2xvY2FsaG9zdDo4MDgw_stargazer_token [95 104 116 116 112 58 47 47 108 111 99 97 108 104 111 115 116 58 56 48 56 48 0 1 97 72 82 48 99 68 111 118 76 50 120 118 89 50 70 115 97 71 57 122 100 68 111 52 77 68 103 119 95 115 116 97 114 103 97 122 101 114 95 116 111 107 101 110] eyJhbGciOiJIUzUxMiJ9.eyJib2R5Ijoie1widXNlcklkXCI6e1widXNlcklkJGFjY2VzcyQwXCI6XCJoYXJ5LndpbGxpYW1zLmljQGdtYWlsLmNvbVwifSxcImNvb2tpZVwiOlwiYmJjNjlhNzEwN2U0YjM4Y2JiOWYyZWUyNzNkNGYxYjNmMWE1MjkwNFwiLFwic2d3VXJsXCI6XCJodHRwOi8vc3luYy1nYXRld2F5LmdvbmRvci5zdmMua3ViZS1sb2NhbC5pbzo4MC9zeW5jX2dhdGV3YXlcIn0iLCJleHAiOjE1MDYwNzMxMDcsImlhdCI6MTUwNTk4NjcwNywianRpIjoiIiwiaXNzIjoiYXV0aG8ifQ.6dCmDQ2AxI14E7RLrDotxzF6BWx5w-ya2pAJvLGK1spNgfABh4kGn-D-nTWkx0FYZEybWvq8El-en5Pv-XQ5Xw [1 101 121 74 104 98 71 99 105 79 105 74 73 85 122 85 120 77 105 74 57 46 101 121 74 105 98 50 82 53 73 106 111 105 101 49 119 105 100 88 78 108 99 107 108 107 88 67 73 54 101 49 119 105 100 88 78 108 99 107 108 107 74 71 70 106 89 50 86 122 99 121 81 119 88 67 73 54 88 67 74 111 89 88 74 53 76 110 100 112 98 71 120 112 89 87 49 122 76 109 108 106 81 71 100 116 89 87 108 115 76 109 78 118 98 86 119 105 102 83 120 99 73 109 78 118 98 50 116 112 90 86 119 105 79 108 119 105 89 109 74 106 78 106 108 104 78 122 69 119 78 50 85 48 89 106 77 52 89 50 74 105 79 87 89 121 90 87 85 121 78 122 78 107 78 71 89 120 89 106 78 109 77 87 69 49 77 106 107 119 78 70 119 105 76 70 119 105 99 50 100 51 86 88 74 115 88 67 73 54 88 67 74 111 100 72 82 119 79 105 56 118 99 51 108 117 89 121 49 110 89 88 82 108 100 50 70 53 76 109 100 118 98 109 82 118 99 105 53 122 100 109 77 117 97 51 86 105 90 83 49 115 98 50 78 104 98 67 53 112 98 122 111 52 77 67 57 122 101 87 53 106 88 50 100 104 100 71 86 51 89 88 108 99 73 110 48 105 76 67 74 108 101 72 65 105 79 106 69 49 77 68 89 119 78 122 77 120 77 68 99 115 73 109 108 104 100 67 73 54 77 84 85 119 78 84 107 52 78 106 99 119 78 121 119 105 97 110 82 112 73 106 111 105 73 105 119 105 97 88 78 122 73 106 111 105 89 88 86 48 97 71 56 105 102 81 46 54 100 67 109 68 81 50 65 120 73 49 52 69 55 82 76 114 68 111 116 120 122 70 54 66 87 120 53 119 45 121 97 50 112 65 74 118 76 71 75 49 115 112 78 103 102 65 66 104 52 107 71 110 45 68 45 110 84 87 107 120 48 70 89 90 69 121 98 87 118 113 56 69 108 45 101 110 53 80 118 45 88 81 53 88 119]
,
Sep 22 2017
re #10, what do you mean with "nearly identical keys"? the two keys you pasted look identical, so are you saying those somehow both exist in leveldb? And you initially get the value of the second one, and after calling removeItem you get the value for the first copy of seemingly the same key?
,
Sep 22 2017
Ah, I figured out what is going on. The code that migrates data from the old (sqlite) based disk format to the new leveldb based format wasn't entirely compatible with how the new code expected data to look like, resulting in these situations where updating or removing items can result in buggy behavior like what is mentioned here (removing not working, or reverting back to an old value of a key. Or probably various other weirdness).
,
Sep 22 2017
,
Sep 22 2017
,
Sep 22 2017
,
Sep 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e8fdb3d52ab59c69baabcda544690100a2a136ea commit e8fdb3d52ab59c69baabcda544690100a2a136ea Author: Marijn Kruisselbrink <mek@chromium.org> Date: Tue Sep 26 01:33:07 2017 Fix bug in localstorage migration that could leave DB in inconsistent state. This extends the LevelDBWrapperImpl API to enable users of that class to fixup broken data in the database. That API is then used to fix incorrectly encoded keys, which should fix all weirdness around removeItem that was being observed. BUG= 765524 Change-Id: I4049445d46d30925774c2fec9dfd4dcdaba12e04 Reviewed-on: https://chromium-review.googlesource.com/678250 Reviewed-by: Mark Pearson <mpearson@chromium.org> Reviewed-by: Michael Nordman <michaeln@chromium.org> Commit-Queue: Marijn Kruisselbrink <mek@chromium.org> Cr-Commit-Position: refs/heads/master@{#504251} [modify] https://crrev.com/e8fdb3d52ab59c69baabcda544690100a2a136ea/content/browser/dom_storage/local_storage_context_mojo.cc [modify] https://crrev.com/e8fdb3d52ab59c69baabcda544690100a2a136ea/content/browser/dom_storage/local_storage_context_mojo_unittest.cc [modify] https://crrev.com/e8fdb3d52ab59c69baabcda544690100a2a136ea/content/browser/leveldb_wrapper_impl.cc [modify] https://crrev.com/e8fdb3d52ab59c69baabcda544690100a2a136ea/content/browser/leveldb_wrapper_impl.h [modify] https://crrev.com/e8fdb3d52ab59c69baabcda544690100a2a136ea/content/browser/leveldb_wrapper_impl_unittest.cc [modify] https://crrev.com/e8fdb3d52ab59c69baabcda544690100a2a136ea/tools/metrics/histograms/histograms.xml
,
Sep 26 2017
Marijn, thanks for the fix. Quick questions: - Will this retroactively fix localstorage for users that already experienced the issue? Meaning does this also cleanup the storage so that keys that were deleted will no longer appear when read? or does it just prevent the issue from happening for keys that will be deleted after this fix? - Should this be merged into M62?
,
Sep 26 2017
Unfortunately due to the bug there is no way of knowing which keys were actually deleted. So we had two choices: delete all data that was migrated from pre-M61 localstorage, or just fix the data on disk such that future deletes of individual items will work. Deleting all data seemed worse than just assuming nothing was deleted, so that's what this fix does. Yes, it's probably a bad enough bug (websites thinking they deleted data but not actually having deleted the data) that merging to M62 makes sense.
,
Sep 26 2017
This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 27 2017
Has this fix been verified in canary?
,
Sep 27 2017
If anybody wants to try to verify this, you should be able to reproduce the original issue fairly reliably by somehow getting a profile (or at least the Local Storage part of one) created by M60 or earlier (with some localstorage data in it). In M61 and any other later version without this fix removeItem wouldn't be able to delete any of the pre-M61 localstorage data, while with the fix it should work.
,
Sep 27 2017
Ranjith, can you verify this in canary.
,
Sep 27 2017
Approved for 62, assuming this is verified on Canary.
,
Sep 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6614575900438cfea1c1099d469eb0707a3210e4 commit 6614575900438cfea1c1099d469eb0707a3210e4 Author: Marijn Kruisselbrink <mek@chromium.org> Date: Fri Sep 29 18:18:30 2017 Fix bug in localstorage migration that could leave DB in inconsistent state. This extends the LevelDBWrapperImpl API to enable users of that class to fixup broken data in the database. That API is then used to fix incorrectly encoded keys, which should fix all weirdness around removeItem that was being observed. BUG= 765524 TBR=mek@chromium.org (cherry picked from commit e8fdb3d52ab59c69baabcda544690100a2a136ea) Change-Id: I4049445d46d30925774c2fec9dfd4dcdaba12e04 Reviewed-on: https://chromium-review.googlesource.com/678250 Reviewed-by: Mark Pearson <mpearson@chromium.org> Reviewed-by: Michael Nordman <michaeln@chromium.org> Commit-Queue: Marijn Kruisselbrink <mek@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#504251} Reviewed-on: https://chromium-review.googlesource.com/692632 Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Cr-Commit-Position: refs/branch-heads/3202@{#511} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/6614575900438cfea1c1099d469eb0707a3210e4/content/browser/dom_storage/local_storage_context_mojo.cc [modify] https://crrev.com/6614575900438cfea1c1099d469eb0707a3210e4/content/browser/dom_storage/local_storage_context_mojo_unittest.cc [modify] https://crrev.com/6614575900438cfea1c1099d469eb0707a3210e4/content/browser/leveldb_wrapper_impl.cc [modify] https://crrev.com/6614575900438cfea1c1099d469eb0707a3210e4/content/browser/leveldb_wrapper_impl.h [modify] https://crrev.com/6614575900438cfea1c1099d469eb0707a3210e4/content/browser/leveldb_wrapper_impl_unittest.cc [modify] https://crrev.com/6614575900438cfea1c1099d469eb0707a3210e4/tools/metrics/histograms/histograms.xml
,
Oct 2 2017
Should be fixed in M62 (once that releases) and M63. M61 unfortunately will remain broken.
,
Oct 3 2017
What is a recommended workaround here that you would recommend for M61 and below? Does the issue repro if we set the localstorage value to an empty string rather than using removeItem() ?
,
Oct 3 2017
Setting it to an empty string should probably work, yes. Both the old and new values will exist in the database, but I think you should always get back the correct (i.e. new) value as long as both exist (since internally values just happen to be loaded in sorted key order, and the correct key encoding will always sort after the incorrect one, thus overwriting the older one).
,
Oct 3 2017
But presumably we would never be able to remove that item? Since whenever we remove it, the old one will reappear from under it? Is there really no way Chrome can detect that there are two values and somehow clear the old one?
,
Oct 3 2017
Yes, with M61 there wouldn't be any way to remove the item. The fix (in M62 once that gets another release and M63) does indeed detect that there are two values and clear the old value. It's only when there is only one value that it is unknown if the value should be deleted or not.
,
Oct 3 2017
Just one last question. When does the detection & clearing happen? Does it happen whenever a page on that domain loads, whenever localstorage is first used, or when that particular localstorage key is read?
,
Oct 3 2017
The first time a page on that origin loads localstorage.
,
Oct 5 2017
Issue 771555 has been merged into this issue.
,
Oct 19 2017
When would the fix be released please?
,
Oct 19 2017
When M62 hits stable, which is currently happening. Now releases generally aren't pushed to all users at the same time, so it might still be a few days until a particular user gets the update. |
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by gov...@chromium.org
, Sep 15 2017Labels: M-61 Needs-Triage-M61