1588043 - Restoring bookmarks may use a lot of memory if history limits are removed

Status: NEW

(Toolkit :: Places, defect, P5)

Description

[User Dderss]

Attachment: FF 70b13 bookmarks import leak, endless look, OOM failure.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:70.0) Gecko/20100101 Firefox/70.0

Steps to reproduce:

Opened Bookmarks, then Import and Backup, then Restore, then any of recent backup files.

Actual results:

The memory footprint of FF (70.0b13) has started to quickly rise to the maximum of all available memory (to 15 GB and beyond) and until complete death of the system. To prevent the latter in my tests I had to act quickly enough to kill FF via Task Manager before it is too late.

I tried at least four different bookmarks files with a different number of entries to make sure that the issue is not just a single glitchy/corrupt JSON (JSONLZ4) file. I also uncompressed one of the JSONLZ4 bookmark files via https://www.jeffersonscher.com/ffu/bookbackreader.html and then checked unpacked JSON file via jsonlint.com -- both went just fine, everything if correct and valid.

I could provide my JSONLZ4 file for testing, if necessary, but before that, I need an anonymizer for bookmarks (I guess Mozilla should have such a tool, if so, where is it?)

Expected results:

Bookmarks should have been restored just fine.

Comment 1

[User Dderss]

FOR CONTEXT: I need to restore my bookmarks after FF 70.0b13 has gotten corrupt in real-time and yanked all of my multi-word tags. During this FF was not even allowing me to create new multi-word tags: they were just disappearing immediately after I used them. After restarting the browser, I am already able to create multi-word tags but all of those that existed prior have gone forever, so I need to restore my bookmarks from yesterday when all of my tags, including multi-word ones, existed. But as it turns out, it is impossible for now.

Comment 2

[Marco Bonardo [:mak]]

Could you please run Places Database Integrity from about:support and report here the log it prints? Thanks.

The tags with space problem is due to Sync in Firefox Preview, see Bug 1570510.

Comment 3

[Marco Bonardo [:mak]]

Ah, you can't see Bug 1570510 because it contained some personal information of a user and was made confidential. Anyway, the new Firefox Preview version should solve the problem.

Comment 4

[User Dderss]

> Task: checkIntegrity
+ The places.sqlite database is sane
+ The favicons.sqlite database is sane
> Task: invalidateCaches
+ The caches have been invalidated
> Task: checkCoherence
+ The database is coherent
> Task: expire
+ Database cleaned up
> Task: originFrecencyStats
+ Recalculated origin frecency stats
> Task: vacuum
+ Initial database size is 440320KiB
+ The database has been vacuumed
+ Final database size is 430080KiB
> Task: stats
+ Places.sqlite size is 430080KiB
+ Favicons.sqlite size is 40256KiB
+ pragma_user_version is 53
+ pragma_page_size is 4096
+ pragma_cache_size is -2048
+ pragma_journal_mode is wal
+ pragma_synchronous is 1
+ History can store a maximum of 99999999 unique pages
+ Table moz_bookmarks has 18474 records
+ Table moz_keywords has 2 records
+ Table sqlite_sequence has 1 records
+ Table moz_annos has 5138 records
+ Table moz_anno_attributes has 3 records
+ Table moz_items_annos has 0 records
+ Table moz_places has 925123 records
+ Table moz_historyvisits has 1270705 records
+ Table moz_inputhistory has 101 records
+ Table sqlite_stat1 has 20 records
+ Table moz_hosts has 0 records
+ Table moz_bookmarks_deleted has 0 records
+ Table moz_meta has 9 records
+ Table moz_origins has 9987 records
+ Index sqlite_autoindex_moz_keywords_1
+ Index sqlite_autoindex_moz_anno_attributes_1
+ Index sqlite_autoindex_moz_inputhistory_1
+ Index sqlite_autoindex_moz_hosts_1
+ Index sqlite_autoindex_moz_bookmarks_deleted_1
+ Index sqlite_autoindex_moz_origins_1
+ Index moz_bookmarks_itemindex
+ Index moz_bookmarks_parentindex
+ Index moz_places_hostindex
+ Index moz_places_visitcount
+ Index moz_historyvisits_fromindex
+ Index moz_historyvisits_dateindex
+ Index moz_historyvisits_placedateindex
+ Index moz_places_frecencyindex
+ Index moz_bookmarks_itemlastmodifiedindex
+ Index moz_places_lastvisitdateindex
+ Index moz_annos_placeattributeindex
+ Index moz_items_annos_itemattributeindex
+ Index moz_bookmarks_guid_uniqueindex
+ Index moz_places_guid_uniqueindex
+ Index moz_keywords_placepostdata_uniqueindex
+ Index moz_bookmarks_dateaddedindex
+ Index moz_places_originidindex
+ Index moz_places_url_hashindex
> Task: _refreshUI

Comment 5

[User Dderss]

So everything is valid, clean, vacuumed and sane but I just tried to restore bookmarks but the result is the same: 70.0b13 is insane so during the import, it enters some endless loop -- possibly due to spaces in my tags?

If there is a fix for that, can you push it down to the 70 branch (Developer Edition) to 70.0b14 or something? (It is not good to switch browsers or wait for 71/Preview just to deal with that.)

Thanks in advance.

Comment 6

[Marco Bonardo [:mak]]

(In reply to User Dderss from comment #4)

• Final database size is 430080KiB

This is not normal, did you change some preferences, like places.history.expiration.max_pages ?
the kind of bug you are seeing looks like one of the downsides of playing with that pref.

Comment 7

[User Dderss]

Yes, of course, this parameter was set to a many 9s like seven years ago or something (so the browser would actually keep history, which is crucial). But this it should not affect moz_bookmarks table since it is not constrained by any foreign keys to moz_historyvisits or moz_places which the setting is about.

Comment 8

[User Dderss]

(The size of bookmarks JSON (unpacked) is about 3.5 MB, which should not be an issue)

Comment 9

[User Dderss]

I have a theory (maybe it is wrong, please correct me):

This profile of mine was created this past winter, and its Places.sqlite database has pragma page size equal to 4096.

A newer test profile has pragma page size equal to 32768, and just now I was able to import the bookmarks there from the same file that explodes my main profile. Pragma max page count is the same for both and equals to 1073741823 (all other pragma values for both databases are the same, too; except for page size, as mentioned earlier).

So I tried to fix it: DB Browser for SQLite does nominally allow changing pragma page size value but it is a "stubborn" parameter that it is not easy to change for an existing database (https://www.sqlite.org/pragma.html#pragma_page_size), so I was not able to make it 32768 for my older main profile's Places.sqlite database.

How can I fix it? Or if it is impossible, what is the best way to move all of the history visits and places to a new database which will have pragma file page equal to 32768 since the very beginning? Then maybe restoring/importing bookmarks will work fine just as in my test profile.

Thanks in advance.

Comment 10

[Marco Bonardo [:mak]]

The size of the database affects every operation, and particularly memory usage.
That pragma page_size you are looking at has nothing to do with places.history.expiration.max_pages, they are totally unrelated, and changing the page_size won't help you in any way.
The problem is that you removed the automatic history limit that we set, by setting max_pages to a very large value. Due to that the database can grow unbound, and because it's very large, every operation will take more time and consume more memory. The about:config warning clearly says that modifying settings can have unexpected consequences, that we may not be able to handle.

We can surely look into improving memory usage for this case, but it is an unexpected situation due to settings, thus it's not the kind of usage we optimize for. I'll set it the priority as P5, we will surely accept community patches, but for now we can't dedicate developers to this.

The only fix I can suggest is to lower the places.history.expiration.max_pages value by quite a lot (something around 100k is usual, 200k or 300k may work), or remove it completely and let Firefox manage history expiration. With time the db will go back to a manageable size, and memory usage of large operations will reduce.

Comment 11

[User Dderss]

Thanks but the new description is INCORRECT: it is clearly not just "a lot of memory" but a quick and deadly uncontrolled memory leak which occupies all of the memory, even all of Windows page file until the whole OS is dead, not just FF. And, indeed, if you think about it, there is nothing "large" in the operation of importing/restoring 3.5 MB (uncompressed) worth of bookmarks.

By now I managed to solve the issue by converting JSON into HTML format and then importing it via "Import Bookmarks from HTML..." where there is no memory leak. There was a different crash though (FF has died): https://crash-stats.mozilla.org/report/index/32b59f14-af63-4e70-8254-689980191011 [@ mozilla::dom::PlacesObservers::NotifyListeners].

I had to do the import one more time before it was fully completed so in the end, I have got many bookmarks added thrice. I used DELETE FROM moz_bookmarks WHERE rowid NOT IN (SELECT MIN(rowid) FROM moz_bookmarks GROUP BY fk, position, title); SQL command to get rid of the redundancy in the table.

Thus I suggest to:

1. fix the memory leak in the "Restore"

2. fix NotifyListerers-related crash in "Import Bookmarks from HTML..."

3. make import a transaction so there would be no way to generate redundancy due to half-failed import attempts: only if the whole file is processed successfully any changes should be written to the Places.SQLite database (it may be already this way but due to bug in #2 it fails); or at least automatically run SQL command I suggested above

4. move away from a fear of "large" SQLite files. SQLite rungs databases with giant tables with dozens of millions of rows and crazy gigabytes of overall size, so there is no excuse to write its processing in such an unreliable way that any and all things can bring it down.

5. add to "Verify Integrity" processing in "about:support" any indication that it has started after the user has clicked on it. Currently, nothing is happening in the page until the section with a log appears sometime later.

My current impression is those issues are not anywhere "P1" priority, you are right, but overall, even to this day, Places continues to be most unreliable and yet most crucial to the user's data thing alongside SessionStorage (which e.g. still loses favicons no matter what folks are doing to prevent that). Let's hope that its architecture will improve so the user's data would never be lost due to synching glitches or anything else. And yes, FF has to be able to keep history for however long user wants not for however long FF's obscure internals unpredictably want to.

Comment 12

[Marco Bonardo [:mak]]

(In reply to User Dderss from comment #11)

Thanks but the new description is INCORRECT: it is clearly not just "a lot of memory" but a quick and deadly uncontrolled memory leak which occupies all of the memory, even all of Windows page file until the whole OS is dead, not just FF. And, indeed, if you think about it, there is nothing "large" in the operation of importing/restoring 3.5 MB (uncompressed) worth of bookmarks.

For now it's just a large memory use, that could mean very large, there is nothing that points to a "leak" case for now. When evidence will appear of a leak, then we can update accordingly. It doesn't change much from the bug's point of view.

My current impression is those issues are not anywhere "P1" priority, you are right, but overall, even to this day, Places continues to be most unreliable and yet most crucial to the user's data thing

It's also the largest store in the profile, and the most visible one in the UI (everyone notices when bookmarks or history break, not many will notice a cookie or HSTS annotation disappearing...), and the one that users play most with (running raw queries, modifying working conditions...). So it's pathologic that is reported a lot more by users.
We dedicated a lot of time into making the system more reliable, and we'll keep doing, but it's clearly expected to work inside specs. Your database is 6 times larger than expected, it's working very far from the conditions that are considered good.

Note that HTML files may not contain all the info of JSON backups, they are intended for another use (exchange with other software).

Comment 13

[User Dderss]

Thanks again but those specifications are not declared anywhere for users properly.

I would recommend stating all limitations publicly and transparently before lawyers in the USA or EU officials will cause trouble for Mozilla when executing consumer protection policies. The fine print in general EULA does not help as the latest cases show. The statements of yours and of your colleagues will work against the company.

For example, FireFox should never say "Keep history" in the settings if, in reality, it can randomly erase parts of it; the consumers should not be deceived.

Comment 14

[Marco Bonardo [:mak]]

I disagree with your opinion.
about:config shows a warning the first time it's opened, and until you set a checkbox, explaining dangers. It also has a background explaining that changes can compromise security and performance. The pref is not even exposed, the user must add it on purpose. Firefox is also a free product, choosen and installed by the user. I don't think anyone would ignore these facts.
No browser in the world retains all history, most browsers are limited to 90 days, Firefox is a nice exception in this. History is a very generic term used by all the software in the world, and no software has infinite history, for obvious technical reasons.