Bug 1367214, part 2 - Ensure that all the UndisplayedMap handling code consistently acts on the normalized parent. r=dholbert draft
authorJonathan Watt <jwatt@jwatt.org>
Mon, 14 Aug 2017 23:57:38 +0100
changeset 646187 d3d8de877ce87e75834fe494bfe11312f25a468c
parent 646186 9d0db1e26537f7cfeaa17c6d2248aa2afb08f196
child 646205 c9c96a19e9bb3af78104513faa2383af600c9465
push id74020
push userjwatt@jwatt.org
push dateMon, 14 Aug 2017 23:03:12 +0000
reviewersdholbert
bugs1367214
milestone57.0a1
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: 7alakeste2U
layout/base/nsFrameManager.cpp
--- 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 acts on (and passed 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");
 }
@@ -135,16 +141,19 @@ nsFrameManager::Destroy()
 static nsIContent*
 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);