741760 - svg-documents-as-images end up to CC graph

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

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

We don't seem to optimize out from CC graph those SVG documents which are
loaded from <html:img>
As an example, load http://blog.dholbert.org/2010/10/svg-as-image.html and check the
CC log. (easy way is to install about:cc addon from bug 726346, run CC and find documents)

Daniel, could you perhaps explain what and where keeps svg documents alive?
Can we somehow easily access the svg document from the html document?
Since if we could, we could mark svg document to be in CC generation when the html document
is in it.

Comment 1

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

I think only those documents which are animated end up to the cc graph. Perhaps
non-animated don't get to purple buffer so often.

Comment 2

[Daniel Holbert [:dholbert]]

> Daniel, could you perhaps explain what and where keeps svg documents alive?

IIRC, the image cache owns a VectorImage (implements the imgIContainer interface)
 ... which owns a SVGDocumentWrapper  (convenience layer to make VectorImage simpler)
 ... which owns a nsIContentViewer
 ... which owns the nsIDocument.

So -- when a VectorImage gets freed from the image cache, it frees its SVGDocumentWrapper, and the SVGDocumentWrapper's destructor ends up calling SVGDocumentWrapper::DestroyViewer(), which calls OnPageHide on the document and Close()/Destroy() on the nsIContentViewer before releasing its pointer to the viewer:
 http://mxr.mozilla.org/mozilla-central/source/image/src/SVGDocumentWrapper.cpp#92

> Can we somehow easily access the svg document from the html document?

Yes -- given a handle to the imgIContainer (which e.g. the nsImageFrame can look up), we can call imgIContainer::GetRootLayoutFrame().  That returns the SVG's root nsIFrame, from which we can grab the nsIDocument.

> Since if we could, we could mark svg document to be in CC generation when the html
> document

Is that problematic if other documents are also simultaneously using that image, though?  Imagelib gives out handles to the same underlying image (just based on the image URI being the same), so images don't "belong" to any one document.  So I'm not immediately clear on why the CC properties of one document should rub off on an image that happens to be used in that document.

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Is that problematic if other documents are also simultaneously using that
> image, though?  Imagelib gives out handles to the same underlying image
> (just based on the image URI being the same), so images don't "belong" to
> any one document.  So I'm not immediately clear on why the CC properties of
> one document should rub off on an image that happens to be used in that
> document.

The CC just wants to know when some data is definitely alive, so it can avoid looking at it.  If the SVG document is kept alive by multiple things, then if any of those is definitely alive, then the SVG document is definitely alive.

Comment 4

[Daniel Holbert [:dholbert]]

Ah, OK -- that sounds like a sensible thing to be doing, then.

At some point in the future, if we were to decide to throw away SVG images in background tabs, then it sounds like this CC annotation could potentially keep parts of those images alive for longer than we'd like.  But we probably aren't going to do anything like that anytime soon, and it'd be at worst a temporary leak, I think.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> > Daniel, could you perhaps explain what and where keeps svg documents alive?
> So -- when a VectorImage gets freed from the image cache, it frees its
> SVGDocumentWrapper, and the SVGDocumentWrapper's destructor ends up calling
> SVGDocumentWrapper::DestroyViewer(), which calls OnPageHide on the document
> and Close()/Destroy() on the nsIContentViewer before releasing its pointer
> to the viewer:
>  http://mxr.mozilla.org/mozilla-central/source/image/src/SVGDocumentWrapper.
> cpp#92

(To follow up on Comment 2 -- from that point on, I'm fuzzy on the document-cleanup details, but I believe any remaining refcounted pointers to the document will either get cycle-collected or are cleaned up as a result of the nsIContentViewer going away.  Off the top of my head, I don't know who owns such pointers.)

I don't immediately recall why animated images in particular would be more likely to end up in the CC graph.  Note that the nsSMILAnimationController has a weak pointer to its document, so it's not creating cycles at least.

Comment 6

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

(In reply to Daniel Holbert [:dholbert] from comment #5)> 
> I don't immediately recall why animated images in particular would be more
> likely to end up in the CC graph.
Something just ends up addref/release something in the svg document.

Comment 7

[Daniel Holbert [:dholbert]]

Ah, right.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> > Can we somehow easily access the svg document from the html document?
> 
> Yes -- given a handle to the imgIContainer (which e.g. the nsImageFrame can
> look up), we can call imgIContainer::GetRootLayoutFrame().  That returns the
> SVG's root nsIFrame, from which we can grab the nsIDocument.

For example, this:
  http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsImageLoadingContent.cpp#310
shows how you can get a handle on the imgIContainer for e.g. an nsHTMLImageElement.

Unfortunately there are a number of different places where an image can get referenced by a nsIDocument, depending on the way the image is being used. (<img> vs bullet vs CSS background/border vs some XUL-specific cases)

For reference, I think patches 4,5,6,7,8 on bug 666446 cover all or most of the situations that can reference an image.  (Note that nsImageLoadingContent covers <img> / <svg:image> / <svg:feImage>, and nsImageLoader covers CSS background/border.)

Comment 9

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

Need to figure out something here, since Australis uses svg documents, and they are now
all the time in the graph. (Luckily quite small docs)

Comment 10

[Daniel Holbert [:dholbert]]

So as long as these SVG images are in the imagelib cache, they're definitely alive. Put another was, as long as imagelib has a SVGDocumentWrapper (and VectorImage) object for a given SVG image, the SVG document is definitely alive. (This is true even if there are no host documents currently using the image. So I think the "host document getting a handle on the inner SVG document" strategy that we've been hinting at so far is probably not the right way to go.)

Is there some invocation / annotation we can do from inside imagelib, to make the Cycle Collector aware that these documents are definitely alive?

Comment 11

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

The easiest is probably to call imglib in nsCCUncollectableMarker::Observe
and update relevant svg documents CCGeneration.

So after sGeneration has been updated in ::Observe,
somehow call imagedoc->MarkUncollectableForCCGeneration(nsCCUncollectableMarker::sGeneration)

Comment 12

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

Attachment: patch

This helps.
My testcase is to open http://dom.spec.whatwg.org/ in e10s and check the CC log.
SVG images show up in CC graph after each paint or so, with the patch they are gone.
The IsBeingUsedAsImage/IsVisible handling is based on what dholbert hinted on IRC.
http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/image/SVGDocumentWrapper.cpp#67


I'm not 100% happy with the name IsCertainlyAliveNode, but IsANodeWhichShouldCertainlyKeptAlive is a bit long.


https://treeherder.mozilla.org/#/jobs?repo=try&revision=18864d990675e7e16e9c28b929b40df25bfe765e

Comment 13

[Andrew McCreight [:mccr8]]

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

Review of attachment 8762871 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, assuming that dholbert can verify that the IsBeingUsedAsImage() && IsVisible() implies that the document is alive logic.l

Comment 14

[Daniel Holbert [:dholbert]]

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

Review of attachment 8762871 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on IsBeingUsedAsImage() && IsVisible() as an aliveness test, modulo a comment update noted below.  (I didn't look closely at the rest of the patch, since I don't understand those CC mechanics as much and mccr8 already reviewed them.)

::: dom/base/FragmentOrElement.cpp
@@ +1491,5 @@
> +  MOZ_ASSERT(aNode->GetUncomposedDoc() == aDoc);
> +
> +  // We know the node is certainly alive if it is in a document which has been
> +  // marked to be in-CC-generation or if the document is a currently visible
> +  // svg image.

"currently visible" is misleading here.  IsVisible doesn't tell you whether the image is actually visible (to the user) -- it really just tells you whether it's still in the cache.

So -- consider extending this comment slightly with:
  // marked to be in-CC-generation or if the document is an svg image that's
  // being kept alive by the image cache. (Note that an svg image's internal
  // SVG document will receive an OnPageHide() call when it gets purged from
  // the image cache; hence, we use IsVisible() as a hint that the document is
  // actively being kept alive by the cache.)

Comment 15

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

Attachment: +comments

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44256084ae2e
svg-documents-as-images end up to CC graph, r=mccr8, dholbert

Comment 17

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

ok, this is leaking. Some assumption is wrong here.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for reftest leaks:

https://hg.mozilla.org/integration/mozilla-inbound/rev/13fb23503932
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=44256084ae2e8a2777eeaf040f8bdb584a818ef8

Comment 19

[Daniel Holbert [:dholbert]]

I haven't been able to reproduce the leaks locally, but I'm pretty sure they're from SVG-in-OpenType fonts (tested by the "text-svgglyphs" reftests directory).  We use some of our SVG-as-an-image code for that feature, but I don't recall exactly how they work. I'd be willing to bet they violate one of our assumptions here, though. (e.g. maybe their internal documents never receive an OnPageHide() call?)

Here's a Try run which I used to test this theory:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=4352cdd11188
That run is parented with Smaug's inbound push (so it's got the leaky patch here).  My only change was to disable the text-svgglyphs reftest.list file.  This produced a green debug reftest run.  So, I think that confirms that the leaking is all happening in stuff that we load for the text-svgglyphs testcaess.

Comment 20

[Daniel Holbert [:dholbert]]

(Looks like SVG-in-opentype documents indeed never call OnPageHide. I've got a patch that I think should fix that; I'll probably spin off a helper-bug to land that patch, if my initial tests pan out.)

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f42ac848925a
svg-documents-as-images end up to CC graph, r=mccr8, dholbert

Comment 22

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

bug 1280676 landed to m-i. re-landing.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f42ac848925a