Bug 1585819 - Restrict loading XBL bindings from WrapObject to the little amount of XUL elements that remain with bindings. r=bzbarsky
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 03 Oct 2019 02:31:16 +0000
changeset 496146 d7f1f760cd4582ab452b04fdea0425a4072a0295
parent 496145 ff9b6e41dabcd5520a105ab816a37b1667f5ab68
child 496147 842c1f1da8a9c1f6d0f45a7a005701027c88cd3e
push id97050
push userealvarez@mozilla.com
push dateThu, 03 Oct 2019 02:46:33 +0000
treeherderautoland@d7f1f760cd45 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky
bugs1585819
milestone71.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 1585819 - Restrict loading XBL bindings from WrapObject to the little amount of XUL elements that remain with bindings. r=bzbarsky People keep shooting themselves in the feet because of this codepath and writing slow code. There are just a few elements with bindings left, so just check those. Also simplify a bit the code. The XUL element + tagname check should be pretty fast now, and ComputedStyle objects no longer keep weak pointers to pres contexts and such, so can be safely kept on a RefPtr across an style flush. Differential Revision: https://phabricator.services.mozilla.com/D47995
dom/base/CustomElementRegistry.h
dom/base/Element.cpp
dom/base/Element.h
xpcom/ds/StaticAtoms.py
--- a/dom/base/CustomElementRegistry.h
+++ b/dom/base/CustomElementRegistry.h
@@ -367,26 +367,16 @@ class CustomElementRegistry final : publ
    private:
     RefPtr<CustomElementRegistry> mRegistry;
     RefPtr<nsAtom> mAtom;
     RefPtr<CustomElementCreationCallback> mCallback;
   };
 
  public:
   /**
-   * Returns whether there's a definition that is likely to match this type
-   * atom. This is not exact, so should only be used for optimization, but it's
-   * good enough to prove that the chrome code doesn't need an XBL binding.
-   */
-  bool IsLikelyToBeCustomElement(nsAtom* aTypeAtom) const {
-    return mCustomDefinitions.GetWeak(aTypeAtom) ||
-           mElementCreationCallbacks.GetWeak(aTypeAtom);
-  }
-
-  /**
    * Looking up a custom element definition.
    * https://html.spec.whatwg.org/#look-up-a-custom-element-definition
    */
   CustomElementDefinition* LookupCustomElementDefinition(nsAtom* aNameAtom,
                                                          int32_t aNameSpaceID,
                                                          nsAtom* aTypeAtom);
 
   CustomElementDefinition* LookupCustomElementDefinition(
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -512,122 +512,81 @@ void Element::ClearStyleStateLocks() {
   StyleStateLocks locks = LockedStyleStates();
 
   DeleteProperty(nsGkAtoms::lockedStyleStates);
   ClearHasLockedStyleStates();
 
   NotifyStyleStateChange(locks.mLocks);
 }
 
-static bool IsLikelyCustomElement(const nsXULElement& aElement) {
-  const CustomElementData* data = aElement.GetCustomElementData();
-  if (!data) {
-    return false;
-  }
-
-  const CustomElementRegistry* registry =
-      nsContentUtils::GetCustomElementRegistry(aElement.OwnerDoc());
-  if (!registry) {
+static bool MayNeedToLoadXBLBinding(const Element& aElement) {
+  if (!aElement.IsAnyOfXULElements(nsGkAtoms::panel, nsGkAtoms::textbox,
+                                   nsGkAtoms::arrowscrollbox)) {
+    // Other elements no longer have XBL bindings. Please don't add to the list
+    // above unless completely necessary.
     return false;
   }
-
-  return registry->IsLikelyToBeCustomElement(data->GetCustomElementType());
-}
-
-static bool MayNeedToLoadXBLBinding(const Document& aDocument,
-                                    const Element& aElement) {
-  auto* xulElem = nsXULElement::FromNode(aElement);
-  if (!xulElem) {
+  if (!aElement.IsInComposedDoc()) {
     return false;
   }
   // If we have a frame, the frame has already loaded the binding.
-  // Otherwise, don't do anything else here unless we're dealing with XUL.
-  if (!aDocument.GetPresShell() || aElement.GetPrimaryFrame()) {
+  if (aElement.GetPrimaryFrame() || !aElement.OwnerDoc()->GetPresShell()) {
     return false;
   }
-  return !IsLikelyCustomElement(*xulElem);
-}
-
-StyleUrlOrNone Element::GetBindingURL(Document* aDocument) {
-  if (!MayNeedToLoadXBLBinding(*aDocument, *this)) {
-    return StyleUrlOrNone::None();
-  }
-
-  // Get the computed -moz-binding directly from the ComputedStyle
-  RefPtr<ComputedStyle> style =
-      nsComputedDOMStyle::GetComputedStyleNoFlush(this, nullptr);
-  if (!style) {
-    return StyleUrlOrNone::None();
-  }
-
-  return style->StyleDisplay()->mBinding;
+  // If we have a binding, well..
+  if (aElement.GetXBLBinding()) {
+    return false;
+  }
+  // We need to try.
+  return true;
 }
 
 JSObject* Element::WrapObject(JSContext* aCx,
                               JS::Handle<JSObject*> aGivenProto) {
   JS::Rooted<JSObject*> obj(aCx, nsINode::WrapObject(aCx, aGivenProto));
   if (!obj) {
     return nullptr;
   }
 
-  if (XRE_IsContentProcess() && !NodePrincipal()->IsSystemPrincipal()) {
-    // We don't use XBL in content privileged content processes.
-    return obj;
-  }
-
-  Document* doc = GetComposedDoc();
-  if (!doc) {
-    // There's no baseclass that cares about this call so we just
-    // return here.
+  if (!MayNeedToLoadXBLBinding(*this)) {
     return obj;
   }
 
-  // We must ensure that the XBL Binding is installed before we hand
-  // back this object.
-
-  if (HasFlag(NODE_MAY_BE_IN_BINDING_MNGR) && GetXBLBinding()) {
-    // There's already a binding for this element so nothing left to
-    // be done here.
-
-    // In theory we could call ExecuteAttachedHandler here when it's safe to
-    // run script if we also removed the binding from the PAQ queue, but that
-    // seems like a scary change that would mostly just add more
-    // inconsistencies.
+  RefPtr<ComputedStyle> style =
+      nsComputedDOMStyle::GetComputedStyleNoFlush(this, nullptr);
+  if (!style) {
+    return obj;
+  }
+
+  // We have a binding that must be installed.
+  const StyleUrlOrNone& computedBinding = style->StyleDisplay()->mBinding;
+  if (!computedBinding.IsUrl()) {
     return obj;
   }
 
-  // Make sure the ComputedStyle goes away _before_ we load the binding
-  // since that can destroy the relevant presshell.
-
-  {
-    StyleUrlOrNone result = GetBindingURL(doc);
-    if (result.IsUrl()) {
-      auto& url = result.AsUrl();
-      nsCOMPtr<nsIURI> uri = url.GetURI();
-      nsCOMPtr<nsIPrincipal> principal = url.ExtraData().Principal();
-
-      // We have a binding that must be installed.
-      nsXBLService* xblService = nsXBLService::GetInstance();
-      if (!xblService) {
-        dom::Throw(aCx, NS_ERROR_NOT_AVAILABLE);
-        return nullptr;
-      }
-
-      RefPtr<nsXBLBinding> binding;
-      xblService->LoadBindings(this, uri, principal, getter_AddRefs(binding));
-
-      if (binding) {
-        if (nsContentUtils::IsSafeToRunScript()) {
-          binding->ExecuteAttachedHandler();
-        } else {
-          nsContentUtils::AddScriptRunner(
-              NewRunnableMethod("nsXBLBinding::ExecuteAttachedHandler", binding,
-                                &nsXBLBinding::ExecuteAttachedHandler));
-        }
-      }
+  auto& url = computedBinding.AsUrl();
+  nsCOMPtr<nsIURI> uri = url.GetURI();
+  nsCOMPtr<nsIPrincipal> principal = url.ExtraData().Principal();
+
+  nsXBLService* xblService = nsXBLService::GetInstance();
+  if (!xblService) {
+    dom::Throw(aCx, NS_ERROR_NOT_AVAILABLE);
+    return nullptr;
+  }
+
+  RefPtr<nsXBLBinding> binding;
+  xblService->LoadBindings(this, uri, principal, getter_AddRefs(binding));
+
+  if (binding) {
+    if (nsContentUtils::IsSafeToRunScript()) {
+      binding->ExecuteAttachedHandler();
+    } else {
+      nsContentUtils::AddScriptRunner(
+          NewRunnableMethod("nsXBLBinding::ExecuteAttachedHandler", binding,
+                            &nsXBLBinding::ExecuteAttachedHandler));
     }
   }
 
   return obj;
 }
 
 /* virtual */
 nsINode* Element::GetScopeChainParent() const { return OwnerDoc(); }
--- a/dom/base/Element.h
+++ b/dom/base/Element.h
@@ -454,18 +454,16 @@ class Element : public FragmentOrElement
      * SetDirectionality for every element, and UpdateState is very very slow
      * for some elements.
      */
     if (aNotify) {
       UpdateState(true);
     }
   }
 
-  mozilla::StyleUrlOrNone GetBindingURL(Document* aDocument);
-
   Directionality GetComputedDirectionality() const;
 
   static const uint32_t kAllServoDescendantBits =
       ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO |
       ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO |
       NODE_DESCENDANTS_NEED_FRAMES;
 
   /**
--- a/xpcom/ds/StaticAtoms.py
+++ b/xpcom/ds/StaticAtoms.py
@@ -138,16 +138,17 @@ STATIC_ATOMS = [
     Atom("aria_required", "aria-required"),
     Atom("aria_selected", "aria-selected"),
     Atom("aria_setsize", "aria-setsize"),
     Atom("aria_sort", "aria-sort"),
     Atom("aria_valuemax", "aria-valuemax"),
     Atom("aria_valuemin", "aria-valuemin"),
     Atom("aria_valuenow", "aria-valuenow"),
     Atom("arrow", "arrow"),
+    Atom("arrowscrollbox", "arrowscrollbox"),
     Atom("article", "article"),
     Atom("as", "as"),
     Atom("ascending", "ascending"),
     Atom("aside", "aside"),
     Atom("aspectRatio", "aspect-ratio"),
     Atom("async", "async"),
     Atom("attribute", "attribute"),
     Atom("attributes", "attributes"),