Bug 1505875 - Clear out the ShadowRoot host pointer when unattaching it. r=smaug

As expected, this is specific to the UA widget stuff. What's going on here is that given we don't clear out the host when unattaching the shadow tree, mutating that shadow tree still notifies all the way up to the document, and that gets all the other code confused, thinking that the node is connected. Indeed, the first assertion that fails when loading that test-case in a debug build is: https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/dom/base/nsNodeUtils.cpp#93 This seems the best fix to avoid confusion. Also clear the mutation observer, to completely forget about the host. Chrome code dealing with UA widgets needs to be careful, but I think this is safe. All the code that assumes that GetHost() doesn't return null is in code dealing with connected shadow trees only (style system / layout), or in mutation observer notifications from the host. Differential Revision: https://phabricator.services.mozilla.com/D11369 --HG-- extra : moz-landing-system : lando
2018-11-08 20:55:22 +00:00 · 2018-11-08 20:55:22 +00:00 · 80bd2cd013
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@ -1329,25 +1329,24 @@ Element::AttachShadowWithoutNameChecks(ShadowRootMode aMode)
 void
 Element::UnattachShadow()
 {
-  RefPtr<ShadowRoot> shadowRoot = GetShadowRoot();
+  ShadowRoot* shadowRoot = GetShadowRoot();
  if (!shadowRoot) {
    return;
  }

  nsAutoScriptBlocker scriptBlocker;

-  nsIDocument* doc = GetComposedDoc();
-  if (doc) {
+  if (nsIDocument* doc = GetComposedDoc()) {
    if (nsIPresShell* shell = doc->GetShell()) {
      shell->DestroyFramesForAndRestyle(this);
    }
  }
  MOZ_ASSERT(!GetPrimaryFrame());

-  // Simply unhook the shadow root from the element.
-  MOZ_ASSERT(!shadowRoot->HasSlots(), "Won't work when shadow root has slots!");
-  shadowRoot->Unbind();
+  shadowRoot->Unattach();
  SetShadowRoot(nullptr);
+
+  // Beware shadowRoot could be dead after this call.
 }

 void
--- a/dom/base/ShadowRoot.cpp
+++ b/dom/base/ShadowRoot.cpp
@ -178,6 +178,17 @@ ShadowRoot::Unbind()
  }
 }

+void
+ShadowRoot::Unattach()
+{
+  MOZ_ASSERT(!HasSlots(), "Won't work!");
+  MOZ_ASSERT(IsUAWidget());
+  MOZ_ASSERT(GetHost());
+  Unbind();
+  GetHost()->RemoveMutationObserver(this);
+  SetHost(nullptr);
+}
+
 void
 ShadowRoot::InvalidateStyleAndLayoutOnSubtree(Element* aElement)
 {
--- a/dom/base/ShadowRoot.h
+++ b/dom/base/ShadowRoot.h
@ -98,6 +98,10 @@ public:
  // being connected.
  void Unbind();

+  // Only intended for UA widgets. Forget our shadow host and unbinds all our
+  // kids.
+  void Unattach();
+
  // Calls BindToTree on each of our kids, and also maybe flags us as being
  // connected.
  nsresult Bind();
--- a/dom/base/crashtests/1505875.html
+++ b/dom/base/crashtests/1505875.html
@ -0,0 +1,10 @@
+<script>
+function go() {
+  var r = document.getSelection().getRangeAt(0).cloneRange();
+  a.type = "";
+  r.insertNode(b);
+}
+</script>
+<body onload=go()>
+<input id="a" autofocus type="date">
+<summary id="b">x</summary>
--- a/dom/base/crashtests/crashtests.list
+++ b/dom/base/crashtests/crashtests.list
@ -245,3 +245,4 @@ load 1445670.html
 load 1458016.html
 pref(dom.webcomponents.shadowdom.enabled,true) load 1459688.html
 load 1460794.html
+pref(dom.webcomponents.shadowdom.enabled,true) load 1505875.html