Bug 1510848 - Do not unattach UA Widget Shadow Root if the element is already re-attached to the tree r=emilio,smaug

This patch moves all UA Widget calls to helper functions in Element.cpp. The helper function AttachAndSetUAShadowRoot sets the shadow root in a runnable, so that it is in the same order of NotifyUAWidget* runnables.

Differential Revision: https://phabricator.services.mozilla.com/D13479

--HG--
extra : moz-landing-system : lando

dom/base/Element.cpp

@@ -1209,6 +1209,73 @@ already_AddRefed<ShadowRoot> Element::AttachShadowWithoutNameChecks(
   return shadowRoot.forget();
 }
 
+void Element::AttachAndSetUAShadowRoot() {
+  MOZ_DIAGNOSTIC_ASSERT(!CanAttachShadowDOM(),
+                        "Cannot be used to attach UI shadow DOM");
+
+  // Attach the UA Widget Shadow Root in a runnable so that the code runs
+  // in the same order of NotifyUAWidget* calls.
+  nsContentUtils::AddScriptRunner(NS_NewRunnableFunction(
+      "Element::AttachAndSetUAShadowRoot::Runnable",
+      [self = RefPtr<Element>(this)]() {
+        if (self->GetShadowRoot()) {
+          MOZ_ASSERT(self->GetShadowRoot()->IsUAWidget());
+          return;
+        }
+
+        RefPtr<ShadowRoot> shadowRoot =
+            self->AttachShadowWithoutNameChecks(ShadowRootMode::Closed);
+        shadowRoot->SetIsUAWidget();
+      }));
+}
+
+void Element::NotifyUAWidgetSetupOrChange() {
+  MOZ_ASSERT(IsInComposedDoc());
+  // Schedule a runnable, ensure the event dispatches before
+  // returning to content script.
+  // This event cause UA Widget to construct or cause onattributechange callback
+  // of existing UA Widget to run; dispatching this event twice should not cause
+  // UA Widget to re-init.
+  nsContentUtils::AddScriptRunner(NS_NewRunnableFunction(
+      "Element::NotifyUAWidgetSetupOrChange::UAWidgetSetupOrChange",
+      [self = RefPtr<Element>(this),
+       ownerDoc = RefPtr<nsIDocument>(OwnerDoc())]() {
+        MOZ_ASSERT(self->GetShadowRoot() &&
+                   self->GetShadowRoot()->IsUAWidget());
+
+        nsContentUtils::DispatchChromeEvent(
+            ownerDoc, self, NS_LITERAL_STRING("UAWidgetSetupOrChange"),
+            CanBubble::eYes, Cancelable::eNo);
+      }));
+}
+
+void Element::NotifyUAWidgetTeardown(UnattachShadowRoot aUnattachShadowRoot) {
+  MOZ_ASSERT(IsInComposedDoc());
+  // The runnable will dispatch an event to tear down UA Widget,
+  // and unattach the Shadow Root.
+  nsContentUtils::AddScriptRunner(NS_NewRunnableFunction(
+      "Element::NotifyUAWidgetTeardownAndUnattachShadow::UAWidgetTeardown",
+      [aUnattachShadowRoot, self = RefPtr<Element>(this),
+       ownerDoc = RefPtr<nsIDocument>(OwnerDoc())]() {
+        if (!self->GetShadowRoot()) {
+          // No UA Widget Shadow Root was ever attached.
+          return;
+        }
+        MOZ_ASSERT(self->GetShadowRoot()->IsUAWidget());
+
+        nsresult rv = nsContentUtils::DispatchChromeEvent(
+            ownerDoc, self, NS_LITERAL_STRING("UAWidgetTeardown"),
+            CanBubble::eYes, Cancelable::eNo);
+        if (NS_WARN_IF(NS_FAILED(rv))) {
+          return;
+        }
+
+        if (aUnattachShadowRoot == UnattachShadowRoot::Yes) {
+          self->UnattachShadow();
+        }
+      }));
+}
+
 void Element::UnattachShadow() {
   ShadowRoot* shadowRoot = GetShadowRoot();
   if (!shadowRoot) {

dom/base/Element.h

@@ -1214,6 +1214,23 @@ class Element : public FragmentOrElement {
 
   already_AddRefed<ShadowRoot> AttachShadowWithoutNameChecks(
       ShadowRootMode aMode);
+
+  // Attach UA Shadow Root if it is not attached.
+  void AttachAndSetUAShadowRoot();
+
+  // Dispatch an event to UAWidgetsChild, triggering construction
+  // or onattributechange callback on the existing widget.
+  void NotifyUAWidgetSetupOrChange();
+
+  enum class UnattachShadowRoot {
+    No,
+    Yes,
+  };
+
+  // Dispatch an event to UAWidgetsChild, triggering UA Widget destruction.
+  // and optionally remove the shadow root.
+  void NotifyUAWidgetTeardown(UnattachShadowRoot = UnattachShadowRoot::Yes);
+
   void UnattachShadow();
 
   ShadowRoot* GetShadowRootByMode() const;

dom/base/nsObjectLoadingContent.cpp

@@ -598,16 +598,8 @@ void nsObjectLoadingContent::UnbindFromTree(bool aDeep, bool aNullParent) {
   }
 
   // Unattach plugin problem UIWidget if any.
-  if (thisElement->IsInComposedDoc() && thisElement->GetShadowRoot()) {
+  if (thisElement->IsInComposedDoc() && nsContentUtils::IsUAWidgetEnabled()) {
-    nsContentUtils::AddScriptRunner(NS_NewRunnableFunction(
+    thisElement->NotifyUAWidgetTeardown();
-        "nsObjectLoadingContent::UnbindFromTree::UAWidgetUnbindFromTree",
-        [thisElement]() {
-          nsContentUtils::DispatchChromeEvent(
-              thisElement->OwnerDoc(), thisElement,
-              NS_LITERAL_STRING("UAWidgetUnbindFromTree"), CanBubble::eYes,
-              Cancelable::eNo);
-          thisElement->UnattachShadow();
-        }));
   }
 
   if (mType == eType_Plugin) {
@@ -2591,22 +2583,10 @@ void nsObjectLoadingContent::NotifyStateChanged(ObjectType aOldType,
       bool hasProblemState = !(newState & pluginProblemState).IsEmpty();
 
       if (hadProblemState && !hasProblemState) {
-        nsContentUtils::AddScriptRunner(NS_NewRunnableFunction(
+        thisEl->NotifyUAWidgetTeardown();
-            "nsObjectLoadingContent::UnbindFromTree::UAWidgetUnbindFromTree",
-            [thisEl]() {
-              nsContentUtils::DispatchChromeEvent(
-                  thisEl->OwnerDoc(), thisEl,
-                  NS_LITERAL_STRING("UAWidgetUnbindFromTree"), CanBubble::eYes,
-                  Cancelable::eNo);
-              thisEl->UnattachShadow();
-            }));
       } else if (!hadProblemState && hasProblemState) {
-        nsGenericHTMLElement::FromNode(thisEl)->AttachAndSetUAShadowRoot();
+        thisEl->AttachAndSetUAShadowRoot();
-
+        thisEl->NotifyUAWidgetSetupOrChange();
-        AsyncEventDispatcher* dispatcher = new AsyncEventDispatcher(
-            thisEl, NS_LITERAL_STRING("UAWidgetBindToTree"), CanBubble::eYes,
-            ChromeOnlyDispatch::eYes);
-        dispatcher->RunDOMEventWhenSafe();
       }
     }
   } else if (aOldType != mType) {

dom/html/HTMLInputElement.cpp

@@ -4337,10 +4337,7 @@ nsresult HTMLInputElement::BindToTree(nsIDocument* aDocument,
       nsContentUtils::IsUAWidgetEnabled() && IsInComposedDoc()) {
     // Construct Shadow Root so web content can be hidden in the DOM.
     AttachAndSetUAShadowRoot();
-    AsyncEventDispatcher* dispatcher =
+    NotifyUAWidgetSetupOrChange();
-        new AsyncEventDispatcher(this, NS_LITERAL_STRING("UAWidgetBindToTree"),
-                                 CanBubble::eYes, ChromeOnlyDispatch::eYes);
-    dispatcher->RunDOMEventWhenSafe();
   }
 
   if (mType == NS_FORM_INPUT_PASSWORD) {
@@ -4369,16 +4366,9 @@ void HTMLInputElement::UnbindFromTree(bool aDeep, bool aNullParent) {
     WillRemoveFromRadioGroup();
   }
 
-  if (GetShadowRoot() && IsInComposedDoc()) {
+  if ((mType == NS_FORM_INPUT_TIME || mType == NS_FORM_INPUT_DATE) &&
-    nsContentUtils::AddScriptRunner(NS_NewRunnableFunction(
+      nsContentUtils::IsUAWidgetEnabled() && IsInComposedDoc()) {
-        "HTMLInputElement::UnbindFromTree::UAWidgetUnbindFromTree",
+    NotifyUAWidgetTeardown();
-        [self = RefPtr<Element>(this)]() {
-          nsContentUtils::DispatchChromeEvent(
-              self->OwnerDoc(), self,
-              NS_LITERAL_STRING("UAWidgetUnbindFromTree"), CanBubble::eYes,
-              Cancelable::eNo);
-          self->UnattachShadow();
-        }));
   }
 
   nsImageLoadingContent::UnbindFromTree(aDeep, aNullParent);
@@ -4557,30 +4547,15 @@ void HTMLInputElement::HandleTypeChange(uint8_t aNewType, bool aNotify) {
     if (oldType == NS_FORM_INPUT_TIME || oldType == NS_FORM_INPUT_DATE) {
       if (mType != NS_FORM_INPUT_TIME && mType != NS_FORM_INPUT_DATE) {
         // Switch away from date/time type.
-        RefPtr<Element> self = this;
+        NotifyUAWidgetTeardown();
-        nsContentUtils::AddScriptRunner(NS_NewRunnableFunction(
-            "HTMLInputElement::UnbindFromTree::UAWidgetUnbindFromTree",
-            [self]() {
-              nsContentUtils::DispatchChromeEvent(
-                  self->OwnerDoc(), self,
-                  NS_LITERAL_STRING("UAWidgetUnbindFromTree"), CanBubble::eYes,
-                  Cancelable::eNo);
-              self->UnattachShadow();
-            }));
       } else {
         // Switch between date and time.
-        AsyncEventDispatcher* dispatcher = new AsyncEventDispatcher(
+        NotifyUAWidgetSetupOrChange();
-            this, NS_LITERAL_STRING("UAWidgetAttributeChanged"),
-            CanBubble::eYes, ChromeOnlyDispatch::eYes);
-        dispatcher->RunDOMEventWhenSafe();
       }
     } else if (mType == NS_FORM_INPUT_TIME || mType == NS_FORM_INPUT_DATE) {
       // Switch to date/time type.
       AttachAndSetUAShadowRoot();
-      AsyncEventDispatcher* dispatcher = new AsyncEventDispatcher(
+      NotifyUAWidgetSetupOrChange();
-          this, NS_LITERAL_STRING("UAWidgetBindToTree"), CanBubble::eYes,
-          ChromeOnlyDispatch::eYes);
-      dispatcher->RunDOMEventWhenSafe();
     }
   }
 }

dom/html/HTMLMarqueeElement.cpp

@@ -61,21 +61,17 @@ nsresult HTMLMarqueeElement::BindToTree(nsIDocument* aDocument,
 
   if (nsContentUtils::IsUAWidgetEnabled() && IsInComposedDoc()) {
     AttachAndSetUAShadowRoot();
-    AsyncEventDispatcher* dispatcher =
+    NotifyUAWidgetSetupOrChange();
-        new AsyncEventDispatcher(this, NS_LITERAL_STRING("UAWidgetBindToTree"),
-                                 CanBubble::eYes, ChromeOnlyDispatch::eYes);
-    dispatcher->RunDOMEventWhenSafe();
   }
 
   return rv;
 }
 
 void HTMLMarqueeElement::UnbindFromTree(bool aDeep, bool aNullParent) {
-  if (GetShadowRoot() && IsInComposedDoc()) {
+  if (nsContentUtils::IsUAWidgetEnabled() && IsInComposedDoc()) {
-    AsyncEventDispatcher* dispatcher = new AsyncEventDispatcher(
+    // We don't want to unattach the shadow root because it used to
-        this, NS_LITERAL_STRING("UAWidgetUnbindFromTree"), CanBubble::eYes,
+    // contain a <slot>.
-        ChromeOnlyDispatch::eYes);
+    NotifyUAWidgetTeardown(UnattachShadowRoot::No);
-    dispatcher->RunDOMEventWhenSafe();
   }
 
   nsGenericHTMLElement::UnbindFromTree(aDeep, aNullParent);
@@ -140,10 +136,7 @@ nsresult HTMLMarqueeElement::AfterSetAttr(int32_t aNameSpaceID, nsAtom* aName,
                                           bool aNotify) {
   if (nsContentUtils::IsUAWidgetEnabled() && IsInComposedDoc() &&
       aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::direction) {
-    AsyncEventDispatcher* dispatcher = new AsyncEventDispatcher(
+    NotifyUAWidgetSetupOrChange();
-        this, NS_LITERAL_STRING("UAWidgetAttributeChanged"), CanBubble::eYes,
-        ChromeOnlyDispatch::eYes);
-    dispatcher->RunDOMEventWhenSafe();
   }
   return nsGenericHTMLElement::AfterSetAttr(
       aNameSpaceID, aName, aValue, aOldValue, aMaybeScriptedPrincipal, aNotify);

dom/html/HTMLMediaElement.cpp

@@ -4000,12 +4000,7 @@ nsresult HTMLMediaElement::AfterSetAttr(int32_t aNameSpaceID, nsAtom* aName,
       }
     } else if (nsContentUtils::IsUAWidgetEnabled() &&
                aName == nsGkAtoms::controls && IsInComposedDoc()) {
-      AsyncEventDispatcher* dispatcher = new AsyncEventDispatcher(
+      NotifyUAWidgetSetupOrChange();
-          this, NS_LITERAL_STRING("UAWidgetAttributeChanged"), CanBubble::eYes,
-          ChromeOnlyDispatch::eYes);
-      // This has to happen at this tick so that UA Widget could respond
-      // before returning to content script.
-      dispatcher->RunDOMEventWhenSafe();
     }
   }
 
@@ -4047,10 +4042,7 @@ nsresult HTMLMediaElement::BindToTree(nsIDocument* aDocument,
     // Construct Shadow Root so web content can be hidden in the DOM.
     AttachAndSetUAShadowRoot();
 #ifdef ANDROID
-    AsyncEventDispatcher* dispatcher =
+    NotifyUAWidgetSetupOrChange();
-        new AsyncEventDispatcher(this, NS_LITERAL_STRING("UAWidgetBindToTree"),
-                                 CanBubble::eYes, ChromeOnlyDispatch::eYes);
-    dispatcher->RunDOMEventWhenSafe();
 #else
     // We don't want to call into JS if the website never asks for native
     // video controls.
@@ -4059,10 +4051,7 @@ nsresult HTMLMediaElement::BindToTree(nsIDocument* aDocument,
     // This only applies to Desktop because on Fennec we would need to show
     // an UI if the video is blocked.
     if (Controls()) {
-      AsyncEventDispatcher* dispatcher = new AsyncEventDispatcher(
+      NotifyUAWidgetSetupOrChange();
-          this, NS_LITERAL_STRING("UAWidgetBindToTree"), CanBubble::eYes,
-          ChromeOnlyDispatch::eYes);
-      dispatcher->RunDOMEventWhenSafe();
     }
 #endif
   }
@@ -4283,16 +4272,8 @@ void HTMLMediaElement::UnbindFromTree(bool aDeep, bool aNullParent) {
   mUnboundFromTree = true;
   mVisibilityState = Visibility::UNTRACKED;
 
-  if (GetShadowRoot() && IsInComposedDoc()) {
+  if (nsContentUtils::IsUAWidgetEnabled() && IsInComposedDoc()) {
-    nsContentUtils::AddScriptRunner(NS_NewRunnableFunction(
+    NotifyUAWidgetTeardown();
-        "HTMLMediaElement::UnbindFromTree::UAWidgetUnbindFromTree",
-        [self = RefPtr<Element>(this)]() {
-          nsContentUtils::DispatchChromeEvent(
-              self->OwnerDoc(), self,
-              NS_LITERAL_STRING("UAWidgetUnbindFromTree"), CanBubble::eYes,
-              Cancelable::eNo);
-          self->UnattachShadow();
-        }));
   }
 
   nsGenericHTMLElement::UnbindFromTree(aDeep, aNullParent);

dom/html/nsGenericHTMLElement.cpp

@@ -2569,19 +2569,6 @@ bool nsGenericHTMLElement::IsEventAttributeNameInternal(nsAtom* aName) {
   return nsContentUtils::IsEventAttributeName(aName, EventNameType_HTML);
 }
 
-void nsGenericHTMLElement::AttachAndSetUAShadowRoot() {
-  MOZ_DIAGNOSTIC_ASSERT(!CanAttachShadowDOM(),
-                        "Cannot be used to attach UI shadow DOM");
-  if (GetShadowRoot()) {
-    MOZ_ASSERT(GetShadowRoot()->IsUAWidget());
-    return;
-  }
-
-  RefPtr<ShadowRoot> shadowRoot =
-      AttachShadowWithoutNameChecks(ShadowRootMode::Closed);
-  shadowRoot->SetIsUAWidget();
-}
-
 /**
  * Construct a URI from a string, as an element.src attribute
  * would be set to. Helper for the media elements.

dom/html/nsGenericHTMLElement.h

@@ -226,9 +226,6 @@ class nsGenericHTMLElement : public nsGenericHTMLElementBase {
     return IsNodeInternal(aFirst, aArgs...);
   }
 
-  // Attach UA Shadow Root if it is not attached.
-  void AttachAndSetUAShadowRoot();
-
  protected:
   virtual ~nsGenericHTMLElement() {}
 

dom/media/tests/crashtests/1510848.html

@@ -0,0 +1,4 @@
+<a>
+<dd>
+<video>
+<a>

dom/media/tests/crashtests/crashtests.list

@@ -26,3 +26,4 @@ load 1453030.html
 skip-if(Android) load 1490700.html # No screenshare on Android
 load 1505957.html
 load 1511130.html
+load 1510848.html

layout/base/nsCSSFrameConstructor.cpp

@@ -1689,7 +1689,6 @@ void nsCSSFrameConstructor::CreateGeneratedContentItem(
                        aParentFrame->IsDateTimeControlFrame())) {
     // Video frames and date time control frames may not be leafs when backed by
     // an UA widget, but we still don't want to expose generated content.
-    MOZ_ASSERT(aOriginatingElement.GetShadowRoot()->IsUAWidget());
     return;
   }
 

mobile/android/chrome/geckoview/GeckoViewMediaChild.js

@@ -28,7 +28,7 @@ class GeckoViewMediaChild extends GeckoViewChildModule {
 
   onEnable() {
     debug `onEnable`;
-    addEventListener("UAWidgetBindToTree", this, false);
+    addEventListener("UAWidgetSetupOrChange", this, false);
     addEventListener("MozDOMFullscreen:Entered", this, false);
     addEventListener("MozDOMFullscreen:Exited", this, false);
     addEventListener("pagehide", this, false);
@@ -46,7 +46,7 @@ class GeckoViewMediaChild extends GeckoViewChildModule {
   onDisable() {
     debug `onDisable`;
 
-    removeEventListener("UAWidgetBindToTree", this);
+    removeEventListener("UAWidgetSetupOrChange", this);
     removeEventListener("MozDOMFullscreen:Entered", this);
     removeEventListener("MozDOMFullscreen:Exited", this);
     removeEventListener("pagehide", this);
@@ -103,7 +103,7 @@ class GeckoViewMediaChild extends GeckoViewChildModule {
     debug `handleEvent: ${aEvent.type}`;
 
     switch (aEvent.type) {
-      case "UAWidgetBindToTree":
+      case "UAWidgetSetupOrChange":
         this.handleNewMedia(aEvent.composedTarget);
         break;
       case "MozDOMFullscreen:Entered":

toolkit/actors/UAWidgetsChild.jsm

@@ -18,11 +18,10 @@ class UAWidgetsChild extends ActorChild {
 
   handleEvent(aEvent) {
     switch (aEvent.type) {
-      case "UAWidgetBindToTree":
+      case "UAWidgetSetupOrChange":
-      case "UAWidgetAttributeChanged":
         this.setupOrNotifyWidget(aEvent.target);
         break;
-      case "UAWidgetUnbindFromTree":
+      case "UAWidgetTeardown":
         this.teardownWidget(aEvent.target);
         break;
     }
@@ -72,12 +71,13 @@ class UAWidgetsChild extends ActorChild {
     }
 
     if (!uri || !widgetName) {
+      Cu.reportError("Getting a UAWidgetSetupOrChange event on undefined element.");
       return;
     }
 
     let shadowRoot = aElement.openOrClosedShadowRoot;
     if (!shadowRoot) {
-      Cu.reportError("Getting a UAWidgetBindToTree/UAWidgetAttributeChanged event without the Shadow Root.");
+      Cu.reportError("Getting a UAWidgetSetupOrChange event without the Shadow Root.");
       return;
     }
 

toolkit/content/widgets/docs/ua_widget.rst

@@ -11,17 +11,17 @@ UA Widget lifecycle
 
 UA Widgets are generally constructed when the element is appended to the document and destroyed when the element is removed from the tree. Yet, in order to be fast, specialization was made to each of the widgets.
 
-When the element is appended to the tree, a chrome-only ``UAWidgetBindToTree`` event is dispatched and is caught by a frame script, namely UAWidgetsChild.
+When the element is appended to the tree, a chrome-only ``UAWidgetSetupOrChange`` event is dispatched and is caught by a frame script, namely UAWidgetsChild.
 
 UAWidgetsChild then grabs the sandbox for that origin (lazily creating it as needed), loads the script as needed, and initializes an instance by calling the JS constructor with a reference to the UA Widget Shadow Root created by the DOM. We will discuss the sandbox in the latter section.
 
 The ``onsetup`` method is called right after the instance is constructed. The call to constructor must not throw, or UAWidgetsChild will be confused since an instance of the widget will not be returned, but the widget is already half-initalized. If the ``onsetup`` method call throws, UAWidgetsChild will still be able to hold the reference of the widget and call the destructor later on.
 
-When the element is removed from the tree, ``UAWidgetUnbindFromTree`` is dispatched so UAWidgetsChild can destroy the widget, if it exists. If so, the UAWidgetsChild calls the ``destructor()`` method on the widget, causing the widget to destruct itself.
+When the element is removed from the tree, ``UAWidgetTeardown`` is dispatched so UAWidgetsChild can destroy the widget, if it exists. If so, the UAWidgetsChild calls the ``destructor()`` method on the widget, causing the widget to destruct itself.
 
 When a UA Widget initializes, it should create its own DOM inside the passed UA Widget Shadow Root, including the ``<link>`` element necessary to load the stylesheet, add event listeners, etc. When destroyed (i.e. the destructor method is called), it should do the opposite.
 
-**Specialization**: for video controls, we do not want to do the work if the control is not needed (i.e. when the ``<video>`` or ``<audio>`` element has no "controls" attribute set), so we forgo dispatching the event from HTMLMediaElement in the BindToTree method. Instead, another ``UAWidgetAttributeChanged`` event will cause the sandbox and the widget instance to construct when the attribute is set to true. The same event is also responsible for triggering the ``onchange()`` method on UA Widgets if the widget is already initialized.
+**Specialization**: for video controls, we do not want to do the work if the control is not needed (i.e. when the ``<video>`` or ``<audio>`` element has no "controls" attribute set), so we forgo dispatching the event from HTMLMediaElement in the BindToTree method. Instead, another ``UAWidgetSetupOrChange`` event will cause the sandbox and the widget instance to construct when the attribute is set to true. The same event is also responsible for triggering the ``onchange()`` method on UA Widgets if the widget is already initialized.
 
 Likewise, the datetime box widget is only loaded when the ``type`` attribute of an ``<input>`` is either `date` or `time`.
 

toolkit/modules/ActorManagerParent.jsm

@@ -267,9 +267,8 @@ let ACTORS = {
     child: {
       module: "resource://gre/actors/UAWidgetsChild.jsm",
       events: {
-        "UAWidgetBindToTree": {},
+        "UAWidgetSetupOrChange": {},
-        "UAWidgetAttributeChanged": {},
+        "UAWidgetTeardown": {},
-        "UAWidgetUnbindFromTree": {},
       },
     },
   },