1860622 - AutoRestore mCloningForSVGUse

Status: ASSIGNED

(Core :: SVG, enhancement)

Description

[Daniel Holbert [:dholbert]]

In a discussion about bug 1858792, emilio observed that for correctness sake, we might as well autorestore the old value of mCloningForSVGUse at the end of DoUpdateSVGUseElementShadowTrees -- so that if it was true due to some sort of recursion, we keep it true instead of prematurely setting it to false.

(This won't fix bug 1858792, but it'll reduce the potential for incorrect results in opt builds when we hit the set of conditions in bug 1858792.)

Filing this bug to do that.

Comment 1

[Robert Longson [:longsonr]]

We shouldn't recurse any more now that SMIL no longer flushes style.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson [:longsonr] from comment #1)

We shouldn't recurse any more now that SMIL no longer flushes style.

(That's via bug 1354393 it looks like, which fixed bug bug 1858792 -- thanks!)

Here's a code block for my own reference, since I had forgotten the relevant details here:
https://searchfox.org/mozilla-central/rev/1f27a4022f9f1269d897526c1c892a57743e650c/dom/base/Document.cpp#13680,13683-13684,13703-13704

void Document::DoUpdateSVGUseElementShadowTrees() {
...
  MOZ_ASSERT(!mCloningForSVGUse);
  mCloningForSVGUse = true;
...
  mCloningForSVGUse = false;
}

As long as we're still doing this^ pattern of temporarily setting this bool, it seems like we might as well use an AutoRestore as a belt-and-suspenders sort of thing. Even if we 100%-for-sure-never-do-recursion here, it's still useful to protect against early returns that forget to unset the bool, etc.

[EDIT: the following is wrong, ignore it, sorry]
Having said that, it does look like we're never using this mCloningForSVGUse variable or its accessor CloningForSVGUse() at all in opt builds -- we only check it in the assert above -- so maybe we can just remove this bool at this point?

Comment 3

[Daniel Holbert [:dholbert]]

(Er sorry, we do still have CloningForSVGUse() callers, so this bool is still useful; I made a mistake in my searchfox search. --> Edited my previous comment to remove my incorrect observation about it having been removed.)