714689 - Places creates some nsNavHistoryResult garbage

Status: RESOLVED INACTIVE

(Toolkit :: Places, defect, P5)

Description

[Olli Pettay [:smaug][[e-mail]]]

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.)

Comment 1

[Marco Bonardo [:mak]]

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?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.