686079 - Add memory reporting for bfcache

Status: RESOLVED DUPLICATE of bug 713799

(Core :: DOM: Navigation, defect)

Description

[Alon Zakai (:azakai)]

Attachment: Report #items in bfcache in about:memory

In bug 685758 for example a leak is suspected, when it is just a (big) page kept in the bfcache. If we report more bfcache data that might prevent such misunderstandings.

Attached is a wip patch to list the number of bfcache entries in about:memory. But we can also do more: We should be able to mark compartments that are in the bfcache as such, and/or list not just the # of bfcache entries but the total memory kept in them.

Thoughts?

Comment 1

[Justin Lebar (not reading bugmail)]

Comment on attachment 559644 [details] [diff] [review]
Report #items in bfcache in about:memory

> +class bfcacheMemoryReporter :
> +    public nsIMemoryReporter

NS_MEMORY_REPORTER_IMPLEMENT in nsIMemoryReporter.idl helps you avoid some boilerplate.  There are a bunch of examples of use in nsMemoryReporterManager.cpp.

> diff --git a/docshell/shistory/src/nsSHistory.h b/docshell/shistory/src/nsSHistory.h
> --- a/docshell/shistory/src/nsSHistory.h
> +++ b/docshell/shistory/src/nsSHistory.h
> +namespace {
> +class TransactionAndDistance;
> +}

Does this do what you want?  Does this place TransactionAndDistance in a namespace owned by the header, or in a namespace owned by whatever cpp file the header ends up being included in?

> Thoughts?

If a compartment is both in and out of bfcache (say, Google +1), I imagine you'd want to mark it as not in bfcache.

Comment 2

[Alon Zakai (:azakai)]

(In reply to Justin Lebar [:jlebar] from comment #1)
> Comment on attachment 559644 [details] [diff] [review]
> Report #items in bfcache in about:memory
> 
> > +class bfcacheMemoryReporter :
> > +    public nsIMemoryReporter
> 
> NS_MEMORY_REPORTER_IMPLEMENT in nsIMemoryReporter.idl helps you avoid some
> boilerplate.  There are a bunch of examples of use in
> nsMemoryReporterManager.cpp.

Thanks, will use that.

> 
> > diff --git a/docshell/shistory/src/nsSHistory.h b/docshell/shistory/src/nsSHistory.h
> > --- a/docshell/shistory/src/nsSHistory.h
> > +++ b/docshell/shistory/src/nsSHistory.h
> > +namespace {
> > +class TransactionAndDistance;
> > +}
> 
> Does this do what you want?  Does this place TransactionAndDistance in a
> namespace owned by the header, or in a namespace owned by whatever cpp file
> the header ends up being included in?

This was just a quick hack to get gcc to stop complaining ;) I just wanted to get something working first. I'll write a proper patch soon.

> 
> > Thoughts?
> 
> If a compartment is both in and out of bfcache (say, Google +1), I imagine
> you'd want to mark it as not in bfcache.

Good point, makes sense.

Comment 3

[Alon Zakai (:azakai)]

Hmm, this is not as easy as I hoped. I tried to set things up so that I can ask the session history about each compartment whether it is in the bfcache, with the plan of comparing each compartment to the compartments of pages in the bfcache. However, it looks like when pages go into the bfcache they clear a lot of information, including their script global objects and their window and inner window. So far I haven't been able to figure out how to get at the compartment of a page in the bfcache.

Comment 4

[Boris Zbarsky [:bzbarsky]]

What do you mean by "a page"?  What object are you actually starting on?

The inner window is stored in the bfcache as well, so we really do have all the info we need; all we need is to dig it out.  But where are we starting?

Comment 5

[Alon Zakai (:azakai)]

I am using the list of TransactionAndDistance elements in nsSHistory, to see the pages in the bfcache,

https://mxr.mozilla.org/mozilla-central/source/docshell/shistory/src/nsSHistory.cpp#1010

From each transaction I get a content viewer, then a document from that, but the document appears to have everything we need nulled out (script global object, window, inner window, etc.). The data must be in there somewhere since it is restored when the page is browsed back to, seems to be hidden fairly well though ;)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  So yeah, the SHEntry has a content viewer that you can get the document from.  It also has a window state which claims to be just nsISupports but is actually a WindowStateHolder (something we should consider fixing now that docshell is in libxul along with window stuff) and then you can get all sorts of stuff from it as needed.  Might need to move WindowStateHolder into a header somewhere, of course.

Comment 7

[Alon Zakai (:azakai)]

Thanks! Now it makes sense.

Comment 8

[Alon Zakai (:azakai)]

Attachment: wip patch

Ok, this basically works. It reports the # of pages in the bfcache, and also adds [bfcache] to any compartment's name when it is in the bfcache. Note that it will report that even if part of the compartment is out of the bfcache, I'm not sure how feasible it is to detect that - we would need to check all other open pages, as well as addons and possibly other stuff.

I'll refine the code some more, but I could use some feedback about the interfaces, header changes and so forth, I'm not sure if I'm doing the right things there.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 561239 [details] [diff] [review]
wip patch

> Note that it will report that even if part of the compartment is out of the
> bfcache,

This won't be a problem once we have compartment per global.

The overall approach looks ok to me, though I still think we should move toward storing a WindowStateHolder, not an nsISupports in the history.

Should we mark presshells that are in bfcache too?

Comment 10

[Alon Zakai (:azakai)]

(In reply to Boris Zbarsky (:bz) from comment #9)
> Comment on attachment 561239 [details] [diff] [review]
> wip patch
> 
> > Note that it will report that even if part of the compartment is out of the
> > bfcache,
> 
> This won't be a problem once we have compartment per global.
> 

Ok, sounds good.

> The overall approach looks ok to me, though I still think we should move
> toward storing a WindowStateHolder, not an nsISupports in the history.
> 

Makes sense. I don't see an existing bug for this so filed bug 688002. Will try to get around to it soon.

> Should we mark presshells that are in bfcache too?

That would be useful I think, yeah. I'll add that.

Comment 11

[Alon Zakai (:azakai)]

The PresShell stuff has led me into some problems:

1. When a page is in the bfcache, I can't see a way to find its compartment. The PresShell has a document, but the document doesn't have a window (due to being in the bfcache I believe). The PresShell has GetRootWindow but that isn't the right window for us either.

2. The approach I took in the patch so far doesn't look like it will work for iframes. Currently I get a compartment, then compare it to the compartments of pages in the bfcache. However, pages in iframes in the bfcache can have different compartments. Should I be iterating over all iframes somehow? Or is there some direct way to find all the compartments in a page and everything on it?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> When a page is in the bfcache, I can't see a way to find its compartment

Why do you need that?  We don't report layout memory per-compartment right now; we report it per-URI.  So when constructing the memory reporter key in PresShell::MemoryReporter::SizeEnumerator, just add "[bfcache]" to the key if that _particular_ presshell is in the bfcache.  Which the presshell knows about itself.  That will give us entries per-URI for loaded stuff and separate entries per-URI for bfcached stuff, which seems fine.

Or am I missing something?

Comment 13

[Alon Zakai (:azakai)]

Ah, I forgot that the PresShell knows that about itself. mFrozen is the right thing to use, I guess?

I'm still worried about point 2 above, though, not for PresShells but for compartments. When I go over the compartments and check if they are in the bfcache, currently I look if they are a compartment used by a page in the bfcache, by enumerating the pages in the bfcache and comparing them. But I am not sure if that will be true if they are just an iframe inside a page in the bfcache, since the enumerated pages are just the toplevel ones I suspect. (I noticed this in passing when doing other stuff and didn't investigate fully yet.) Seems like I should also be checking if their parent page (in which they are an iframe) is in the bfcache and so forth. Does that make sense?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> mFrozen is the right thing to use, I guess?

Yes.

> Seems like I should also be checking if their parent page (in which they are an iframe)
> is in the bfcache and so forth.

Yeah, agreed.

Comment 15

[Nicholas Nethercote [inactive]]

azakai, it sounds like bz addressed your concerns.  Is this bug in a state where you can move it forward?  dmandelin expressed interest today in better understanding the effect of the bfcache on memory usage.

Comment 16

[Alon Zakai (:azakai)]

I've been busy with other things recently, have not had much time for this bug I am afraid. But what I have working so far is detection of PresShells, and of JS compartments of toplevel windows. What I have not been able to get working is detection of JS compartments of internal windows (inside iframes).

I have heard the compartments situation will be simplified at some point, to one compartment per tab or something like that? If so, it would make this bug much simpler (and basically finished).

Comment 17

[Nicholas Nethercote [inactive]]

Compartment-per-global (bug 650353).

Comment 18

[Nicholas Nethercote [inactive]]

I think we can mark this as fixed or maybe a dup -- jst added identification of windows as active/cached/other, and bug 713799 and bug 687724 cover the rest of it.

azakai, do you agree?

Comment 19

[Alon Zakai (:azakai)]

Looks like those bugs cover this or will cover it, yeah.

Sorry I wasn't able to finish this, the iframes JS bit was more complicated than I thought. Hopefully with compartment per global it will eventually become easy to do.

Comment 20

[Nicholas Nethercote [inactive]]

> Hopefully with compartment per global it will eventually
> become easy to do.

That's the plan! :)