964187 - Removed DOM images still stay in mVisibleImages cache

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jan Horak [:jhorak]]

We're experimencing growing DOM orphan nodes in an internal web app only in Firefox 24 ESR. In firefox 26 it seems to be fixed. 

I've been trying to find when this issue has been fixed and I've ended in this change: http://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=64e01f43e027
Any hint what commit could fix this issue?

Comment 1

[Jan Horak [:jhorak]]

It is most likely fixed by bug 847223.

Comment 2

[Nicholas Nethercote [inactive]]

> It is most likely fixed by bug 847223.

Really? That bug was about image decoding. AFAIK it had nothing to do with DOM nodes.

More generally: even if we work out which revision fixed the patch, my understanding is that ESR releases only get security fixes backported to them, and this seems unlikely to meet that criteria. So I think this bug is unlikely to lead to any action.

Comment 3

[Andrew McCreight [:mccr8]]

They also take stability fixes, which usually means crashes, but perhaps a liberal interpretation would include particularly bad memory problems.

Nothing in that range particularly jumps out at me.  As Nick said, it would be a little surprising if an image decoding bug fixed an orphan node problem.  That bug is also way too risky and complex to backport, I think, so hopefully the solution lies elsewhere.

Comment 4

[Jan Horak [:jhorak]]

I've just made test builds with [1] and without [2] patches from bug 847223 and there is significant difference in memory occupied by orphan nodes. 

To reproduce the issue all I have to open the affected page and wait. In few minutes the [2] has around 30MB in orphan nodes while the [1] keeps around 11MB (approx amount of memory taken when page shows up). Unfortunately I can't make simple reproducer.

For reference, here's the downstream bug: https://bugzilla.redhat.com/show_bug.cgi?id=1036443

Comment 5

[Andrew McCreight [:mccr8]]

tn, do you have any idea why your image decoding improvements could reduce the number of DOM nodes held alive?

Comment 6

[Timothy Nikkel (:tnikkel)]

Hmm, we have a list of nsIImageLoadingContent on the presshell of "visible" images. The list holds refs to the objects. Bug 847223 turned the list from an array to a hashtable, along with other changes. Perhaps bug 847223 removed images from the visible list that we just left on the list before?

I'd definitely like to understand this more.

I could produce some patches to dump the number of items in and contents of the list before/after bug 847223 that could tell us if my hypothesis is correct. jhorak, you willing to try that?

Comment 7

[Jan Horak [:jhorak]]

(In reply to Timothy Nikkel (:tn) from comment #6)
> Hmm, we have a list of nsIImageLoadingContent on the presshell of "visible"
> images. The list holds refs to the objects. Bug 847223 turned the list from
> an array to a hashtable, along with other changes. Perhaps bug 847223
> removed images from the visible list that we just left on the list before?
> 
> I'd definitely like to understand this more.
> 
> I could produce some patches to dump the number of items in and contents of
> the list before/after bug 847223 that could tell us if my hypothesis is
> correct. jhorak, you willing to try that?

Sure, gladly.

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: printfs for image array

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: image hash table printfs

Comment 10

[Timothy Nikkel (:tnikkel)]

"printfs for image array" is for before bug 847223, "image hash table printfs" is for after bug 847223.

UpdateImageVisibility does a full update of visible images, determining which ones are visible or not considering all images. The patches print out the visible images and the number of them when it is complete.

EnsureImageInVisibleList just adds one image to the list of visible images and printfs it out (along with the current number of visible images).

RemoveImageFromVisibleList only exists after bug 847223, it removes one image from the visible list (and printfs the number in the list).

Let me know if you have any questions.

Comment 11

[Jan Horak [:jhorak]]

Thank you for patches. I've made both builds and run them simultaneously. In some time I've checked the output.

I'm only seeing 'PresShell::EnsureImageInVisibleList' messages in terminal.

Array:
PresShell::EnsureImageInVisibleList 0x7f1ccccd1800 https://pn-rhsetup.rhev.lab.eng.brq.redhat.com/webadmin/webadmin/clear.cache.gif (total visible images 5714)

Hash table:
PresShell::EnsureImageInVisibleList 0x7fdeb8214c00 https://pn-rhsetup.rhev.lab.eng.brq.redhat.com/webadmin/webadmin/clear.cache.gif (total visible images 562)

It seems there is 10x more visible images in array. Both instances had the same page opened.

I see only "https://pn-rhsetup.rhev.lab.eng.brq.redhat.com/webadmin/webadmin/clear.cache.gif" image in terminal.

What is interesting there's a table with list items and when I scroll through the items, memory is freed and I get:
UpdateImageVisibility visible images (97)
https://pn-rhsetup.rhev.lab.eng.brq.redhat.com/webadmin/webadmin/clear.cache.gif
https://pn-rhsetup.rhev.lab.eng.brq.redhat.com/webadmin/webadmin/clear.cache.gif
..........snip
https://pn-rhsetup.rhev.lab.eng.brq.redhat.com/webadmin/webadmin/clear.cache.gif
https://pn-rhsetup.rhev.lab.eng.brq.redhat.com/webadmin/webadmin/clear.cache.gif
PresShell::EnsureImageInVisibleList 0x7f1ccccd1800 https://pn-rhsetup.rhev.lab.eng.brq.redhat.com/webadmin/webadmin/clear.cache.gif (total visible images 98)

This cleanup after scrolling it might be 'feature' of web application.

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to jhorak from comment #11)
> What is interesting there's a table with list items and when I scroll
> through the items, memory is freed and I get:
> 
> This cleanup after scrolling it might be 'feature' of web application.

UpdateImageVisibility is triggered after "enough" scrolling. So that explains what you were seeing there.

So from this information is sounds like a lot of images are being dynamically added to the page that aren't visible, and then removed. If an image is dynamically inserted, and is visible, and then removed we will probably still hold on to it. So the undesirable behaviour improved, but still exists.

Comment 13

[Jan Horak [:jhorak]]

(In reply to Timothy Nikkel (:tn) from comment #12)
> (In reply to jhorak from comment #11)
> > What is interesting there's a table with list items and when I scroll
> > through the items, memory is freed and I get:
> > 
> > This cleanup after scrolling it might be 'feature' of web application.
> 
> UpdateImageVisibility is triggered after "enough" scrolling. So that
> explains what you were seeing there.
> 
> So from this information is sounds like a lot of images are being
> dynamically added to the page that aren't visible, and then removed. If an
> image is dynamically inserted, and is visible, and then removed we will
> probably still hold on to it. So the undesirable behaviour improved, but
> still exists.

By visible you mean in DOM or inside viewport/window? So we can't consider this as bug in application, right?
I can confirm that orphan nodes are rising when using has table but at much slower pace.

Comment 14

[Timothy Nikkel (:tnikkel)]

(In reply to jhorak from comment #13)
> By visible you mean in DOM or inside viewport/window?

In the DOM and in the viewport, or can be brought into the viewport with at most one screenful of scrolling.

> So we can't consider this as bug in application, right?

Firefox should deal with this better for sure. The application could change things to not hit this problem if it wanted to.

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Attachment: testcase

A testcase by Jan Horak. 

This testcase is derived from RHEV-M GUI which is compiled by GWT. The mechanism is described in https://bugzilla.redhat.com/show_bug.cgi?id=1054242#c19 but in short it uses an image place holder (clear.cache.gif) and substitutes the DOM nodes dynamically.

A workaround for it is to set layout.imagevisibility.enabled=false

An interesting thing is the onload="this.tmp="hi_dude";" image tag. When the image has some extra variable, it's marked as "orphan-nodes". Otherwise it lives in "heap-unclassified".

Comment 16

[Martin Stránský [:stransky] (ni? me)]

It can be reproduced with latest trunk, but the memory raise is slower (~300 MB in 10 minutes).

Comment 17

[Timothy Nikkel (:tnikkel)]

Attachment: patch

I think this'll fix it.

Comment 18

[Timothy Nikkel (:tnikkel)]

(In reply to Martin Stránský from comment #15)
> An interesting thing is the onload="this.tmp="hi_dude";" image
> tag. When the image has some extra variable, it's marked as "orphan-nodes".
> Otherwise it lives in "heap-unclassified".

Nick, is this how we expect orphan-nodes to be measured?

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It's an artifact of how we track down orphan-nodes.  We rely on them being accessible from the JS heap, where we can trace them from the set of roots we start at.  If there is no JS wrapper then there's no way to find them.  Since being "orphaned" from C++ requires a Gecko bug we decided this was a reasonable approach.

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 8405875 [details] [diff] [review]
patch

Why not do this in nsImageLoadingContent::FrameDestroyed instead?
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsImageLoadingContent.cpp?rev=b29352bc495f#462
We're deregistering/untracking the image there so it seems wrong
to continue to consider it "visible" without a frame.

Comment 21

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #20)
> Comment on attachment 8405875 [details] [diff] [review]
> patch
> 
> Why not do this in nsImageLoadingContent::FrameDestroyed instead?
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> nsImageLoadingContent.cpp?rev=b29352bc495f#462
> We're deregistering/untracking the image there so it seems wrong
> to continue to consider it "visible" without a frame.

That's a good point. One of my original goals was to leave images as visible on frame destroy so that if we were reconstructing a frame (eager delete, lazy re-create) we would leave the image locked and not accidentally discard the decoded image and have to re-decode it. But as soon as we lose the frame we get removed from the nsDocument image tracking which drops our lock, so we in fact never had this behaviour.

So currently our behaviour is that when we get the new frame we will be locked again (because it still has positive visible count). With your proposed patch we will have to wait until the new frame is reflowed and it's visibility is checked. I think only uncommon flushes create frames but don't do reflow. So in the important case this will usually happen during the same flush, so it shouldn't make any difference. So sounds good to me.

Comment 22

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 8406093 [details] [diff] [review]
patch v2

Thanks for the explanation.

PresShell() asserts it's non-null.  I think you want GetPresShell()
here since you null-check the result.

r=mats with that

Comment 24

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1accd58e429e
and review comment, oops forgot the first time
https://hg.mozilla.org/integration/mozilla-inbound/rev/56da72c95d48

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1accd58e429e
https://hg.mozilla.org/mozilla-central/rev/56da72c95d48