Bug 1367214, part 2 - Ensure that all the UndisplayedMap handling code consistently acts on the normalized parent. r=dholbert
authorJonathan Watt <jwatt@jwatt.org>
Fri, 04 Aug 2017 14:52:25 +0100
changeset 374844 2cb7c3957b57e81990f5051d1385d0bff2d44829
parent 374843 a5e5540c7503623b3d84f6a6847c1f0803fe1a7f
child 374845 de339d0ca73b29a2e5f956893b1a340f56fcbbcf
push id32340
push userkwierso@gmail.com
push dateWed, 16 Aug 2017 02:03:08 +0000
treeherdermozilla-central@4e93516e92e5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1367214
milestone57.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 1367214, part 2 - Ensure that all the UndisplayedMap handling code consistently acts on the normalized parent. r=dholbert We have four entry points that deal with the parents of display:none/ display:contents content. These are the functions for setting, changing, getting and removing a style context. Or more specifically: GetUndisplayedNodeInMapFor called by GetDisplay[None|Contents]StyleFor (via GetStyleContextInMap) SetStyleContextInMap called by RegisterDisplay[None|Contents]StyleFor ChangeStyleContextInMap called by ChangeRegisteredDisplay[None|Contents]StyleFor UnregisterDisplay[None|Contents]StyleFor okay, this is actually two functions, but they act as a pair This change makes all these functions call GetApplicableParent up front and act on and pass around the parent that it returns. This is so that throughout the code we are always handling the parent that will be used as the key in the UndisplayedMap entry. This is necessary so that all the code that sets/gets the 'MayHaveChildrenWithLayoutBoxesDisabled' bit on/from an nsIContent object is using the same object, otherwise everything breaks down. MozReview-Commit-ID: 6gso1tyr33E
layout/base/nsFrameManager.cpp
layout/base/nsFrameManager.h
--- a/layout/base/nsFrameManager.cpp
+++ b/layout/base/nsFrameManager.cpp
@@ -85,26 +85,32 @@ public:
   void RemoveNodesFor(nsIContent* aParentContent);
 
   nsAutoPtr<LinkedList<UndisplayedNode>>
     UnlinkNodesFor(nsIContent* aParentContent);
 
   // Removes all entries from the hash table
   void  Clear();
 
+  /**
+   * Get the applicable parent for the map lookup. This is almost always the
+   * provided argument, except if it's a <xbl:children> element, in which case
+   * it's the parent of the children element.
+   *
+   * All functions that are entry points into code that handles "parent"
+   * objects (used as the hash table keys) must ensure that the parent objects
+   * that they act on (and pass to other code) have been normalized by calling
+   * this method.
+   */
+  static nsIContent* GetApplicableParent(nsIContent* aParent);
+
 protected:
   LinkedList<UndisplayedNode>* GetListFor(nsIContent* aParentContent);
   LinkedList<UndisplayedNode>* GetOrCreateListFor(nsIContent* aParentContent);
   void AppendNodeFor(UndisplayedNode* aNode, nsIContent* aParentContent);
-  /**
-   * Get the applicable parent for the map lookup. This is almost always the
-   * provided argument, except if it's a <xbl:children> element, in which case
-   * it's the parent of the children element.
-   */
-  nsIContent* GetApplicableParent(nsIContent* aParent);
 };
 
 //----------------------------------------------------------------------
 
 nsFrameManager::~nsFrameManager()
 {
   NS_ASSERTION(!mPresShell, "nsFrameManager::Destroy never called");
 }
@@ -127,24 +133,27 @@ nsFrameManager::Destroy()
   delete mDisplayContentsMap;
   mDisplayContentsMap = nullptr;
 
   mPresShell = nullptr;
 }
 
 //----------------------------------------------------------------------
 
-static nsIContent*
-ParentForUndisplayedMap(const nsIContent* aContent)
+/* static */ nsIContent*
+nsFrameManager::ParentForUndisplayedMap(const nsIContent* aContent)
 {
   MOZ_ASSERT(aContent);
 
   nsIContent* parent = aContent->GetParentElementCrossingShadowRoot();
   MOZ_ASSERT(parent || !aContent->GetParent(), "no non-elements");
 
+  // Normalize the parent:
+  parent = UndisplayedMap::GetApplicableParent(parent);
+
   return parent;
 }
 
 /* static */ nsStyleContext*
 nsFrameManager::GetStyleContextInMap(UndisplayedMap* aMap,
                                      const nsIContent* aContent)
 {
   UndisplayedNode* node = GetUndisplayedNodeInMapFor(aMap, aContent);
@@ -153,17 +162,23 @@ nsFrameManager::GetStyleContextInMap(Und
 
 /* static */ UndisplayedNode*
 nsFrameManager::GetUndisplayedNodeInMapFor(UndisplayedMap* aMap,
                                            const nsIContent* aContent)
 {
   if (!aContent) {
     return nullptr;
   }
+
+  // This function is an entry point into UndisplayedMap handling code, so the
+  // parent that we act on must be normalized by GetApplicableParent (as per
+  // that function's documentation).  We rely on ParentForUndisplayedMap to
+  // have done that for us.
   nsIContent* parent = ParentForUndisplayedMap(aContent);
+
   for (UndisplayedNode* node = aMap->GetFirstNode(parent);
        node; node = node->getNext()) {
     if (node->mContent == aContent)
       return node;
   }
 
   return nullptr;
 }
@@ -193,17 +208,22 @@ nsFrameManager::SetStyleContextInMap(Und
 #if defined(DEBUG_UNDISPLAYED_MAP) || defined(DEBUG_DISPLAY_BOX_CONTENTS_MAP)
   static int i = 0;
   printf("SetStyleContextInMap(%d): p=%p \n", i++, (void *)aContent);
 #endif
 
   MOZ_ASSERT(!GetStyleContextInMap(aMap, aContent),
              "Already have an entry for aContent");
 
+  // This function is an entry point into UndisplayedMap handling code, so the
+  // parent that we act on must be normalized by GetApplicableParent (as per
+  // that function's documentation).  We rely on ParentForUndisplayedMap to
+  // have done that for us.
   nsIContent* parent = ParentForUndisplayedMap(aContent);
+
 #ifdef DEBUG
   nsIPresShell* shell = aStyleContext->PresContext()->PresShell();
   NS_ASSERTION(parent || (shell && shell->GetDocument() &&
                           shell->GetDocument()->GetRootElement() == aContent),
                "undisplayed content must have a parent, unless it's the root "
                "element");
 #endif
   aMap->AddNodeFor(parent, aContent, aStyleContext);
@@ -226,16 +246,20 @@ nsFrameManager::ChangeStyleContextInMap(
 {
   MOZ_ASSERT(aMap, "expecting a map");
 
 #if defined(DEBUG_UNDISPLAYED_MAP) || defined(DEBUG_DISPLAY_BOX_CONTENTS_MAP)
    static int i = 0;
    printf("ChangeStyleContextInMap(%d): p=%p \n", i++, (void *)aContent);
 #endif
 
+  // This function is an entry point into UndisplayedMap handling code, so the
+  // parent that we act on must be normalized by GetApplicableParent (as per
+  // that function's documentation).  We rely on ParentForUndisplayedMap to
+  // have done that for us.
   nsIContent* parent = ParentForUndisplayedMap(aContent);
 
   for (UndisplayedNode* node = aMap->GetFirstNode(parent);
        node; node = node->getNext()) {
     if (node->mContent == aContent) {
       node->mStyle = aStyleContext;
       return;
     }
@@ -252,16 +276,20 @@ nsFrameManager::UnregisterDisplayNoneSty
   static int i = 0;
   printf("ClearUndisplayedContent(%d): content=%p parent=%p --> ", i++, (void *)aContent, (void*)aParentContent);
 #endif
 
   if (!mDisplayNoneMap) {
     return;
   }
 
+  // This function is an entry point into UndisplayedMap handling code, so we
+  // must call GetApplicableParent so the parent we pass around is correct.
+  aParentContent = UndisplayedMap::GetApplicableParent(aParentContent);
+
   for (UndisplayedNode* node = mDisplayNoneMap->GetFirstNode(aParentContent);
        node; node = node->getNext()) {
     if (node->mContent == aContent) {
       mDisplayNoneMap->RemoveNodeFor(aParentContent, node);
 
 #ifdef DEBUG_UNDISPLAYED_MAP
       printf( "REMOVED!\n");
 #endif
@@ -339,16 +367,20 @@ nsFrameManager::UnregisterDisplayContent
   static int i = 0;
   printf("ClearDisplayContents(%d): content=%p parent=%p --> ", i++, (void *)aContent, (void*)aParentContent);
 #endif
 
   if (!mDisplayContentsMap) {
     return;
   }
 
+  // This function is an entry point into UndisplayedMap handling code, so we
+  // must call GetApplicableParent so the parent we pass around is correct.
+  aParentContent = UndisplayedMap::GetApplicableParent(aParentContent);
+
   for (UndisplayedNode* node = mDisplayContentsMap->GetFirstNode(aParentContent);
        node; node = node->getNext()) {
     if (node->mContent == aContent) {
       mDisplayContentsMap->RemoveNodeFor(aParentContent, node);
 
 #ifdef DEBUG_DISPLAY_CONTENTS_MAP
       printf( "REMOVED!\n");
 #endif
@@ -615,30 +647,33 @@ nsFrameManagerBase::UndisplayedMap::GetA
   }
 
   return aParent;
 }
 
 LinkedList<UndisplayedNode>*
 nsFrameManagerBase::UndisplayedMap::GetListFor(nsIContent* aParent)
 {
-  aParent = GetApplicableParent(aParent);
+  MOZ_ASSERT(aParent == GetApplicableParent(aParent),
+             "The parent that we use as the hash key must have been normalized");
 
   LinkedList<UndisplayedNode>* list;
   if (Get(aParent, &list)) {
     return list;
   }
 
   return nullptr;
 }
 
 LinkedList<UndisplayedNode>*
 nsFrameManagerBase::UndisplayedMap::GetOrCreateListFor(nsIContent* aParent)
 {
-  aParent = GetApplicableParent(aParent);
+  MOZ_ASSERT(aParent == GetApplicableParent(aParent),
+             "The parent that we use as the hash key must have been normalized");
+
   return LookupOrAdd(aParent);
 }
 
 
 UndisplayedNode*
 nsFrameManagerBase::UndisplayedMap::GetFirstNode(nsIContent* aParentContent)
 {
   auto* list = GetListFor(aParentContent);
--- a/layout/base/nsFrameManager.h
+++ b/layout/base/nsFrameManager.h
@@ -204,16 +204,18 @@ public:
   /*
    * Add/restore state for one frame
    */
   void CaptureFrameStateFor(nsIFrame* aFrame, nsILayoutHistoryState* aState);
 
   void RestoreFrameStateFor(nsIFrame* aFrame, nsILayoutHistoryState* aState);
 
 protected:
+  static nsIContent* ParentForUndisplayedMap(const nsIContent* aContent);
+
   void ClearAllMapsFor(nsIContent* aParentContent);
 
   static nsStyleContext* GetStyleContextInMap(UndisplayedMap* aMap,
                                               const nsIContent* aContent);
   static mozilla::UndisplayedNode*
     GetUndisplayedNodeInMapFor(UndisplayedMap* aMap,
                                const nsIContent* aContent);
   static mozilla::UndisplayedNode*