Bug 1652618 - Ensure UA widgets are attached and detached synchronously. r=smaug

This changes the UA widget setup (again). What is going on in this
test-case is that we have a marquee inside a video, two things that have
their own UA widget. Given how the code is currently written, the
runnable to attach and set up the marquee's widget is posted before than
the video one (which is potentially reasonable).

However that means that the marquee one runs before and flushes layout,
and catches the video in an inconsistent state (in the composed doc, but
without a shadow root). That in turn messes up reflow because
nsVideoFrame assumes stuff.

Rather than putting the attach / detach logic in script runners, just
run that bit synchronously, and post only the event async. I audited the
consumers of those events and it seems fine to me, they either already
deal with the possibility of the shadow root being already detached or
they don't care.

For teardown, none of the destructors of the UA widgets rely on the
shadow root being still attached to the element.

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

dom/base/Element.cpp

@@ -1122,24 +1122,20 @@ already_AddRefed<ShadowRoot> Element::AttachShadowWithoutNameChecks(
   return shadowRoot.forget();
 }
 
-void Element::AttachAndSetUAShadowRoot() {
+void Element::AttachAndSetUAShadowRoot(NotifyUAWidgetSetup aNotify) {
   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;
-        }
+  if (!GetShadowRoot()) {
+    RefPtr<ShadowRoot> shadowRoot =
+        AttachShadowWithoutNameChecks(ShadowRootMode::Closed);
+    shadowRoot->SetIsUAWidget();
+  }
 
-        RefPtr<ShadowRoot> shadowRoot =
-            self->AttachShadowWithoutNameChecks(ShadowRootMode::Closed);
-        shadowRoot->SetIsUAWidget();
-      }));
+  MOZ_ASSERT(GetShadowRoot()->IsUAWidget());
+  if (aNotify == NotifyUAWidgetSetup::Yes) {
+    NotifyUAWidgetSetupOrChange();
+  }
 }
 
 void Element::NotifyUAWidgetSetupOrChange() {
@@ -1153,9 +1149,6 @@ void Element::NotifyUAWidgetSetupOrChange() {
       "Element::NotifyUAWidgetSetupOrChange::UAWidgetSetupOrChange",
       [self = RefPtr<Element>(this),
        ownerDoc = RefPtr<Document>(OwnerDoc())]() {
-        MOZ_ASSERT(self->GetShadowRoot() &&
-                   self->GetShadowRoot()->IsUAWidget());
-
         nsContentUtils::DispatchChromeEvent(ownerDoc, self,
                                             u"UAWidgetSetupOrChange"_ns,
                                             CanBubble::eYes, Cancelable::eNo);
@@ -1164,18 +1157,20 @@ void Element::NotifyUAWidgetSetupOrChange() {
 
 void Element::NotifyUAWidgetTeardown(UnattachShadowRoot aUnattachShadowRoot) {
   MOZ_ASSERT(IsInComposedDoc());
+  if (!GetShadowRoot()) {
+    return;
+  }
+  MOZ_ASSERT(GetShadowRoot()->IsUAWidget());
+  if (aUnattachShadowRoot == UnattachShadowRoot::Yes) {
+    UnattachShadow();
+  }
+
   // 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),
+      [self = RefPtr<Element>(this),
        ownerDoc = RefPtr<Document>(OwnerDoc())]() {
-        if (!self->GetShadowRoot()) {
-          // No UA Widget Shadow Root was ever attached.
-          return;
-        }
-        MOZ_ASSERT(self->GetShadowRoot()->IsUAWidget());
-
         // Bail out if the element is being collected by CC
         bool hasHadScriptObject = true;
         nsIScriptGlobalObject* scriptObject =
@@ -1184,16 +1179,9 @@ void Element::NotifyUAWidgetTeardown(UnattachShadowRoot aUnattachShadowRoot) {
           return;
         }
 
-        nsresult rv = nsContentUtils::DispatchChromeEvent(
+        Unused << nsContentUtils::DispatchChromeEvent(
             ownerDoc, self, u"UAWidgetTeardown"_ns, CanBubble::eYes,
             Cancelable::eNo);
-        if (NS_WARN_IF(NS_FAILED(rv))) {
-          return;
-        }
-
-        if (aUnattachShadowRoot == UnattachShadowRoot::Yes) {
-          self->UnattachShadow();
-        }
       }));
 }
 

dom/base/Element.h

@@ -1251,7 +1251,8 @@ class Element : public FragmentOrElement {
       ShadowRootMode aMode);
 
   // Attach UA Shadow Root if it is not attached.
-  void AttachAndSetUAShadowRoot();
+  enum class NotifyUAWidgetSetup : bool { No, Yes };
+  void AttachAndSetUAShadowRoot(NotifyUAWidgetSetup = NotifyUAWidgetSetup::Yes);
 
   // Dispatch an event to UAWidgetsChild, triggering construction
   // or onchange callback on the existing widget.

dom/base/nsObjectLoadingContent.cpp

@@ -2536,7 +2536,6 @@ void nsObjectLoadingContent::NotifyStateChanged(ObjectType aOldType,
       thisEl->NotifyUAWidgetTeardown();
     } else if (!hadProblemState && hasProblemState) {
       thisEl->AttachAndSetUAShadowRoot();
-      thisEl->NotifyUAWidgetSetupOrChange();
     }
   } else if (aOldType != mType) {
     // If our state changed, then we already recreated frames

dom/html/HTMLInputElement.cpp

@@ -4222,7 +4222,6 @@ nsresult HTMLInputElement::BindToTree(BindContext& aContext, nsINode& aParent) {
       IsInComposedDoc()) {
     // Construct Shadow Root so web content can be hidden in the DOM.
     AttachAndSetUAShadowRoot();
-    NotifyUAWidgetSetupOrChange();
   }
 
   if (mType == NS_FORM_INPUT_PASSWORD) {
@@ -4487,7 +4486,6 @@ void HTMLInputElement::HandleTypeChange(uint8_t aNewType, bool aNotify) {
     } else if (mType == NS_FORM_INPUT_TIME || mType == NS_FORM_INPUT_DATE) {
       // Switch to date/time type.
       AttachAndSetUAShadowRoot();
-      NotifyUAWidgetSetupOrChange();
     }
   }
 }

dom/html/HTMLMarqueeElement.cpp

@@ -53,7 +53,6 @@ nsresult HTMLMarqueeElement::BindToTree(BindContext& aContext,
 
   if (IsInComposedDoc()) {
     AttachAndSetUAShadowRoot();
-    NotifyUAWidgetSetupOrChange();
   }
 
   return rv;

dom/html/HTMLMediaElement.cpp

@@ -4741,7 +4741,6 @@ nsresult HTMLMediaElement::BindToTree(BindContext& aContext, nsINode& aParent) {
   if (IsInComposedDoc()) {
     // Construct Shadow Root so web content can be hidden in the DOM.
     AttachAndSetUAShadowRoot();
-    NotifyUAWidgetSetupOrChange();
 
     // The preload action depends on the value of the autoplay attribute.
     // It's value may have changed, so update it.

dom/xml/nsXMLPrettyPrinter.cpp

@@ -10,6 +10,7 @@
 #include "nsPIDOMWindow.h"
 #include "nsNetUtil.h"
 #include "mozilla/dom/Element.h"
+#include "mozilla/dom/ShadowRoot.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/dom/Document.h"
 #include "nsVariant.h"
@@ -84,7 +85,7 @@ nsresult nsXMLPrettyPrinter::PrettyPrint(Document* aDocument,
   }
 
   // Attach an UA Widget Shadow Root on it.
-  rootElement->AttachAndSetUAShadowRoot();
+  rootElement->AttachAndSetUAShadowRoot(Element::NotifyUAWidgetSetup::No);
   RefPtr<ShadowRoot> shadowRoot = rootElement->GetShadowRoot();
   MOZ_RELEASE_ASSERT(shadowRoot && shadowRoot->IsUAWidget(),
                      "There should be a UA Shadow Root here.");

layout/generic/crashtests/1652618.html

@@ -0,0 +1,15 @@
+<style>
+* {
+  float: left !important;
+  all: initial;
+  block-size: 247ch;
+  columns: 251ch auto;
+}
+</style>
+<script>
+window.addEventListener('load', () => {
+  var x = document.createElementNS('http://www.w3.org/1999/xhtml', 'audio')
+  try { x.innerHTML = '<marquee>' } catch (e) {}
+  try { document.documentElement.appendChild(x) } catch (e) {}
+})
+</script>

layout/generic/crashtests/crashtests.list

@@ -780,4 +780,5 @@ load 1640275.html
 pref(layout.accessiblecaret.enabled,true) load 1644819.html
 load 1645549-1.html
 load 1648577.html
+load 1652618.html
 load 1652897.html

toolkit/actors/UAWidgetsChild.jsm

@@ -50,6 +50,13 @@ class UAWidgetsChild extends JSWindowActorChild {
     let { widget } = this.widgets.get(aElement);
 
     if (typeof widget.onchange == "function") {
+      if (aElement.openOrClosedShadowRoot != widget.shadowRoot) {
+        Cu.reportError(
+          "Getting a UAWidgetSetupOrChange event without the ShadowRoot. " +
+            "Torn down already?"
+        );
+        return;
+      }
       try {
         widget.onchange();
       } catch (ex) {
@@ -107,7 +114,8 @@ class UAWidgetsChild extends JSWindowActorChild {
     let shadowRoot = aElement.openOrClosedShadowRoot;
     if (!shadowRoot) {
       Cu.reportError(
-        "Getting a UAWidgetSetupOrChange event without the Shadow Root."
+        "Getting a UAWidgetSetupOrChange event without the Shadow Root. " +
+          "Torn down already?"
       );
       return;
     }
@@ -130,6 +138,9 @@ class UAWidgetsChild extends JSWindowActorChild {
     if (!isSystemPrincipal) {
       widget = widget.wrappedJSObject;
     }
+    if (widget.shadowRoot != shadowRoot) {
+      Cu.reportError("Widgets should expose their shadow root.");
+    }
     this.widgets.set(aElement, { widget, widgetName });
     try {
       widget.onsetup();