Remove SVGUseElement::target_element_instance_

This reference should always be the same as the first child of the
shadow root. Since we don't make heavy use of it though - and it
shouldn't be too slow to access via the ShadowRoot, just get rid of it
to have less state to keep up-to-date.

Bug: 997176
Change-Id: I798bbc7ac64934d5eda1250b32150b50b4d2acdb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1771960Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#691722}

third_party/blink/renderer/core/svg/svg_use_element.cc

@@ -98,7 +98,6 @@ void SVGUseElement::Trace(blink::Visitor* visitor) {
   visitor->Trace(y_);
   visitor->Trace(width_);
   visitor->Trace(height_);
-  visitor->Trace(target_element_instance_);
   visitor->Trace(target_id_observer_);
   SVGGraphicsElement::Trace(visitor);
   SVGURIReference::Trace(visitor);
@@ -115,15 +114,13 @@ static inline bool IsWellFormedDocument(const Document& document) {
 
 Node::InsertionNotificationRequest SVGUseElement::InsertedInto(
     ContainerNode& root_parent) {
-  // This functions exists to assure assumptions made in the code regarding
-  // SVGElementInstance creation/destruction are satisfied.
   SVGGraphicsElement::InsertedInto(root_parent);
-  if (!root_parent.isConnected())
-    return kInsertionDone;
+  if (root_parent.isConnected()) {
+    InvalidateShadowTree();
 #if DCHECK_IS_ON()
-  DCHECK(!target_element_instance_ || !IsWellFormedDocument(GetDocument()));
+    DCHECK(!InstanceRoot() || !IsWellFormedDocument(GetDocument()));
 #endif
-  InvalidateShadowTree();
+  }
   return kInsertionDone;
 }
 
@@ -223,11 +220,10 @@ void SVGUseElement::SvgAttributeChanged(const QualifiedName& attr_name) {
     }
 
     UpdateRelativeLengthsInformation();
-    if (target_element_instance_) {
-      DCHECK(target_element_instance_->CorrespondingElement());
-      TransferUseWidthAndHeightIfNeeded(
-          *this, *target_element_instance_,
-          *target_element_instance_->CorrespondingElement());
+    if (SVGElement* instance_root = InstanceRoot()) {
+      DCHECK(instance_root->CorrespondingElement());
+      TransferUseWidthAndHeightIfNeeded(*this, *instance_root,
+                                        *instance_root->CorrespondingElement());
     }
 
     if (LayoutObject* object = GetLayoutObject())
@@ -284,13 +280,8 @@ void SVGUseElement::CancelShadowTreeRecreation() {
   GetDocument().UnscheduleUseShadowTreeUpdate(*this);
 }
 
-void SVGUseElement::ClearInstanceRoot() {
-  target_element_instance_ = nullptr;
-}
-
 void SVGUseElement::ClearResourceReference() {
   UnobserveTarget(target_id_observer_);
-  ClearInstanceRoot();
   RemoveAllOutgoingReferences();
 }
 
@@ -314,6 +305,14 @@ Element* SVGUseElement::ResolveTargetElement(ObserveBehavior observe_behavior) {
       ->getElementById(element_identifier);
 }
 
+SVGElement* SVGUseElement::InstanceRoot() const {
+  // The shadow tree is torn down lazily, so check if there's a pending rebuild
+  // or if we're disconnected from the document.
+  if (needs_shadow_tree_recreation_ || !isConnected())
+    return nullptr;
+  return To<SVGElement>(UseShadowRoot().firstChild());
+}
+
 void SVGUseElement::BuildPendingResource() {
   // This runs just before computed style is updated, so this SVGUseElement
   // should always be connected to the Document. It should also not be an
@@ -342,9 +341,9 @@ String SVGUseElement::title() const {
 
   // If there is no <title> child in <use>, we lookup first <title> child in
   // shadow tree.
-  if (target_element_instance_) {
+  if (SVGElement* instance_root = InstanceRoot()) {
     if (Element* title_element =
-            Traversal<SVGTitleElement>::FirstChild(*target_element_instance_))
+            Traversal<SVGTitleElement>::FirstChild(*instance_root))
       return title_element->innerText();
   }
   // Otherwise return a null string.
@@ -424,7 +423,7 @@ SVGElement* SVGUseElement::CreateInstanceTree(SVGElement& target_root) const {
 }
 
 void SVGUseElement::AttachShadowTree(SVGElement& target) {
-  DCHECK(!target_element_instance_);
+  DCHECK(!InstanceRoot());
   DCHECK(!needs_shadow_tree_recreation_);
   DCHECK(!InUseShadowTree());
 
@@ -435,9 +434,7 @@ void SVGUseElement::AttachShadowTree(SVGElement& target) {
   // Set up root SVG element in shadow tree.
   // Clone the target subtree into the shadow tree, not handling <use> and
   // <symbol> yet.
-  target_element_instance_ = CreateInstanceTree(target);
-  ShadowRoot& shadow_root = UseShadowRoot();
-  shadow_root.AppendChild(target_element_instance_);
+  UseShadowRoot().AppendChild(CreateInstanceTree(target));
 
   AddReferencesToFirstDegreeNestedUseElements(target);
 
@@ -449,19 +446,14 @@ void SVGUseElement::AttachShadowTree(SVGElement& target) {
   }
 
   // Assure shadow tree building was successful.
-  DCHECK(target_element_instance_);
-  DCHECK_EQ(target_element_instance_->CorrespondingUseElement(), this);
-  DCHECK_EQ(target_element_instance_->CorrespondingElement(), &target);
+  DCHECK(InstanceRoot());
+  DCHECK_EQ(InstanceRoot()->CorrespondingUseElement(), this);
+  DCHECK_EQ(InstanceRoot()->CorrespondingElement(), &target);
 
   // Expand all <use> elements in the shadow tree.
   // Expand means: replace the actual <use> element by what it references.
   ExpandUseElementsInShadowTree();
 
-  // If the instance root was a <use>, it could have been replaced now, so
-  // reset |m_targetElementInstance|.
-  target_element_instance_ = To<SVGElement>(shadow_root.firstChild());
-  DCHECK_EQ(target_element_instance_->parentNode(), shadow_root);
-
   PostProcessInstanceTree();
 
   // Update relative length information.
@@ -510,8 +502,7 @@ Path SVGUseElement::ToClipPath() const {
 
 SVGGraphicsElement* SVGUseElement::VisibleTargetGraphicsElementForClipping()
     const {
-  auto* svg_graphics_element =
-      DynamicTo<SVGGraphicsElement>(UseShadowRoot().firstChild());
+  auto* svg_graphics_element = DynamicTo<SVGGraphicsElement>(InstanceRoot());
   if (!svg_graphics_element)
     return nullptr;
 
@@ -637,7 +628,6 @@ void SVGUseElement::ExpandUseElementsInShadowTree() {
 void SVGUseElement::InvalidateShadowTree() {
   if (!InActiveDocument() || needs_shadow_tree_recreation_)
     return;
-  ClearInstanceRoot();
   ScheduleShadowTreeRecreation();
   InvalidateDependentShadowTrees();
 }
@@ -661,11 +651,8 @@ bool SVGUseElement::SelfHasRelativeLengths() const {
       width_->CurrentValue()->IsRelative() ||
       height_->CurrentValue()->IsRelative())
     return true;
-
-  if (!target_element_instance_)
-    return false;
-
-  return target_element_instance_->HasRelativeLengths();
+  SVGElement* instance_root = InstanceRoot();
+  return instance_root && instance_root->HasRelativeLengths();
 }
 
 FloatRect SVGUseElement::GetBBox() {

third_party/blink/renderer/core/svg/svg_use_element.h

@@ -22,6 +22,8 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_SVG_SVG_USE_ELEMENT_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_SVG_SVG_USE_ELEMENT_H_
 
+#include "base/gtest_prod_util.h"
+#include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/loader/resource/document_resource.h"
 #include "third_party/blink/renderer/core/svg/svg_animated_length.h"
 #include "third_party/blink/renderer/core/svg/svg_geometry_element.h"
@@ -105,7 +107,7 @@ class SVGUseElement final : public SVGGraphicsElement,
   Element* ResolveTargetElement(ObserveBehavior);
   void AttachShadowTree(SVGElement& target);
   void DetachShadowTree();
-  void ClearInstanceRoot();
+  CORE_EXPORT SVGElement* InstanceRoot() const;
   SVGElement* CreateInstanceTree(SVGElement& target_root) const;
   void ClearResourceReference();
   bool HasCycleUseReferencing(const ContainerNode& target_instance,
@@ -132,8 +134,12 @@ class SVGUseElement final : public SVGGraphicsElement,
   bool element_url_is_local_;
   bool have_fired_load_event_;
   bool needs_shadow_tree_recreation_;
-  Member<SVGElement> target_element_instance_;
   Member<IdTargetObserver> target_id_observer_;
+
+  FRIEND_TEST_ALL_PREFIXES(SVGUseElementTest,
+                           NullInstanceRootWhenNotConnectedToDocument);
+  FRIEND_TEST_ALL_PREFIXES(SVGUseElementTest,
+                           NullInstanceRootWhenShadowTreePendingRebuild);
 };
 
 }  // namespace blink

third_party/blink/renderer/core/svg/svg_use_element_test.cc

@@ -74,4 +74,44 @@ TEST_F(SVGUseElementTest,
   ASSERT_FALSE(use->GetShadowRoot()->getElementById("target"));
 }
 
+TEST_F(SVGUseElementTest, NullInstanceRootWhenNotConnectedToDocument) {
+  GetDocument().body()->SetInnerHTMLFromString(R"HTML(
+    <svg>
+      <defs>
+        <rect id="r" width="100" height="100" fill="blue"/>
+      </defs>
+      <use id="target" href="#r"/>
+    </svg>
+  )HTML");
+  GetDocument().View()->UpdateAllLifecyclePhases(LifecycleUpdateReason::kTest);
+
+  auto* target = To<SVGUseElement>(GetDocument().getElementById("target"));
+  ASSERT_TRUE(target);
+  ASSERT_TRUE(target->InstanceRoot());
+
+  target->remove();
+
+  ASSERT_FALSE(target->InstanceRoot());
+}
+
+TEST_F(SVGUseElementTest, NullInstanceRootWhenShadowTreePendingRebuild) {
+  GetDocument().body()->SetInnerHTMLFromString(R"HTML(
+    <svg>
+      <defs>
+        <rect id="r" width="100" height="100" fill="blue"/>
+      </defs>
+      <use id="target" href="#r"/>
+    </svg>
+  )HTML");
+  GetDocument().View()->UpdateAllLifecyclePhases(LifecycleUpdateReason::kTest);
+
+  auto* target = To<SVGUseElement>(GetDocument().getElementById("target"));
+  ASSERT_TRUE(target);
+  ASSERT_TRUE(target->InstanceRoot());
+
+  GetDocument().getElementById("r")->setAttribute(html_names::kWidthAttr, "50");
+
+  ASSERT_FALSE(target->InstanceRoot());
+}
+
 }  // namespace blink