[Affected versions]: Nightly 65.0a1 Build ID: 20181024152251 Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15e7c4c036a265f9258944dc4f5b867d104c5d51 [Affected platforms]: Ubuntu 16.04 x64 [Prerequisites]: - start the try build and set dom.storage.next_gen to "false" [Steps to reproduce]: 1. Open ~20 sites and fully load them 2. Turn on NextGen Storage by setting the pref to "true" 3. Reload all tabs 4. Restart the browser using the same profile 5. Leave it idle like 3-5 mins or continue browsing the previously opened sites (make sure they are restored when Nightly is opened) 6. Go to about:preferences#privacy and check the size of the Cookies, site data and cache stored *** reload all tabs several times (including about:preferences#privacy) to reproduce the issue, might take several retries *** [Expected Result]: The stored data should be within reasonable limits. [Actual Result]: Check the attached file. There will be an abnormal used storage space displayed. Sometimes that number doubles and interestingly it will be the same number even if I open different 20 pages in a new profile and follow the same steps to reproduce the issue. [Notes]: This number might turn back to normal after refreshing the page or restarting, however, it may also re-appear. Restarting the browser does not help either, it can be reproduced on the same profile again afterward.

Can you send me exact list of those 20 sites ? Thanks.

Exploratory was performed on the following top 20 sites: 1. Google.com 2. Youtube.com 3. Facebook.com 4. Baidu.com 5. Wikipedia.org 6. Yahoo.com 7. Qq.com 8. Taobao.com 9. Tmall.com 10. Twitter.com 11. Amazon.com 12. Instagram.com 13. Vk.com 14. Sohu.com 15. Reddit.com 16. Jd.com 17. Live.com 18. Yandex.ru 19. Weibo.com 20. 360.cn

I have a patch for this.

Comment on attachment 9031831 [details] [diff] [review] patch Review of attachment 9031831 [details] [diff] [review]: ----------------------------------------------------------------- I don't understand how this patch fixes things by only adding asserts, and I'm unable to easily stitch a guess together from the patch/bug/commit message. If it was one of the other bugs' patches, they should be marked as dependencies and the commit message should explain that, thanks! ::: dom/localstorage/test/unit/test_usage.js @@ +7,5 @@ > +{ > + const data = {}; > + data.key = "key1"; > + data.value = "value1"; > + data.usage = data.key.length + data.value.length; Maybe add a comment here like: Note that LocalStorage's internal sizing estimates are based on the number of utf-8 bytes in a string whereas JS's string length is the number of utf-16 code units. So this is only true when our string is made up of ASCII characters 0-127.

Comment on attachment 9031831 [details] [diff] [review] patch Review of attachment 9031831 [details] [diff] [review]: ----------------------------------------------------------------- :janv explained offline: The usage Get() call was *only* happening in the MOZ_ASSERT path, which is obvious from the patch except when your brain is assuming that couldn't possibly be the case that that was the bug, because of course the tests must work! ;)

(In reply to Andrew Sutherland [:asuth] from comment #5) > @@ +7,5 @@ > > +{ > > + const data = {}; > > + data.key = "key1"; > > + data.value = "value1"; > > + data.usage = data.key.length + data.value.length; > > Maybe add a comment here like: > > Note that LocalStorage's internal sizing estimates are based on the number > of utf-8 bytes in a string whereas JS's string length is the number of > utf-16 code units. So this is only true when our string is made up of ASCII > characters 0-127. I'm not sure about this. I tried these 3 variations: 1. ASCII character 0-127 data.value = "v"; data.value.length in JS returns 1 aValue.Length() in C++ (LSSnapshot::SetItem, aValue is nsAString) returns 1 length(value) in SQL (|pragma encoding| returns UTF-8) returns 1 2. Latin Letter data.value = "á"; // c3 a1, LATIN SMALL LETTER A WITH ACUTE data.value.length in JS returns 1 aValue.Length() in C++ returns 1 length(value) in SQL returns 1 3. Unicode Character data.value = "\u10400"; // U+10400, DESERET CAPITAL LETTER LONG I data.value.length in JS returns 2 aValue.Length() in C++ returns 2 length(value) in SQL returns 2 So I'm not really sure we need to worry about non ASCII 0-127 characters. The problem described in bug 1513892 comment 12 is probably something else, in some specific case sqlite returns different length for given string. It's probably a bug in our length computation or a bug in sqlite's length implementation. However we don't want to depend on correct length calculation in sqlite, so we will always calculate usage in our C++ code and store results in database for fast retrieval later.

Andrew, can I land the patch w/o the comment in the test ?

I had badly confused myself when I wrote that suggested comment. (I think I was looking at the BLOB typing when I re-read the docs.) Apologies. My comment at https://bugzilla.mozilla.org/show_bug.cgi?id=1513892#c20 is an accurate characterization. JS and nsAString::Length() return the number of utf-16 code units, SQLite length() is the number of characters (regardless of how many bytes the character needs. (In reply to Jan Varga [:janv] from comment #7) > 3. Unicode Character > data.value = "\u10400"; // U+10400, DESERET CAPITAL LETTER LONG I > > data.value.length in JS returns 2 > aValue.Length() in C++ returns 2 > length(value) in SQL returns 2 Hm, so, I get a length of 1 for SQLite if directly providing the character. sqlite> select length(char(0x10400)); 1 And the char() generation seems to do the right thing too: sqlite> select char(0x10400); 𐐀 So I'm a little concerned about this. Can you double check whatever steps you performed here?

And also note that my comment was further confused by assuming we weren't fixing the localStorage length() usage, while also being confused about how length() worked.

(In reply to Andrew Sutherland [:asuth] from comment #9) > > 3. Unicode Character > > data.value = "\u10400"; // U+10400, DESERET CAPITAL LETTER LONG I > > > > data.value.length in JS returns 2 > > aValue.Length() in C++ returns 2 > > length(value) in SQL returns 2 > > Hm, so, I get a length of 1 for SQLite if directly providing the character. > sqlite> select length(char(0x10400)); > 1 > And the char() generation seems to do the right thing too: > sqlite> select char(0x10400); > 𐐀 > > So I'm a little concerned about this. Can you double check whatever steps > you performed here? Here's what I did with test_usage.js: - I replaced |data.value = "value1";| with |data.value = "\u10400";| - I commented out the "Stage 2" to not remove the value - I ran |mach xpcshell-test --interactive dom/localstorage/test/unit/test_usage.js| to get data.sqlite - I ran |select length(value) from data;| and it returns 2 |select length(char(0x10400));| returns 1

Comment on attachment 9031831 [details] [diff] [review] patch Review of attachment 9031831 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/localstorage/test/unit/test_usage.js @@ +7,5 @@ > +{ > + const data = {}; > + data.key = "key1"; > + data.value = "value1"; > + data.usage = data.key.length + data.value.length; Yeah, let's forget about this comment since we're dealing with the unicode issues in the other bug. Feel free to land this as-is. Thanks. I'm still interested in making sure we understand what the 1 versus 2 difference is for our bug thread, though.

(In reply to Jan Varga [:janv] from comment #11) > Here's what I did with test_usage.js: > - I replaced |data.value = "value1";| with |data.value = "\u10400";| Ah, the problem is that you're providing 5 characters to the escape, not 4. inp: '\u10400' out: "၀0" You want to do: '\u{10400}' inp: '\u{10400}' out: "𐐀"

Ah, that explains it, thanks. So, I will land the patch later today.

Pushed by jvarga@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb63db77102 The used storage space for saved cookies,site data, cache displayed in about:preferences is abnormal after enabling NextGen Storage; r=asuth