533291 - SVG/SMIL optimization: Avoid re-composing groups of animations that haven't changed

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: fix

The initial SMIL patch had code to avoid re-composing animations whose values haven't changed since our last sample, as an optimization.

However, for correctness, we couldn't use the optimization until we had functionality to check whether the animation *target* had changed, so we commented out this optimization in the initial landing.

The attached patch adds this missing functionality (in a method called "nsSMILAnimationFunction::UpdateCachedTarget", which returns a boolean indicating whether the cached target has changed).  It also re-enables the optimization.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Previous fix used a nsSMILCompositorKey to represent the "cached target", because that struct contains exactly the information we're interested in here. (the target element, target attribute name, and target attribute type -- CSS vs XML).

However, that struct has an owning reference on the element & attribute name, which could potentially make those live longer than we want. (i.e. if an animation gets disabled, we'll stop updating its cached target, and so we'd retain a reference to the old cached target indefinitely)

This new fix solves this, with these changes:
 (a) (cosmetic): rename nsSMILCompositorKey to "nsSMILTargetIdentifier", since that's a better description of what that struct is.
 (b) Create a "weak" version, nsSMILWeakTargetIdentifier, which has private non-refcounted raw pointers, solely for use in equality tests in UpdateCachedTarget

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: break test

The fix currently breaks this test -- basically, it's a situation where our existing animations haven't changed, but one animation has been removed.  In that situation, we need to make sure to re-compose.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Patch A: re-form nsSMILCompositorKey into nsSMILTargetIdentifier & nsSMILWeakTargetIdentifier

I'm now posting a series of patches to fix this bug.

Patches A, B, and C all lay groundwork for enabling the optimization, and patch D actually enables the optimization (and includes two reftests based on the attached "break test" testcase).

The previous |fix v2| here was basically patches A+B+D, and patch C specifically addresses the situation that the "break test" attachment poses.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Patch B: Add UpdateCachedTarget() method, to catch when animation function's target changes

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Patch C: when an animation becomes inactive, make sure to recompose its target, particularly if nothing else has changed

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Patch Z: enable optimization & add tests

Trivial patch -- just enables the early-return optimization and adds two tests based on the already-attached |break test| testcase.

Comment 7

[Daniel Holbert [:dholbert]]

Forgot to mention -- this bug's patches layer on top of the syncbase timing patches in bug 474743.  (This bug is essentially independent of that bug, but there's some shared contextual code in nsSMILAnimationController.h that causes bitrot if the patches are applied in the wrong order.)

Adding dependency on that bug.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 420494 [details] [diff] [review]
Patch Z: enable optimization & add tests

The final optimization-enabling  (patch D) actually breaks a few mochitests right now:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1262884647.1262894038.26196.gz

Those mochitests pass locally if I don't apply patch D. Investigating...

Comment 9

[Daniel Holbert [:dholbert]]

Ok, so those mochitest failures indicate that there are still two situations where the optimization would have us return early (because no animation functions have changed), but where something *else* has changed that we need to recompose to account for:
===============
 (i)  changes to the text-zoom value.  (SVG is supposed to ignore text-zoom, so we divide it out when creating our animated value of font-size[1]. If it changes, we need to redo that calculation.)

 (ii) changes to underlying base value.  e.g. suppose we have:
   <rect x="0">
     <set attributeName="x" by="30" begin="0" dur="0.1s" fill="freeze" />
   </rect>
  So, in the first sample, we'll compose and get an animated value of 30.  The animation doesn't change after that, so we won't bother recomposing in subsequent samples -- we'll take the early return in Patch D.
  Then, suddenly, a script calls rect.setAttribute("x", "100"), changing the base value.  This is a change that need to make us recompose (to get a new animated value of 130) -- but right now, we won't notice.  We'll still take the early return in Patch D, because the animation functions haven't changed.
===============

We could fix (i) by caching the text-zoom value in the nsSMILAnimationController every sample, and if it changes, we'd force-recompose all font-size animations during that sample.

I'm not immediately sure what the best way to fix (ii) is, though... My only idea so far is this:
 - Make the nsSMILCompositor cache its target's base value. Then when we clear out mLastCompositorTable with RemoveCompositorFromTable, we could also check the base value from last sample's compositor against the base value from this samples compositor.

As an alternative, I was initially thinking that we might be able to store state for "this attribute's base value changed since last sample" on the target element itself.  But I don't think this is possible for e.g. CSS properties whose base value is 'inherit', or whose base value is controlled by a stylesheet. In that case, the base value (un-animated computed style) could easily change between samples without the target element being notified.

[1] http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILCSSValueType.cpp#340

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 420494 [details] [diff] [review]
Patch Z: enable optimization & add tests

Renaming Patch D to "Patch Z", since it's really the final trivial "enable the optimization" patch, and since it looks like we'll need a few more patches before we can apply it. (to handle cases (i) and (ii) above -- I'll probably label these additional patches D & E.)

Comment 11

[Daniel Holbert [:dholbert]]

Two more changes we need to check for, to make sure we recompose (e.g. in the case of a frozen animation for whom we'd otherwise skip recomposing, because it looks like nothing's changed):
 (iii) Computed value of 'font-size' changing out from under us, during an animation that involves "em" units.
 (iv) Computed value of 'color' changing out from under us, during an animation that involves "currentColor" as one of the endpoints.

(I caught this due to a mochitest failure in "test_animLengthUnits.xhtml", after applying this optimization plus WIP patches to fix the two issues from comment 9.)

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: Patch B v2: Add UpdateCachedTarget() method, to catch when animation function's target changes [r=roc]

Here's patch B again, now including a minor tweak to the header comment for HasChanged(). (That header-comment included a note that's no longer exactly correct, after this patch's code changes.)

Carrying forward r+.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: Patch C: recompose newly-inactive animations. [r=roc]

Updated version of Patch C -- I've just added a clause at the beginning of nsSMILCompositor::ComposeAttribute to check if we've got 0 animation functions, and if so, bail out (clearing the target's animated value).

It didn't used to be possible to have a nsSMILCompositor with no animation functions, but it now is (as of this patch), because the added PutEntry call in AddAnimationToCompositorTable can potentially create a new nsSMILCompositor in the table, without adding an animation function to that compositor. (since the animation function we're concerned with is newly-inactive)

The change I've made just makes ComposeAttribute make a little more sense in this situation -- otherwise, it would iterate across a zero-length list twice, and then set the animated value == the base value, which isn't particularly useful.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: Patch D: cache base values

Patch summary:
 1. (primary) Cache each animation-target's base value, and pass the cached values forward from one sample's compositors to the next.  This lets us detect that we need to force-recompose when the underlying value changes, in e.g. a "by" or "to" animation. (This is item "ii" from comment 9).

 2. Refactor nsSMILCompositor::ComposeAttribute a bit:
   -- Call helper-function to cache base value, after we look it up, to achieve the above.
   -- Refactor out a chunk of existing code into a helper method, GetFirstFuncToAffectSandwich()
   -- Remove 'changed' local var, and just use the "mForceCompositing" member var (created in an earlier patch) to serve the same purpose.  (This lets helper functions -- e.g. the new GetFirstFuncToAffectSandwich method -- tell us that we've changed)
   -- Remove unneeded null-check on mAnimationFunctions[i].  (There's no reason it'd be null, and we already use those entries without null-checking them elsewhere)
   -- Rename "resultValue" to "sandwichResultValue" to make its use a little clearer. (It starts out as the base value, & gets progressively updated via calls to nsSMILAnimationFunction::ComposeResult)

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: Patch E: Add "aCanCache" param to ValueFromString

This new (and I think final) patch handles cases (iii) and (iv) from comment 11.  It also handles case (i) from comment 9 (which could also be handled in a custom patch, but doesn't need to right now).

The idea here is to have nsISMILAttr::ValueFromString tell the caller (via a new outparam "aCanCache") whether the given string will **always** compute to the same value, or whether the resulting value might change from sample to sample.

In most implementations of ValueFromString, this outparam can just be set to true (i.e. you can cache the value -- it won't change), with these exceptions:
 - for SVG Lengths (nsSVGLength2::SMILLength) we need to re-evaluate the value every sample if it uses relative units (em/ex/%)
 - for CSS properties, we also need to handle relative units, along with 'inherit' and 'currentColor'.  These would require a bit more work to handle, since 'inherit' and 'currentColor' both get converted away in nsStyleAnimation::ComputeValue/ExtractComputedValue -- so for now, I'm just assuming all CSS properties / mapped-attributes need to be reparsed on every sample.  (see comment added in nsSMILCSSProperty::ValueFromString)  This is why this patch takes care of case (i) from comment 9 right now -- it makes us always recompose 'font-size' (and all other properties).

Anyway, so now that we've got this new "aCanCache" outparam in ValueFromString -- if that gets set to true, then nsSMILAnimationFunction (the caller of ValueFromString) can behave like it always has changed (because, really, it's values might have changed _meaning_), for the purpose of forcing a recompose.  I accomplish this by checking one additional flag in nsSMILAnimationFunction::HasChanged().

NOTE: The parameter name "aCanCache" might be a little misleading, because I don't actually cache the resulting value anywhere.  What really gets "cached" (by not being updated when nothing has changed) is the result of the whole animation sandwich.  Still, I think the name "aCanCache" conveys the right idea, and it's shorter than the other names I came up with. :)

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: Patch Z v2: enable optimization & add tests

I'm rebasing "Patch Z" on top of the new patches (D and E), and I've also added 1 new test "test_smilChangeAfterFrozen.xhtml".  This test checks that we correctly update the animated value when a script changes...
 - the base value (in "by" animation, where "from" implicitly == the base value)
 - an inherited value (in from/to animation involving "inherit")
 - the 'color' value (in from/to animation involving "currentColor")
...in a completed, frozen animation.  This test unfortunately involves some setTimeouts, which are required because of bug 545282. (but I don't think they can cause randomorange, as explained above SETTIMEOUT_INTERVAL in the test)

Requesting review.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is good as far as it goes. However, in the long term we wouldn't want to keep recomposing an animation forever just because CSS 'font-size' was animated (for example). Do you have any ideas on how we might eventually fix that, if we need to?

Comment 19

[Daniel Holbert [:dholbert]]

If you're talking about "font-size" w.r.t. changes in text-zoom, as described in part (i) of comment 9: I've got a pretty trivial patch that could handle that.  It just caches the text-zoom value, and force-recomposes any animations on "font-size" when it changes.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/6fce6ee8bbfa/smil_cacheTextZoom
(This patch wouldn't add anything right now, though, since we'll already always recompose CSS properties for now.)

If you're talking *not* always recomposing CSS properties: we can clean up most of those cases as described in the last sentence of this comment, in Patch E:
>+  // XXXdholbert: For simplicity, just assume that all CSS values have to
>+  // reparsed every sample. This prevents us from doing the "nothing's changed
>+  // so don't recompose" optimization (bug 533291) for CSS properties & mapped
>+  // attributes.  If it ends up being expensive to always recompose those, we
>+  // can be a little smarter here.  We really only need to disable aCanCache
>+  // for "inherit" & "currentColor" (whose values could change at any time), as
>+  // well as for length-valued types (particularly those with em/ex/percent
>+  // units, since their conversion ratios can change at any time).
>+  aCanCache = PR_FALSE;
>+  return NS_OK;

With the potential changes described there, we'd only keep recomposing animations if they were part of a sandwich that includes em/ex/% units or the inherit/currentColor values.  (These are a bit messy to check for -- they'd require some changes to the nsStyleAnimation parsing methods -- so I'm not making that change yet.)

Beyond that, we could even fix up those special-cases by extending what we cache on each compositor.  Patch D makes us cache the base value, but we could also cache (where appropriate) the...
  (a) inherited value
  (b) currentColor value
  (c) em/ex/% length-conversion-factors.
With that cached, we could detect when these values change, and force a recompose when that happens.  I suspect that could end up being too much caching-work, to optimize an infrequently-needed-and-not-too-expensive case.

Anyway -- most of the wins that we currently get from this bug are in <animateTransform>, which is very expensive to recompose (as compared to CSS properties), as shown in bug 527762.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, thanks. I definitely don't want to optimize this any further now.

Comment 21

[Daniel Holbert [:dholbert]]

Sounds good. Thanks for the review -- I'll land this (with supporting bug 542670) once the tree re-opens.

Comment 22

[Daniel Holbert [:dholbert]]

Landed:
http://hg.mozilla.org/mozilla-central/rev/358b21fc6be0
http://hg.mozilla.org/mozilla-central/rev/97a930b2fd07
http://hg.mozilla.org/mozilla-central/rev/44792d09c46a
http://hg.mozilla.org/mozilla-central/rev/26d2ef4bf7bf
http://hg.mozilla.org/mozilla-central/rev/3aef13f285d4
http://hg.mozilla.org/mozilla-central/rev/9cdfc9ee6754

Comment 23

[Daniel Holbert [:dholbert]]

The landed patches had 2 changes w.r.t. patches posted here:
 - Added aCanCache to new ValueFromString implementations (in nsSVGAngle, nsSVGInteger, nsSVGViewBox), in changeset 3aef13f285d4
 - Added viewBox attr on <svg> in an existing mochitest, in changeset 9cdfc9ee6754, because without that change, we get divide-by-zero issues due to using percent units with an implicitly zero-size viewbox. (due to 'display:none' I think) And on the linux TryServer tinderbox, that seems to makes our base value appear to stay the same when it should have changed, and breaks our optimization. (I couldn't reproduce the failure locally -- though I could reproduce the dividing-by-zero -- but anyway, this change fixed the test.)

Comment 24

[Boris Zbarsky [:bzbarsky]]

For the record, this isn't just an optimization.  It's needed to prevent hangs in certain cases.  For example, if I take a build from before this patch landed and open http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-30-t.html the browser hangs.  The reason it hangs is that processing a restyle triggers a frame reconstruct and creation of the frame triggers the anumation controller, which sets attributes (this might well be a separate bug we need to fix!), which in this case, because <use> is involved, immediately posts a reframe restyle.  Then we end up in an infinite loop processing restyles.

Relevant stack bit:

#0  nsCSSFrameConstructor::PostRestyleEventCommon (this=0x1e90e260, aContent=0x1e920310, aRestyleHint=eReStyle_Self, aMinChangeHint=nsChangeHint_ReconstructFrame, aForAnimation=0) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:11192
#1  0x129c65a8 in nsCSSFrameConstructor::PostRestyleEvent (this=0x1e90e260, aContent=0x1e920310, aRestyleHint=eReStyle_Self, aMinChangeHint=nsChangeHint_ReconstructFrame) at nsCSSFrameConstructor.h:238
#2  0x12a32fd4 in nsIPresShell::PostRecreateFramesFor (this=0x1e90de60, aContent=0x1e920310) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsPresShell.cpp:3508
#3  0x13373cae in nsSVGUseElement::TriggerReclone (this=0x1e920310) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/svg/content/src/nsSVGUseElement.cpp:454
#4  0x13374f94 in nsSVGUseElement::AttributeChanged (this=0x1e920310, aDocument=0x219fe600, aContent=0x1e91aa70, aNameSpaceID=0, aAttribute=0x12041c4, aModType=1) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/svg/content/src/nsSVGUseElement.cpp:202
#5  0x12d59332 in nsNodeUtils::AttributeChanged (aContent=0x1e91aa70, aNameSpaceID=0, aAttribute=0x12041c4, aModType=1) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsNodeUtils.cpp:124
#6  0x12d3b5db in nsGenericElement::SetAttrAndNotify (this=0x1e91aa70, aNamespaceID=0, aName=0x12041c4, aPrefix=0x0, aOldValue=@0x67aad8, aParsedValue=@0xbfffb044, aModification=1, aFireMutation=0, aNotify=1, aValueForAfterSetAttr=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsGenericElement.cpp:4430
#7  0x132efe8d in nsSVGElement::DidModifySVGObservable (this=0x1e91aa70, aObservable=0x1e904b84, aModType=nsISVGValue::mod_other) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/svg/content/src/nsSVGElement.cpp:990
#8  0x13376930 in nsSVGValue::NotifyObservers (this=0x1e904b84, f={__pfn = 0x15, __delta = 0}, aModType=nsISVGValue::mod_other) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/svg/content/src/nsSVGValue.cpp:70
#9  0x133769f1 in nsSVGValue::DidModify (this=0x1e904b84, aModType=nsISVGValue::mod_other) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/svg/content/src/nsSVGValue.cpp:87
#10 0x13380fa5 in nsSVGTransformSMILAttr::SetAnimValue (this=0x1a8e2fb0, aValue=@0xbfffb1a4) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/svg/content/src/nsSVGTransformSMILAttr.cpp:128
#11 0x1338897d in nsSMILCompositor::ComposeAttribute (this=0x11bece58) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/smil/nsSMILCompositor.cpp:170
#12 0x13382e3d in DoComposeAttribute (aCompositor=0x11bece58) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/smil/nsSMILAnimationController.cpp:280
#13 0x13383955 in nsTHashtable<nsSMILCompositor>::s_EnumStub (table=0x1ac069f0, entry=0x11bece58, number=1110, arg=0xbfffb2b8) at nsTHashtable.h:420
#14 0x005952a4 in PL_DHashTableEnumerate (table=0x1ac069f0, etor=0x13383938 <nsTHashtable<nsSMILCompositor>::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*)>, arg=0xbfffb2b8) at pldhash.c:754
#15 0x1338446f in nsTHashtable<nsSMILCompositor>::EnumerateEntries (this=0x1ac069f0, enumFunc=0x13382e2c <DoComposeAttribute(nsSMILCompositor*, void*)>, userArg=0x0) at nsTHashtable.h:241
#16 0x13382d88 in nsSMILAnimationController::DoSample (this=0x1e911400, aSkipUnchangedContainers=0) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/smil/nsSMILAnimationController.cpp:360
#17 0x132f25a1 in nsSMILAnimationController::Resample (this=0x1e911400) at nsSMILAnimationController.h:86
#18 0x132f25c0 in nsSMILAnimationController::FlushResampleRequests (this=0x1e911400) at nsSMILAnimationController.h:93
#19 0x132edbf2 in nsSVGElement::FlushAnimations (this=0x1e920310) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/svg/content/src/nsSVGElement.cpp:1792
#20 0x132ef1d8 in nsSVGElement::GetAnimatedLengthValues (this=0x1e920310, aFirst=0xbfffb46c) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/svg/content/src/nsSVGElement.cpp:1207
#21 0x13373b84 in nsSVGUseElement::PrependLocalTransformTo (this=0x1e920310, aMatrix=@0xbfffb510) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/svg/content/src/nsSVGUseElement.cpp:477
#22 0x132bedb4 in nsSVGGFrame::GetCanvasTM (this=0x21a2f4b8) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/svg/base/src/nsSVGGFrame.cpp:101
#23 0x132cfe07 in nsSVGPathGeometryFrame::GetCanvasTM (this=0x21a2f588) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/svg/base/src/nsSVGPathGeometryFrame.cpp:381
#24 0x132d008c in nsSVGPathGeometryFrame::GeneratePath (this=0x21a2f588, aContext=0xbfffb6c0, aOverrideTransform=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/svg/base/src/nsSVGPathGeometryFrame.cpp:490
#25 0x132d07ef in nsSVGPathGeometryFrame::UpdateCoveredRegion (this=0x21a2f588) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/svg/base/src/nsSVGPathGeometryFrame.cpp:254
#26 0x132ce065 in nsSVGOuterSVGFrame::UpdateAndInvalidateCoveredRegion (this=0x21a21300, aFrame=0x21a2f588) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/svg/base/src/nsSVGOuterSVGFrame.cpp:642
#27 0x132dbfd3 in nsSVGUtils::UpdateGraphic (aSVGFrame=0x21a2f5b8) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/svg/base/src/nsSVGUtils.cpp:684
#28 0x132d12d2 in nsSVGPathGeometryFrame::DidSetStyleContext (this=0x21a2f588, aOldStyleContext=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/svg/base/src/nsSVGPathGeometryFrame.cpp:97
#29 0x12a86a42 in nsFrame::Init (this=0x21a2f588, aContent=0x1ac009d0, aParent=0x21a2f4b8, aPrevInFlow=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/generic/nsFrame.cpp:366
#30 0x132c0024 in nsSVGGeometryFrame::Init (this=0x21a2f588, aContent=0x1ac009d0, aParent=0x21a2f4b8, aPrevInFlow=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/svg/base/src/nsSVGGeometryFrame.cpp:57
#31 0x129aeab3 in nsCSSFrameConstructor::InitAndRestoreFrame (this=0x1e90e260, aState=@0xbfffbf3c, aContent=0x1ac009d0, aParentFrame=0x21a2f4b8, aPrevInFlow=0x0, aNewFrame=0x21a2f588, aAllowCounters=1) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:4512
#32 0x129b5876 in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0x1e90e260, aItem=@0x1ac06a20, aState=@0xbfffbf3c, aParentFrame=0x21a2f4b8, aFrameItems=@0xbfffbdb8) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:3739
#33 0x129b6004 in nsCSSFrameConstructor::ConstructFramesFromItem (this=0x1e90e260, aState=@0xbfffbf3c, aIter=@0xbfffba94, aParentFrame=0x21a2f4b8, aFrameItems=@0xbfffbdb8) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:5411
#34 0x129b6195 in nsCSSFrameConstructor::ConstructFramesFromItemList (this=0x1e90e260, aState=@0xbfffbf3c, aItems=@0xbfffbc48, aParentFrame=0x21a2f4b8, aFrameItems=@0xbfffbdb8) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:8976
#35 0x129b6d50 in nsCSSFrameConstructor::ProcessChildren (this=0x1e90e260, aState=@0xbfffbf3c, aContent=0x1e920310, aStyleContext=0x21a2e438, aFrame=0x21a2f4b8, aCanHaveGeneratedContent=0, aFrameItems=@0xbfffbdb8, aAllowBlockStyles=0, aPendingBinding=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:9084
#36 0x129b5b1f in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0x1e90e260, aItem=@0x1ac00890, aState=@0xbfffbf3c, aParentFrame=0x21a213e0, aFrameItems=@0xbfffbfd8) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:3791
#37 0x129b6004 in nsCSSFrameConstructor::ConstructFramesFromItem (this=0x1e90e260, aState=@0xbfffbf3c, aIter=@0xbfffbeb4, aParentFrame=0x21a213e0, aFrameItems=@0xbfffbfd8) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:5411
#38 0x129b6195 in nsCSSFrameConstructor::ConstructFramesFromItemList (this=0x1e90e260, aState=@0xbfffbf3c, aItems=@0xbfffbf94, aParentFrame=0x21a213e0, aFrameItems=@0xbfffbfd8) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:8976
#39 0x129bd5af in nsCSSFrameConstructor::ContentInserted (this=0x1e90e260, aContainer=0x1e9120a0, aChild=0x1e920310, aIndexInContainer=75, aFrameState=0x1e918f00) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:6709
#40 0x129be9da in nsCSSFrameConstructor::RecreateFramesForContent (this=0x1e90e260, aContent=0x1e920310, aAsyncInsert=0) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:8595
#41 0x129c0198 in nsCSSFrameConstructor::RestyleElement (this=0x1e90e260, aContent=0x1e920310, aPrimaryFrame=0x21a2f4b8, aMinHint=nsChangeHint_ReconstructFrame) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:7555
#42 0x129c03b1 in nsCSSFrameConstructor::ProcessOneRestyle (this=0x1e90e260, aContent=0x1e920310, aRestyleHint=eReStyle_Self, aChangeHint=nsChangeHint_ReconstructFrame) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:11021

Do we need a separate bug on the fact that nsSVGPathGeometryFrame::DidSetStyleContext can cause notifying SetAttr calls?  If not, can we add a testcase for this so we don't accidentally regress it?

Comment 25

[Robert Longson [:longsonr]]

That was tracked as bug 483584. Not sure if the testcase in there needs checking in or whether that was covered by the tests in this bug though.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Ah, indeed.  I'll take this there.