If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Places creates some nsNavHistoryResult garbage

NEW
Unassigned

Status

()

Toolkit
Places
P5
normal
6 years ago
6 months ago

People

(Reporter: smaug, Unassigned)

Tracking

12 Branch
x86_64
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

6 years ago
I wonder if places could somehow manually destroy nsNavHistory* objects without
need for cycle collector. Or perhaps we could mark nsNavHistory* objects
black so that they wouldn't really participate cycle collection.

Note, nsNavHistory* objects show up in CC logs only occasionally.

http://mozilla.pettay.fi/xul.svg is an example. Middle-bottom part is
nsNavHistory* related. (you can hover over nodes to see what they are.)
Results can be used in many parts of the ui and also internally in the backend, plus they are also strong observers to bookmarks/history. So that's why they participate in cycle collection, in the past there were many leaks due to incomplete collection, so removing it may reintroduce those leaks. Afaik (though I was not here yer) we were strongly invited to participate to cycle collection, since the opposite would have not been approved for landing.

When a result root node is closed we already cleanup some of the resources manually to free up memory without relying on GC/CC. So, we may cleanup more. Though in some case the root node is never closed (bookmarks menu and toolbar are the most evident cases) till shutdown.

Is this graph generated from current Nightly? What problem are we trying to solve?
(Reporter)

Comment 2

6 years ago
Graph is generated from a Nightly which has some CC handling related patches.

The problem to solve is to shrink CC graph size. It is usually huge graphs which make CC slow.

I was just hoping that there could be some easy way to know whether nsNavHistory* objects
are certainly alive (black), and in such case certain optimizations could be done so that those 
objects wouldn't even be added to the graph.
There's some existing code for checking if an nsDocument is being displayed or not, so if a nsNavHistory* has a reference to some other object foo that
a) strongly holds onto the nsNavHistory*
b) can be checked if it is in the document or not
...then we can skip the nsNavHistory* in the probably common case where it is associated with a currently-displayed window.

In other words, foo will necessarily keep the nsNavHistory alive, and the document will keep foo alive, and we can get from nsNavHistory to the document in a couple of steps, then check if it is alive or not.

I don't know if that will work here or not, but this is a general pattern that is used to avoid adding things to the CC.
A result may even be used by another service to have a live-updating view of bookmarks, not just by a document. Tagging service was doing that, for example, and also having another observer to bookmarks. It's not doing that anymore in Nightly though.
Btw, probably would be safer, as a first step, to cleanup up manually some local reference when the root node is closed, simplifying the graph that way. Actually we already do some of that at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistoryResult.cpp#699 that may be a good hook point for further work.

Updated

6 years ago
Depends on: 730340

Updated

6 months ago
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.