nsFrameManager::NotifyDestroyingFrame does nsFrameManager::ClearAllUndisplayedContentIn which calls RemoveNodesFor on mUndisplayedMap. We might want to condition this on some boolean flag on the node or frame.

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #0) > nsFrameManager::NotifyDestroyingFrame does > nsFrameManager::ClearAllUndisplayedContentIn Note: bug 1367217 has a patch that renames that method (ClearAllUndisplayedContentIn) to "ClearAllMapsFor". I'm updating the bug-summary accordingly, on the assumption that that patch is going to land + stick.

Baku, can you review the nsINode change?

Comment on attachment 8880021 [details] [diff] [review] patch And, Mats, the layout change.

Wouldn't be better to include both display:none/contents in the same flag? If so, we could simply do an early return at the top in ClearAllMapsFor: http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/layout/base/nsFrameManager.cpp#279 i.e. avoiding the iteration at the end there. Also, why can't we reset the flag at the end in ClearAllMapsFor for aParentContent, i.e. change the meaning of the flag to be "this node is in a nsFrameManager map"?

Hmm, that iteration might always be needed when I think about it...

Comment on attachment 8880021 [details] [diff] [review] patch r- for now, per comment 4.

Comment on attachment 8880021 [details] [diff] [review] patch Review of attachment 8880021 [details] [diff] [review]: ----------------------------------------------------------------- r+ for the dom part.

Comment on attachment 8883137 [details] [diff] [review] patch r=mats, with a few nits: >layout/base/nsFrameManager.cpp >+ if (!mUndisplayedMap || >+ !aParentContent->MayHaveChildrenWithLayoutBoxesDisabled()) { >+ MOZ_ASSERT(!mUndisplayedMap->GetFirstNode(aParentContent), >+ "We're going to fail to remove nodes from mUndisplayedMap"); We need a "!mUndisplayedMap ||" in the assertion expression too, otherwise it will crash if mUndisplayedMap is null. >+ // XXXjwatt If we're willing to pay the cost of checking mDisplayContentsMap >+ // we could call aParentContent->UnsetMayHaveChildrenWithLayoutBoxesDisabled >+ // here. display:contents is rare though, so I think we should at least add: if (!mDisplayContentsMap) { aParentContent->UnsetMayHaveChildrenWithLayoutBoxesDisabled(); } and update the "checking mDisplayContentsMap" in the comment to say "checking for entries in mDisplayContentsMap". >+#ifdef DEBUG >+ else { >+ MOZ_ASSERT(!mUndisplayedMap->GetFirstNode(aParentContent) && >+ !mDisplayContentsMap->GetFirstNode(aParentContent), >+ "We failed to remove a node from mUndisplayedMap/mDisplayContentsMap"); >+ } >+#endif The #ifdef DEBUG is unnecessary, MOZ_ASSERT is a no-op in a non-DEBUG build. >+ if (!mDisplayContentsMap || >+ !aParentContent->MayHaveChildrenWithLayoutBoxesDisabled()) { >+ MOZ_ASSERT(!mDisplayContentsMap->GetFirstNode(aParentContent), >+ "We're going to fail to remove nodes from mDisplayContentsMap"); MOZ_ASSERT expression crashes if mDisplayContentsMap is null. Also, there's a ^M at the end of the MOZ_ASSERT line. >+ // XXXjwatt If we're willing to pay the cost of checking mUndisplayedMap >+ // we could call aParentContent->UnsetMayHaveChildrenWithLayoutBoxesDisabled >+ // here. Maybe worth handling a null mUndisplayedMap, as above? There are ^M's at the end of the first two lines in the comment.

This patch addresses the review comments for the r+'ed patch and tidies/fixes the code some more to try and avoid some failures of the new assertions on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50c947b3764fc5e582d8136c81b86f22fbf3e821 The failures seem to be the result of the MayHaveChildrenWithLayoutBoxesDisabled bit being set on a "parent" node that is not the node that we look at later. Something related to XBL/XUL as far as I can tell. The main substantive change I made to the patch is to put the SetMayHaveChildrenWithLayoutBoxesDisabled() call down in UndisplayedMap::AppendNodeFor instead of further up the call stack in nsFrameManager::SetStyleContextInMap. This is because UndisplayedMap::AppendNodeFor can change the "parent" that is used in the hash table for children of <xbl::children>. That change fixed some, but not all of the assertions. So far I haven't figured out why, but I'm wondering if this may be a pre-existing bug related to the parent being different before and after binding.

Ideally I'd like to actually strengthen the assertions even further, but this too fails some tests on Try, although different ones: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c122fd725050c9d52be8e0c52738c4d0af349a67

jwatt, I'm going through our [qf:p1] bugs, and it looks like this one might've stalled -- is this close to landing? (Sounds like we've still got a few test failures to sort out?)

I was off on PTO, then a combination of off sick and busy with reviews. I should have a new version of the patch up shortly.

Comment on attachment 8895676 [details] Bug 1367214, part 2 - Ensure that all the UndisplayedMap handling code consistently acts on the normalized parent. This patch is to solve the assertion failures in comment 10 and 11. Basically it makes sure that all code is looking at the same "parent" object throughout. I'm not super enthusiastic about the way that this patch requires all "entry points" into the relevant code to call the function to get the final canonical parent. Especially since some of these "entry points" are not the real entry points but actually a shared implementation between real [DisplayNone|DisplayContents] entry points. It also makes it more likely that someone adding a new entry point in future will do it wrong. I don't see a better way of doing this though, and we need to if we're going to be setting/reading a flag on/from the "parent".

Note that the patches here apply on top of the patches from bug 1388939.

Comment on attachment 8895675 [details] Bug 1367214, part 1 - Fix nsFrameManager::ChangeStyleContextInMap to work for direct children of a shadow root. https://reviewboard.mozilla.org/r/166950/#review172410 r=me

Comment on attachment 8895676 [details] Bug 1367214, part 2 - Ensure that all the UndisplayedMap handling code consistently acts on the normalized parent. https://reviewboard.mozilla.org/r/166952/#review172418 ::: layout/base/nsFrameManager.cpp:99 (Diff revision 1) > * 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 is handling a "parent" > + * related to an instances of this class must call this method up front to Typo: "an instances" Maybe you meant "any instances"? or "an instance"? ::: layout/base/nsFrameManager.cpp:171 (Diff revision 1) > + // This function is an entry point into UndisplayedMap handling code, so we > + // must call GetApplicableParent so the parent we pass around is correct. > + nsIContent* parent = > + UndisplayedMap::GetApplicableParent(ParentForUndisplayedMap(aContent)); This function-nesting seems a bit unnecessary/confusing. Perhaps ParentForUndisplayedMap() should just *internally* call GetApplicableParent? That function (ParentForUndisplayedMap) already sounds very strongly like it should return the correct parent to use with the undisplayed map -- so it's a bit odd that we can't really trust the parent that it returns and have to call another "NoReallyGiveMeTheCorrectParentForUndisplayedMap" wrapper function...

Comment on attachment 8895677 [details] Bug 1367214, part 3 - Avoid hashtable lookups in the undisplayed map for elements that have never had display:none children. https://reviewboard.mozilla.org/r/166954/#review172432 ::: commit-message-0601e:1 (Diff revision 1) > +Bug 1367214, part 3 - Avoid hashtable lookups in the undisplayed map for elements that have never had display:none children. r=dholbert s/map/maps/ s/display:none/display:none nor display:contents/ (or similar rewording, to hint at both maps & both types of undisplayed children) ::: dom/base/nsINode.h:1722 (Diff revision 1) > + void SetMayHaveChildrenWithLayoutBoxesDisabled() { SetBoolFlag(NodeMayHaveChildrenWithLayoutBoxesDisabled); } > + void UnsetMayHaveChildrenWithLayoutBoxesDisabled() { ClearBoolFlag(NodeMayHaveChildrenWithLayoutBoxesDisabled); } > + bool MayHaveChildrenWithLayoutBoxesDisabled() const { return GetBoolFlag(NodeMayHaveChildrenWithLayoutBoxesDisabled); } > + These lines are way too long. The contextual code is quite long, too (e.g. a few lines up we have something 97 characters long) but these are a good bit longer (112, 116, 122 characters). Let's just wrap these, to match the formatting of "NodeMayBeApzAware()" a few lines further up in the contextual code -- i.e. to match the "int LargerFunction()" example (the can't-fit-on-one-line-example) from the Coding Style guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style ::: layout/base/nsFrameManager.cpp:324 (Diff revision 1) > + if (aParentContent && !mDisplayContentsMap) { > + MOZ_ASSERT(!mDisplayNoneMap->GetFirstNode(aParentContent), > + "Bad UnsetMayHaveChildrenWithLayoutBoxesDisabled call"); You should probably move this MOZ_ASSERT outside of this inner "if" check, I think. I assume you've got it in there to tightly couple it with the Unset() call -- but it'll still be close enough, and it's clearer & stronger if it's at the correct level of granularity for what it's asserting, IMO. Might even want to put the assertion in the middle of the comment? e.g.: if (haveOneDisplayNoneChild) { // There are no more children of aParentContent in mDisplayNoneMap. MOZ_ASSERT(...); // If we also know that none of its children are in mDisplayContentsMap (This feedback applies in two places -- here in UnregisterDisplayNoneStyleFor(), and in the analogous chunk in UnregisterDisplayContentsStyleFor().) ::: layout/base/nsFrameManager.cpp:369 (Diff revision 1) > + if (mDisplayNoneMap) { > + MOZ_ASSERT(!aParentContent || > + !mDisplayNoneMap->GetFirstNode(aParentContent), > + "We failed to remove a node from mDisplayNoneMap"); > + } > + if (mDisplayContentsMap) { > + MOZ_ASSERT(!aParentContent || > + !mDisplayContentsMap->GetFirstNode(aParentContent), > + "We failed to remove a node from mDisplayContentsMap"); The !aParentContent condition will never be satisfied in these assertions, because we are in an else clause for a "if (!aParentContent || [stuff])" check. So, you should just remove that condition from the assertions. Put another way: if aParentContent were null, then we would've entered the "if" body and we would never hit this "else" clause in the first place. So it's already guaranteed that aParentContent is non-null when we evaluate these assertions.

I'm holding off on r+ for part 2, at this point, since I'm not convinced it makes sense to have redundant-looking calls to GetApplicableParent(ParentForUndisplayedMap(...)), as noted in comment 20...

Comment on attachment 8895676 [details] Bug 1367214, part 2 - Ensure that all the UndisplayedMap handling code consistently acts on the normalized parent. https://reviewboard.mozilla.org/r/166952/#review173858 r=me ::: layout/base/nsFrameManager.cpp:100 (Diff revisions 1 - 2) > * 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 is handling a "parent" > - * related to an instances of this class must call this method up front to > - * ensure that that function and any code that it passes the parent to all > + * 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 s/they acts/they act/ s/and pass/and pass/

Comment on attachment 8897187 [details] Bug 1367214, part 3 - Avoid hashtable lookups in the undisplayed maps for elements that have never had display:none nor display:contents children. https://reviewboard.mozilla.org/r/168474/#review173862 r=me

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/a5e5540c7503 part 1 - Fix nsFrameManager::ChangeStyleContextInMap to work for direct children of a shadow root. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb7c3957b57 part 2 - Ensure that all the UndisplayedMap handling code consistently acts on the normalized parent. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/de339d0ca73b part 3 - Avoid hashtable lookups in the undisplayed maps for elements that do not have display:none nor display:contents children. r=baku,dholbert

https://hg.mozilla.org/mozilla-central/rev/a5e5540c7503 https://hg.mozilla.org/mozilla-central/rev/2cb7c3957b57 https://hg.mozilla.org/mozilla-central/rev/de339d0ca73b