To reproduce: 1. Run Firefox with XPCOM_MEM_LEAK_LOG=2. 2. Load the testcase. 3. Quit Firefox. Result: trace-refcnt complains of a leak.

I don't see where nsSVGGlyphFrame ever destroys its mTextRun on destruction. So I'm not sure why we don't just leak textruns any time we have some SVG text around...

~nsSVGGlyphFrame() calls ClearTextRun() which should delete the textrun.

Ah, indeed. The problem is elsewhere. Here's the problematic stack that allocates the textrun we leak: nsSVGGlyphFrame::EnsureTextRun(float*, float*, int) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/svg/base/src/../../../../../mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp:1384) nsSVGGlyphFrame::GetBaselineOffset(int) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/svg/base/src/../../../../../mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp:731) nsSVGGlyphFrame::SetGlyphPosition(float, float, int) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/svg/base/src/../../../../../mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp:914) nsSVGTextFrame::UpdateGlyphPositioning(int) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/svg/base/src/../../../../../mozilla/layout/svg/base/src/nsSVGTextFrame.cpp:407) nsSVGTextFrame::NotifyGlyphMetricsChange() (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/svg/base/src/../../../../../mozilla/layout/svg/base/src/nsSVGTextFrame.cpp:292) nsSVGTextContainerFrame::NotifyGlyphMetricsChange() (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/svg/base/src/../../../../../mozilla/layout/svg/base/src/nsSVGTextContainerFrame.cpp:61) nsSVGGlyphFrame::NotifyGlyphMetricsChange() (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/svg/base/src/../../../../../mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp:1248) nsSVGGlyphFrame::DidSetStyleContext(nsStyleContext*) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/svg/base/src/../../../../../mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp:229) nsIFrame::SetStyleContext(nsStyleContext*) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/generic/../../../mozilla/layout/generic/nsIFrame.h:667) nsFrameManager::ReResolveStyleContext(nsPresContext*, nsIFrame*, nsIContent*, nsStyleChangeList*, nsChangeHint, int) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/base/../../../mozilla/layout/base/nsFrameManager.cpp:1203) nsFrameManager::ReResolveStyleContext(nsPresContext*, nsIFrame*, nsIContent*, nsStyleChangeList*, nsChangeHint, int) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/base/../../../mozilla/layout/base/nsFrameManager.cpp:1438) nsFrameManager::ComputeStyleChangeFor(nsIFrame*, nsStyleChangeList*, nsChangeHint) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/base/../../../mozilla/layout/base/nsFrameManager.cpp:1479) nsCSSFrameConstructor::RestyleElement(nsIContent*, nsIFrame*, nsChangeHint) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/base/../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:7560) nsCSSFrameConstructor::ProcessOneRestyle(nsIContent*, nsReStyleHint, nsChangeHint) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/base/../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:11021) nsCSSFrameConstructor::ProcessPendingRestyleTable(nsDataHashtable<nsISupportsHashKey, nsCSSFrameConstructor::RestyleData>&) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/base/../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:11125) nsCSSFrameConstructor::ProcessPendingRestyles() (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/base/../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:11153) PresShell::FlushPendingNotifications(mozFlushType) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/base/../../../mozilla/layout/base/nsPresShell.cpp:4756) nsDocument::FlushPendingNotifications(mozFlushType) (/home/bzbarsky/mozilla/vanilla/obj-firefox/content/base/src/../../../../mozilla/content/base/src/nsDocument.cpp:6487) nsComputedDOMStyle::GetPropertyCSSValue(nsAString_internal const&, nsIDOMCSSValue**) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/style/../../../mozilla/layout/style/nsComputedDOMStyle.cpp:451) nsComputedDOMStyle::GetPropertyValue(nsAString_internal const&, nsAString_internal&) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/style/../../../mozilla/layout/style/nsComputedDOMStyle.cpp:312) nsComputedDOMStyle::GetPropertyValue(nsCSSProperty, nsAString_internal&) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/style/../../../mozilla/layout/style/nsComputedDOMStyle.cpp:257) GetCSSComputedValue (/home/bzbarsky/mozilla/vanilla/obj-firefox/content/smil/../../../mozilla/content/smil/nsSMILCSSProperty.cpp:81) nsSMILCSSProperty::GetBaseValue() const (/home/bzbarsky/mozilla/vanilla/obj-firefox/content/smil/../../../mozilla/content/smil/nsSMILCSSProperty.cpp:130) nsSMILCompositor::ComposeAttribute() (/home/bzbarsky/mozilla/vanilla/obj-firefox/content/smil/../../../mozilla/content/smil/nsSMILCompositor.cpp:158) DoComposeAttribute(nsSMILCompositor*, void*) (/home/bzbarsky/mozilla/vanilla/obj-firefox/content/smil/../../../mozilla/content/smil/nsSMILAnimationController.cpp:281) nsTHashtable<nsSMILCompositor>::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) (/home/bzbarsky/mozilla/vanilla/obj-firefox/content/smil/../../dist/include/nsTHashtable.h:421) PL_DHashTableEnumerate (/home/bzbarsky/mozilla/vanilla/obj-firefox/xpcom/build/pldhash.c:754) nsTHashtable<nsSMILCompositor>::EnumerateEntries(PLDHashOperator (*)(nsSMILCompositor*, void*), void*) (/home/bzbarsky/mozilla/vanilla/obj-firefox/content/smil/../../dist/include/nsTHashtable.h:242) nsSMILAnimationController::DoSample(int) (/home/bzbarsky/mozilla/vanilla/obj-firefox/content/smil/../../../mozilla/content/smil/nsSMILAnimationController.cpp:363) nsSMILAnimationController::Resample() (/home/bzbarsky/mozilla/vanilla/obj-firefox/content/svg/content/src/../../../../dist/include/nsSMILAnimationController.h:86) nsSMILAnimationController::FlushResampleRequests() (/home/bzbarsky/mozilla/vanilla/obj-firefox/content/svg/content/src/../../../../dist/include/nsSMILAnimationController.h:93) nsSVGElement::FlushAnimations() (/home/bzbarsky/mozilla/vanilla/obj-firefox/content/svg/content/src/../../../../../mozilla/content/svg/content/src/nsSVGElement.cpp:1859) nsSVGPreserveAspectRatio::GetAnimValue(nsSVGElement*) const (/home/bzbarsky/mozilla/vanilla/obj-firefox/content/svg/content/src/../../../../../mozilla/content/svg/content/src/nsSVGPreserveAspectRatio.h:117) nsSVGUtils::GetViewBoxTransform(nsSVGElement*, float, float, float, float, float, float, nsSVGPreserveAspectRatio const&, int) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/svg/base/src/../../../../../mozilla/layout/svg/base/src/nsSVGUtils.cpp:812) nsSVGSVGElement::GetViewBoxTransform() (/home/bzbarsky/mozilla/vanilla/obj-firefox/content/svg/content/src/../../../../../mozilla/content/svg/content/src/nsSVGSVGElement.cpp:1008) nsSVGOuterSVGFrame::GetCanvasTM() (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/svg/base/src/../../../../../mozilla/layout/svg/base/src/nsSVGOuterSVGFrame.cpp:746) nsSVGTextFrame::GetCanvasTM() (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/svg/base/src/../../../../../mozilla/layout/svg/base/src/nsSVGTextFrame.cpp:276) nsSVGGlyphFrame::GetCanvasTM() (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/svg/base/src/../../../../../mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp:626) nsSVGGlyphFrame::GetGlobalTransform(gfxMatrix*) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/svg/base/src/../../../../../mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp:1279) nsSVGGlyphFrame::EnsureTextRun(float*, float*, int) (/home/bzbarsky/mozilla/vanilla/obj-firefox/layout/svg/base/src/../../../../../mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp:1328) That looks bad. In particular, note that we're flushing restyles here under frame code; if we tried a bit harder I bet we could make |this| die a grisly death under that GetGlobalTransform call. And then I would hope we'd just die due to frame poisoning, but I'm not quite willing to bet on that... Do we have existing bugs on this sort of callstack? I thought we did somewhere.

Yeah, I think we do. We can't do FlushAnimations in places like that.

Possibly we should just be flushing animations when we flush style.

Maybe that would work (I don't know exactly when we flush style), but I think we'd also need to flush in any DOM functions that can manipulate the timeline, such as in SVGSVGElement.SetCurrentTime and SVGAnimationElement.BeginElement et. al. The DOM should reflect the change in timeline immediately after it's manipulated.

(In reply to comment #7) > Maybe that would work (I don't know exactly when we flush style), but I think > we'd also need to flush in any DOM functions that can manipulate the timeline, > such as in SVGSVGElement.SetCurrentTime and SVGAnimationElement.BeginElement > et. al. The DOM should reflect the change in timeline immediately after it's > manipulated. I think Opera is more liberal than this -- if I recall correctly, those methods (SetCurrentTime / BeginElement / etc.) don't cause Opera to immediately update its animated values -- instead, they effectively cause a Runnable to be queued up, and that handles the updating. (So animated values will be updated before any subsequent SetTimeout() calls are handled.) Perhaps we should switch to behavior more like that.

I believe Opera is looking at changing their behavior to reflect timeline changes immediately like we do. I'm not discounting that we could instead resort to their behavior, but I think we should only do that if we really have to.

Ok, agreed. So I think nsSVGLength2 already does the right thing here -- it calls FlushAnimations() in the DOM getters (e.g. DOMAnimVal::GetValue, DOMAnimVal::GetValueAsString)[1], and not in any internal getters. We should mimic that behavior elsewhere -- DOM getters should flush, and internal getters should not. (In particular, in bz's stack from comment 4, we shouldn't be flushing in nsSVGPreserveAspectRatio::GetAnimValue.) [1] http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGLength2.h#189

(In reply to comment #10) > Ok, agreed. > > So I think nsSVGLength2 already does the right thing here -- it calls > FlushAnimations() in the DOM getters (e.g. DOMAnimVal::GetValue, > DOMAnimVal::GetValueAsString)[1], and not in any internal getters. One correction there, RE "not in any internal getters" -- currently, nsSVGLength2::GetAnimValue (an internal getter) *does* actually call FlushAnimations(). It shouldn't, though -- that will be fixed in this bug's patch.

Here's the fix. This: (a) Adds FlushAnimations() calls to DOM getters that touch animated values (b) Removes FlushAnimations() calls from all the internal getters (c) Removes "nsSVGElement aSVGElement" parameter from internal getters, in cases where it was just there for the (now-removed) call to FlushAnimations(). (d) Tweaks a bunch of method-calls in .cpp files, to match part (c) (e) Moves nsSVGViewBox::GetAnimValue() definition from .cpp to .h file, for consistency

Comment on attachment 428530 [details] [diff] [review] fix v1 nsSVGFE::SetupScalingFilter no longer needs its last argument. Can you remove that? Also can you add a comment to the DOM getters something like: // Script may have modified the timeline, so DOM getters resample.

(In reply to comment #11) > One correction there, RE "not in any internal getters" -- currently, > nsSVGLength2::GetAnimValue (an internal getter) *does* actually call > FlushAnimations(). It shouldn't, though -- that will be fixed in this bug's > patch. Yeah, I'm still trying to work out why I added that call there. It really should be only DOM calls that trigger animation flush so far as I can recall. I thought I'd been pretty careful about that but I suppose so long as all the tests pass with that flush removed everything should be ok.

(In reply to comment #13) > (From update of attachment 428530 [details] [diff] [review]) > nsSVGFE::SetupScalingFilter no longer needs its last argument. Can you remove > that? Fixed. > Also can you add a comment to the DOM getters something like: Fixed (just added once per file, above the first affected getter -- any other getters are generally pretty close below that one). (In reply to comment #14) > should be only DOM calls that trigger animation flush so far as I can recall. > so long as all the > tests pass with that flush removed everything should be ok. Thanks Brian -- the tests do indeed still pass with this change, so I think it's correct.

Here's the testcase, packaged as a crashtest (with reftest-wait added to make sure we get through the script). I've confirmed that it causes leaks on TryServer pre-patching: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1267038578.1267051132.13181.gz http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1267038577.1267048264.3384.gz ... but does not leak post-patching: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1267038630.1267051041.12920.gz http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1267038629.1267052410.17904.gz

Pushed fix: http://hg.mozilla.org/mozilla-central/rev/35637227632d and test: http://hg.mozilla.org/mozilla-central/rev/7f1105f96113

This followup removes one more FlushAnimations call, in nsSVGElement::GetAnimatedLengthValues. We can receive calls to GetAnimatedLengthValues from painting code & frame-reconstruction code, and we don't want to perform samples during either of those. (See for example the stack bz pasted in bug 533291 comment 24, which is discussed a bit in bug 483584 comment 21). I'm including two hang tests (as crashtests) from bug 483584. They actually stopped hanging in the last few weeks because bug 533291 capped the infinite recursion -- but this patch will cap the infinite recursion even earlier.

Pushed followup: http://hg.mozilla.org/mozilla-central/rev/367cdcfa611b

Hm, followup seems to have triggered some opt-only reftest failures. Windows: init-pause-1.svg freeze-applied-late-1.svg freeze-applied-late-2.svg freeze-applied-late-4.svg http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1267219944.1267221838.6448.gz freeze-applied-late-1.svg http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1267222095.1267223530.11777.gz Mac: freeze-applied-late-4.svg http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1267224816.1267226519.20528.gz (I only see one Linux opt-reftest cycle with my patch at this point, and it's green, so these failures must be non-linux and/or random) Backing out followup & reopening...

backed out followup: http://hg.mozilla.org/mozilla-central/rev/61a9334cd00f

So the failures show that we're not always resampling (e.g. to detect that a formerly |fill="remove"| animation is now |fill="freeze") before we take the reftest snapshot. The change in "fill" attribute still makes us *request* a resample, but it seems that after the followup is applied, we don't have anywhere that will reliably flush that request until after the reftest snapshot. (i.e. we fall back on the animation timer, rather than an explicit flush) We need to explicitly flush resample requests before the reftest snapshot. dbaron suggests that nsPresShell::FlushPendingNotifications would be a good place for that (right before we flush restyle requests). I think this is also basically what roc suggested in comment 6. :)

Here's the followup again, with a FlushAnimations call added in nsPresShell::FlushPendingNotifications, as described in comment 22. This patch passed tryserver earlier today. Requesting r=bz on the nsPresShell::FlushPendingNotifications chunk.

FlushResampleRequest can trigger arbitrary code execution (via setting attributes), right?

No, I don't think it can trigger arbitrary code execution -- AFAIK, we don't fire events for SMIL attribute-setting. The attribute-setting is done via specializations of nsISMILAttr::SetAnimValue. Those specializations will generally set the animated value directly, and then call nsSVGElement::DidAnimateXXX() to get the value-change noticed & trigger invalidation/repainting. e.g. for nsSVGLength2, SetAnimValue calls aSVGElement->DidAnimateLength(mAttrEnum), which just calls frame->AttributeChanged(). It doesn't actually fire any mutation events for the value-change, though. (<animateTransform> might currently be an exception to this, since it uses DidModify() instead of DidAnimateXXX(), but that should be fixed up in Bug 548899.)

(In reply to comment #25) > (<animateTransform> might currently be an exception > to this, since it uses DidModify() instead of DidAnimateXXX(), but that should > be fixed up in Bug 548899.) So yeah, nsSVGValue::DidModify() calls NotifyObservers, which could trigger arbitrary code. I'm working on Bug 548899 now, so this won't be an issue anymore. (and I don't intend to land the followup here until after that's fixed)

Comment on attachment 429465 [details] [diff] [review] followup v2: also flush animations in nsPresShell::FlushPendingNotifications r=me on the presshell changes assuming bug 548899 is fixed first.

Comment on attachment 429465 [details] [diff] [review] followup v2: also flush animations in nsPresShell::FlushPendingNotifications carrying forward r=jwatt on the non-presshell changes

followup v2 pushed (with Bug 548899's push): http://hg.mozilla.org/mozilla-central/rev/e9ab6e4d121d

Backed out followup again -- this passed TryServer, but it caused some NS_ABORT_IF_FALSE failures on debug mochitest boxes. (I can't wait until TryServer does debug unittest runs :-/ ) http://hg.mozilla.org/mozilla-central/rev/81b2e55ea5b4 The NS_ABORT_IF_FALSE is: ###!!! ABORT: Clearing window pointer while animations are unpaused: 'aScriptGlobalObject || !mAnimationController || mAnimationController->IsPausedByType( nsSMILTimeContainer::PAUSE_PAGEHIDE | nsSMILTimeContainer::PAUSE_BEGIN)', file /builds/slave/mozilla-central-linux-debug/build/content/base/src/nsDocument.cpp, line 3591 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1267545286.1267547287.12723.gz This happens when running tests completely unrelated to SMIL. I think it happens because we've added a call to "mDocument->GetAnimationController()" in generic layout code, which causes us to lazily initialize the animation controller in cases where we otherwise would never have initialized it. I think we need a "HasAnimationController()" function, which would check if nsDocument::mAnimationController is non-null without doing the lazy initialization.

(In reply to comment #30) > I think we need a "HasAnimationController()" function, which would check if > nsDocument::mAnimationController is non-null without doing the lazy > initialization. I'll do this in bug 540090.

Just pushed bug 540090's fix, followed by this followup again (now with HasAnimationController() in place of GetAnimationController(), in the PresShell chunk, per comment 30): http://hg.mozilla.org/mozilla-central/rev/d6dbb4231c11 Hopefully it'll stick this time. :)

Any objections to opening this up? It's trunk-only, and it's been fixed on trunk for weeks and was fixed in the latest developer preview which was released over a week ago. (Plus, we never had any particularly scary testcases -- just a leak with a stack that was flagged as potentially scary looking.)