Bug 1505875 - Clear out the ShadowRoot host pointer when unattaching it. r=smaug
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 08 Nov 2018 20:55:22 +0000
changeset 445207 853e4a06fb03aee6c1a732a8d594a3bfd14d97a2
parent 445206 94fed1a4a43b0c61a4851cc0624339cf55451ee8
child 445208 b246415f6864c04591ea05ca9e06e1cd7e6f9ca2
push id35012
push userbtara@mozilla.com
push dateFri, 09 Nov 2018 05:26:19 +0000
treeherdermozilla-central@63eb34f9b171 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1505875
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
dom/base/Element.cpp
dom/base/ShadowRoot.cpp
dom/base/ShadowRoot.h
dom/base/crashtests/1505875.html
dom/base/crashtests/crashtests.list
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -1324,35 +1324,34 @@ Element::AttachShadowWithoutNameChecks(S
    * 6. Return shadow.
    */
   return shadowRoot.forget();
 }
 
 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
 Element::GetAttribute(const nsAString& aName, DOMString& aReturn)
 {
   const nsAttrValue* val =
     mAttrs.GetAttr(aName,
                    IsHTMLElement() && IsInHTMLDocument() ?
--- a/dom/base/ShadowRoot.cpp
+++ b/dom/base/ShadowRoot.cpp
@@ -174,16 +174,27 @@ ShadowRoot::Unbind()
   for (nsIContent* child = GetFirstChild();
        child;
        child = child->GetNextSibling()) {
     child->UnbindFromTree(true, false);
   }
 }
 
 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)
 {
   MOZ_ASSERT(aElement);
   nsIDocument* doc = GetComposedDoc();
   if (!doc) {
     return;
   }
 
--- a/dom/base/ShadowRoot.h
+++ b/dom/base/ShadowRoot.h
@@ -93,16 +93,20 @@ public:
    */
   void CloneInternalDataFrom(ShadowRoot* aOther);
   void InsertSheetAt(size_t aIndex, StyleSheet&);
 
   // Calls UnbindFromTree for each of our kids, and also flags us as no longer
   // 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();
 
 private:
   void InsertSheetIntoAuthorData(size_t aIndex, StyleSheet&);
 
   void AppendStyleSheet(StyleSheet& aSheet)
new file mode 100644
--- /dev/null
+++ 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
@@ -240,8 +240,9 @@ skip-if(!browserIsRemote) pref(dom.webco
 pref(dom.webcomponents.shadowdom.enabled,true) load 1422883.html
 pref(dom.webcomponents.shadowdom.enabled,true) load 1428053.html
 pref(dom.webcomponents.customelements.enabled,true) load 1441029.html
 load 1449601.html
 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