Insanely deep bookmarks tree crashes the browser
Categories
(Firefox :: Sync, defect, P2)
Tracking
()
People
(Reporter: 29sxiao, Assigned: lina)
Details
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:129.0) Gecko/20100101 Firefox/129.0
Steps to reproduce:
- Sign in to Firefox Sync
- Wait for sync to finish
Actual results:
Entire browser crashes, even in troubleshoot mode. I believe this https://crash-stats.mozilla.org/report/index/ff99f648-78ab-4cad-b1f5-7aaa70240902 crash report is caused by this issue.
Expected results:
Sync to complete and not crash
|
||
Comment 1•7 months ago
|
||
The Bugbug bot thinks this bug should belong to the 'Firefox::Sync' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.
|
||
Comment 2•7 months ago
|
||
Interesting, a stack overflow from dogear - ni lina for any insights.
|
Assignee | |
Comment 3•7 months ago
|
||
Oooh, I haven't seen this one before! I think this is the culprit—when we want to figure out if an item is syncable or not, we recurse upwards through the tree, to check if an item is a descendant of one of the user content roots.
We do have a detect_cycles step, that errors out if we try to build a tree with a folder containing itself as a descendant. I'm puzzled why that didn't work here—the fact that we reached the point where dogear::tree::Node::is_built_in_root is called means we did build a tree, and we didn't think it contained cycles.
:29sxiao, if you're comfortable with it, would you be OK with sending me your places.sqlite and weave/bookmarks.sqlite files from your profile (you can open your profile directory via about:profiles; it's the "Root Directory" for the profile in use) via email?
These two files contain all your bookmarks and history, so it's completely alright if the answer is "no". But it would be the easiest way for me to debug this, and I promise that I'll only use the files to reproduce and fix the issue, I won't share them with anyone else, and I'll securely delete them from my laptop after.
Thank you either way!
Sorry I couldn't respond earlier, I was out of town.
I believe that I figured out why sync was crashing the browser. I once made a nested bookmarks folder 1936 levels deep for fun (with no actual bookmarks), and deleting that folder seems to have fixed the issue, although I'm not sure why just 1936 recursion levels would cause a recursion overflow.
|
|
Assignee | |
Comment 5•6 months ago
|
||
Oh, that's very helpful, thanks! (And that's an impressively deep tree—just for my own curiosity, can I ask if you created it by hand, or used a script or an extension? 😊)
When we merge bookmarks, we recurse down into all folders to look for changes—and, when we look at each item, we recurse back up the tree to figure out of that item should be synced or not—to catch livemarks, which we don't support anymore; and virtual items, which shouldn't have been synced at all—for both the local and remote trees.
I'm a little amused that just 1936 levels was enough to overflow the stack, but...the merger runs on a secondary thread, and does that check repeatedly, for every item, at every level, for both the remote and local trees, while keep all nodes in both trees on the stack, so I'm not too surprised.
Sync's merger doesn't cache the syncability of each item (yet! 😅), because it assumes that most bookmark trees are shallow. Even if that's true for most users, though, we definitely don't want to crash with a stack overflow error for deep trees, and caching the syncability would be a quick win here.
I'll go ahead and snag this bug, and put up a patch when I have some cycles—thanks so much for following up!
Alright, appreciate your work, thank you.
ps. I did it by hand, but it wasn't too hard - I just copy-pasted a tree into into itself a few times.
|
|
||
Updated•6 months ago
|
Description
•