268513 - overflow:scroll causes memory growth

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Kyle Yuan]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041104

I'll attach a testcase soon. In that test case, I use java script generate a
large table (5000 cells) and put it in a <div> which have overflow:scroll set in
its style, then I insert this div into another <div> using innerHTML property. I
repeatly doing this every 3 sec and I noticed that the previous data of table is
never freed until I leave that page. On Windows, mozilla's memory usage has
grown from 36M to 86M after 20 iteration. It also happens on all other platform.

Comment 1

[Kyle Yuan]

Attachment: test case

Comment 2

[Kyle Yuan]

Attachment: make-tree.pl output

This log shows that nsCSSFrameConstructor::BuildScrollFrame is hold 3
additional references to the nsHTMLDivElement which is generated by js and
inserted into another <div>.

Comment 3

[Kyle Yuan]

bz, roc, any advise?

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

At a very quick glance, it looks more like once reference from an nsHashtable
and two from XPConnect.  There's probably a bunch of other stuff leaking, so
there's nowhere near sufficient information in that log alone.

Comment 5

[Kyle Yuan]

I noticed that when create a scrollframe for div, 3 anonymous contents (2 scroll
bars, 1 square) are also created, and added into PresShell's AnonymousContent
hashtable: 
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp#5427

Those 3 contents can not be discovered by nsContentIterator, therefore they also
can not be removed via nsRange::DeleteContents().

CCing jst who fixed bug 257690 and mentioned in bug 236796 comment 12 that we
are going to "make the iterators aware of anonymous content."

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.. I would have expected the anonymous content to go away when the frame is
torn down, but it doesn't look like it does.  Perhaps we need to
SetAnonymousContentFor(nsnull) on the content node when the primary frame is
destroyed?

That said, anonymous content is cleaned up on presshell destruction.  So while
that may be the cause for the runtime memory growth it should not show up in
shutdown leaks...  So David is right; if things are leaking on shutdown you need
to find the root leaks.

It sounds like we probably have two separate bugs here...

Comment 7

[Kyle Yuan]

There is no leak on shutdown in terms of HTMLDivElement. The above reference log
was taken at run time.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  In that case, chances are you're right.  Teaching iterators about
native anonymous content wouldn't really be sufficient, since nodes can be
removed from trees by other means.  I think the right solution is to make
teardown of the primary frame tear down the anonymous content involved...

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK, I tried hacking SetPrimaryFrameFor() to clear anonymous content when it
clears the primary frame.

That didn't help.  I also tried opening a new window and closing the one with
the testcase.  That also didn't reduce memory usage, even though that definitely
clears out the anonymous content table.

Comment 10

[Kyle Yuan]

I created a new method PresShell::ReleaseAnonymousContentFor(nsIContent*) that
does the same thing as SetPrimaryFrameFor()(nsnull) does and removes the content
itself from the HashTable. That successfully released 1 reference, but I have no
idea how to remove the other 2 hold by XPConnect.

Comment 11

[Kyle Yuan]

This patch makes XPConnect(WrapNative) add reference to the document, rather
than the div node
(http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsDOMClassInfo.cpp#4973).
But I'm not sure whether it will cause any problem?

RCS file: /cvsroot/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,v
retrieving revision 1.1013
diff -u -p -r1.1013 nsCSSFrameConstructor.cpp
--- nsCSSFrameConstructor.cpp   6 Nov 2004 07:07:43 -0000       1.1013
+++ nsCSSFrameConstructor.cpp   10 Nov 2004 10:49:23 -0000
@@ -5433,7 +5433,6 @@ nsCSSFrameConstructor::CreateAnonymousFr
         continue;

       content->SetNativeAnonymous(PR_TRUE);
-      content->SetParent(aParent);
       content->SetDocument(aDocument, PR_TRUE, PR_TRUE);

       nsresult rv;
@@ -5471,6 +5470,7 @@ nsCSSFrameConstructor::CreateAnonymousFr
         // create the frame and attach it to our frame
         ConstructFrame(aPresShell, aPresContext, aState, content, aParentFrame,
aChildItems);
       }
+      content->SetParent(aParent);

       creator->PostCreateFrames();
     }

Comment 12

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #10)
> I created a new method PresShell::ReleaseAnonymousContentFor(nsIContent*)

That's what SetAnonymousContentFor(aContent, nsnull) does, no?

> This patch makes XPConnect(WrapNative) add reference to the document

So in this case the <div> is the _parent_?  What's the child?  It's clearly a
XUL element if the classinfo code you cite is relevant... Why is this going
through XPConnect at all?  (There's the side question of why it's not being torn
down properly, but....)

Comment 13

[Kyle Yuan]

(In reply to comment #12)
> That's what SetAnonymousContentFor(aContent, nsnull) does, no?

Not exactly, SetAnonymousContentFor(aContent, nsnull) only clear the anonymous
content for aContent, but it doesn't remove aContent itself from the HashTable
(mAnonymousContentTable) which holding a reference to it.

> 
> So in this case the <div> is the _parent_?  What's the child?  It's clearly a
> XUL element if the classinfo code you cite is relevant... Why is this going
> through XPConnect at all?  (There's the side question of why it's not being torn
> down properly, but....)

In the current code, yes, <div> is the parent, its children are 3 XUL element
created in nsCSSFrameConstructor::CreateAnonymousFrame - two scrollbars and one
something else at the right-bottom between those two scrollbars. Seems only
scrollbars added reference to <div>. I have no idea why this is going through
XPConnect.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> but it doesn't remove aContent itself from the HashTable

Ah, true.  That's a bug and should be fixed in SetAnonymousContentFor().  Good
catch!.

The thing in the right-bottom is the scrollcorner.

And I bet the scrollbar XBL bindings are what's causing XPConnect to be
involved.  The constructor firing would cause an XPConnect wrapper to be created
for |this| (that being the scrollbar) and that would use the parent (the div)
per that classinfo code you cited.

So why does it use the parent, exactly?  And why is this stuff not cleaning
itself up as it should?  jst?  dbradley?  Any idea?

Comment 15

[Johnny Stenback (:jst)]

Any XUL elements we use the parent object as the scope for the JS wrapper, which
means if the XUL elements that makes up the scrollbar ends up being a child of a
HTML div, the wrapper for the XUL element in the scrollbar will keep the div
alive as long as the XUL element is alive... If things go away when you leave
the page (or shortly there after, wait at least 2 seconds), then everything is
cleaned up properly, we're just not GC'ing often enough in this case, it seems...

Comment 16

[Boris Zbarsky [:bzbarsky]]

> If things go away when you leave the page (or shortly there after, wait at
> least 2 seconds)

In my testing, they don't seem to.  At least the memory usage does not drop.  I
opened and closed several windows and loaded about:mozilla, about:, etc several
times to test.

Comment 17

[Kyle Yuan]

jst, is there any problem if I use the document as the scope for the JS wrapper?
That makes more sense to me because those JS wrappers only go away when the
document was gone.

Comment 18

[Kyle Yuan]

Attachment: first try

I'm sure that nsHTMLScrollFrame::Destroy() is not the correct place to put the
fix, but anyway, just want to make sure I'm on the right way.

Comment 19

[Kyle Yuan]

Comment on attachment 166081 [details] [diff] [review]
first try

bz, jst, any comment?

Comment 20

[Boris Zbarsky [:bzbarsky]]

I think the right place to remove the anonymous content is where we set the
primary frame to null...

Comment 21

[Kyle Yuan]

(In reply to comment #20)
> I think the right place to remove the anonymous content is where we set the
> primary frame to null...

Can you be more specific? where do we set the primary frame to null, exactly?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Well, the most obvious place is inside SetPrimaryFrameFor...

But I just realized that it's possible for a frame to get created, create its
anonymous content, and get destroyed all without being set as the primary frame
(eg in some sort of {ib} situation or whatnot).  So what we _really_ want to do
is to clear anonymous content when an nsIAnonymousContentCreator frame dies. 
But I'm not sure how that fits with XTF, and I'm also not sure whether it's
possible to have multiple nsIAnonymousContentCreator frames per single content
node (the SetAnonymousContentFor code sure seems to support that).

In brief, we need to document what our invariants are here, at the very least.  :(

Comment 23

[Kyle Yuan]

Attachment: v0.2

The change in nsPresShell.cpp is to a) release 1 reference count of
XPCWrappedNative object so that it can be GCed later; b) clear the content from
|mAnonymousContentTable| hash table.

The change in nsFrame.cpp is to call PresShell::SetAnonymousContentFor when a
nsIAnonymousContentCreator-derived frame is going to be destroyed.

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 169358 [details] [diff] [review]
v0.2

- In nsFrame::Destroy():

+    nsCOMPtr<nsIAnonymousContentCreator> isAnonymousContentCreator =
do_QueryInterface(NS_STATIC_CAST(nsIFrame*, this));
+    if (isAnonymousContentCreator) {
+      shell->SetAnonymousContentFor(mContent, nsnull);
+    }

It'd be cool if there was a cheaper way to do this than a QI to
nsIAnonymousContentCreator. Maybe we need to add a destructor to that interface
and put this code there?

dbaron should have a look, but r=jst either way.

Comment 25

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 169358 [details] [diff] [review]
v0.2

>Index: layout/base/nsPresShell.cpp
>+        nsCOMPtr<nsISupports> old_ref = mDocument->RemoveReference(content);

I don't understand why this belongs here.  Why should this happen only for
anonymous content?  The only other time we call RemoveReference is when moving
content between documents via insertBefore or replaceChild.

>+  {
>+    nsCOMPtr<nsIAnonymousContentCreator> isAnonymousContentCreator = do_QueryInterface(NS_STATIC_CAST(nsIFrame*, this));

No need for nsCOMPtr.  Just use a raw pointer, e.g.:

nsIAnonymousContentCreator *creator;
if (NS_SUCCEEDED(CallQueryInterface(mContent, &creator)) 
  ...

(And don't call a pointer variable |is...|, even if you're just null-checking
it.)

Comment 26

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 169358 [details] [diff] [review]
v0.2

I meant this rather than mContent.

But also, nsIAnonymousContentCreator isn't the only source of anonymous content
maintained in the pres shell -- :before and :after content are also tracked. 
Shouldn't we clear that too?  Maybe it should just happen in
nsPresShell::NotifyDestroyingFrame, with no nsIAnonymousContentCreator check?

Comment 27

[Kyle Yuan]

(In reply to comment #25)
> (From update of attachment 169358 [details] [diff] [review] [edit])
> >Index: layout/base/nsPresShell.cpp
> >+        nsCOMPtr<nsISupports> old_ref = mDocument->RemoveReference(content);
> 
> I don't understand why this belongs here.  Why should this happen only for
> anonymous content?  The only other time we call RemoveReference is when moving
> content between documents via insertBefore or replaceChild.

This is because when an anonymous content created in
nsCSSFrameConstructor::CreateAnonymousFrames(), if it's a xpconnect wrapped
content, nsDocument::AddReference will be called for adding both the anonymous
content and its XPCWrappedNative object to the document. (see
http://lxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLProtoImpl.cpp#137,
the full code path can be found in attachment 165249 [details]) 

Currently, we only release those XPCWrappedNative objects in nsDocument's dtor
(through mContentWrapperHash's dtor). Here, the explicit call of RemoveReference
  ensures releasing one reference count of XPCWrappedNative so that it can be
GCed later instead of living in memory until the document destroyed.

Comment 28

[Kyle Yuan]

Attachment: v0.3

Moved SetAnonymousContentFor from nsFrame::Destroy to
PresShell::NotifyDestroyingFrame despite whether the frame is an anonymous
content creator or not. (Is there any performance impact for doing this?)

Comment 29

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

comment 27 doesn't answer the question:  why should this happen *only* for
anonymous content?

Comment 30

[Kyle Yuan]

nsDocument::AddReference is called when
nsCSSFrameConstructor::CreateAnonymousFrames() is generating the anonymous
content. I'm just trying to do the reverse thing. There are probably some other
place called nsDocument::AddReference, but they are not releated to this memory
growth.

Comment 31

[Johnny Stenback (:jst)]

As for calling mDocument->RemoveReference() for anonymous content when its
frames are destroyed that makes sense to me. The only reason for the document
holding a reference to the node here is to prevent it from being garbage
collected when all references to it are lost, and here the frame is gone, and
this node won't be reachable in any way any more unless code already has
references to it, in which case it won't be garbage collected etc etc. So that
part seems fine with me.

Comment 32

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 169622 [details] [diff] [review]
v0.3

ok, r=dbaron (or sr).  I understand nsDocument::RemoveReference now (and it's
evil, and needs to be removed eventually), but this will fix a leak in the case
where we can fix it without breaking anything (which really isn't the best
place, but it's the best we can do without breaking stuff).

Comment 33

[Kyle Yuan]

taking, target on 1.9 alpha.

Comment 34

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

This probably could go in for 1.8beta2.

Comment 35

[Kyle Yuan]

that will be great.

Comment 36

[Johnny Stenback (:jst)]

Comment on attachment 169622 [details] [diff] [review]
v0.3

sr=jst

Comment 37

[Daniel Veditz [:dveditz]]

Comment on attachment 169622 [details] [diff] [review]
v0.3

a-, too late.

Comment 38

[Kyle Yuan]

fixed in 1.8 beta2.

Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.813; previous revision: 3.812

Comment 39

[Martijn Wargers (dead)]

I think this fix has caused bug 283611.

Comment 40

[Mats Palmgren (inactive)]

It also caused bug 283863

Comment 41

[Mats Palmgren (inactive)]

It also caused bug 283788.

Comment 42

[Boris Zbarsky [:bzbarsky]]

This also caused bug 283997.  See bug 283611 comment 1 for my guess as to what's
wrong with this patch...

Comment 43

[Kyle Yuan]

I've backed out the fix.

Comment 44

[Rick Stockton]

My bug 284041, which I described nearly the same as 283863, is STILL PRESENT on
today's Trunk Build (Linux, GTK1, Classic Theme).

Only difference between my bug and 283863 is that I can successfully make out
the edges of the scrollbar--- barely. And I've got a really good monitor. 

Comment 45

[Worcester12345]

I tried to add "mlk" keyword, but it would not let me. Can someone else add this?

Comment 46

[Boris Zbarsky [:bzbarsky]]

It may be that one "right" place to clear the anonymous content is when the node
is removed from the document (see nsBindingManager::ChangeDocumentFor).  The
patch in bug 286000 may well end up doing that if it hasn't happened by that time.

That would fix the testcase in this bug, I suspect...

Comment 47

[Boris Zbarsky [:bzbarsky]]

So with the patch for bug 286000 in, do we still get memory growth on the
testcase with just the PresShell::SetAnonymousContentFor part of the patch for
this bug?  If not, I think checking that in is the way to go...

Comment 48

[Kyle Yuan]

bz, I'll check it out today.

Comment 49

[Kyle Yuan]

Attachment: with 286000 fixed, only need 1-line to fix the memory growth

Comment 50

[Kyle Yuan]

Comment on attachment 179932 [details] [diff] [review]
with 286000 fixed, only need 1-line to fix the memory growth

oops, this will cause a crash due to removing an item during enumeration of
hashtable.

Comment 51

[Kyle Yuan]

Attachment: a few more code to prevent the crash

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 179935 [details] [diff] [review]
a few more code to prevent the crash

I guess this works for now, though we should really just eliminate
ReleaseAnonymousContent() as an api method, and then we can just use the
mIsDestroying member for this check....

Comment 53

[Asa Dotzler [:asa]]

Comment on attachment 179935 [details] [diff] [review]
a few more code to prevent the crash

a=asa for checkin to the 1.8b2 trunk

Comment 54

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The additional change to prevent a crash seems wrong, in that it fixes the crash
by leaking in the case where it fixes the crash.  Wouldn't a better thing be to
ensure that the releases happen after the Remove by holding a reference?

Comment 55

[Kyle Yuan]

David, we do not need to call mAnonymousContentTable->Remove during
PresShell::ReleaseAnonymousContent, because the destruction of
mAnonymousContentTable will remove all keys anyway.

I just landed the patch without seeing your comment. Please let me know if the
patch needs further change.

Comment 56

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Sorry, never mind.  I was thinking you were doing all the boolean twiddling for
a different bug.