The IsInObserverList related code in SVGObserverUtils.h/.cpp is unfortunate since it seems like we shouldn't need to check this. The code is only used in one place, in nsSVGIntegrationUtils::AdjustInvalidAreaForSVGEffects. This mechanism was introduced in bug 587336: https://hg.mozilla.org/mozilla-central/rev/c7726ff56cbf This code is from eight years ago when we were still recursing up the frame invalidating each frame whenever we needed to invalidate something. The code was introduced to prevent an invalidation loop. What was happening is that the frame with a filter(#parent) would invalidate, the recursive invalidation would recurse up the frame tree, invalidating the parent which would post a (change hint) restyle event at the original frame referencing the "filter" parent. Hence, invalidation loop forever. Now that we are in a DLBI world the original reason to have this hack has gone.

Unfortunately, removing this code causes the following two reftests to fail: layout/reftests/invalidation/scroll-inactive-layers.html layout/reftests/invalidation/scroll-inactive-layers-2.html These tests check that scrolling doesn't cause repainting of inactive (opacity/transform/filter/mask) layers. Despite the IsInObserverList code being purely for filters, it is the masked element that is repainting on scroll, causing the test failure. The relevant change to the code is the removal of the first highlighted block here: https://searchfox.org/mozilla-central/source/layout/svg/nsSVGIntegrationUtils.cpp#328-330,334-341 What seems to happen is that, with the removal of the first `if` block we no longer early return aInvalidRegion. Instead we enter the second `if` block because 'observers' is nullptr (since the nsSVGMaskFrame does not have a SVGFilterObserverListForCSSProp of course) and return the value computed there. Specifically in my testing, before removing the hack we return aInvalidRegion with the values: left 748 top 2104 right 952 bottom 4458 After removing the first `if` block we return: left 748 top 254 right 952 bottom 4459 The much smaller value for 'top' is what makes the difference and cause us to fail.

The call stack here is: nsSVGIntegrationUtils::AdjustInvalidAreaForSVGEffects mozilla::FrameLayerBuilder::AddPaintedDisplayItem mozilla::PaintedLayerDataNode::PopAllPaintedLayerData mozilla::PaintedLayerDataNode::Finish mozilla::PaintedLayerDataNode::FinishChildrenIntersecting mozilla::PaintedLayerDataTree::FinishPotentiallyIntersectingNodes mozilla::PaintedLayerDataTree::AddingOwnLayer mozilla::ContainerState::ProcessDisplayItems mozilla::FrameLayerBuilder::BuildContainerLayerFor nsDisplayList::BuildLayers nsDisplayList::PaintRoot nsLayoutUtils::PaintFrame mozilla::PresShell::Paint nsViewManager::ProcessPendingUpdatesPaint nsViewManager::ProcessPendingUpdatesForView nsViewManager::ProcessPendingUpdates nsRefreshDriver::Tick Looking at AddPaintedDisplayItem, it seems that what it passes into AdjustInvalidAreaForSVGEffects as the value for aInvalidRegion is: frame->GetVisualOverflowRect() .ToOutsidePixels(appUnitsPerDevPixel); whereas what we return when we fail is: (frame->GetVisualOverflowRect() + aToReferenceFrame) .ToOutsidePixels(appUnitsPerDevPixel) There's clearly a bug in the removed `if` block where it fails to take account of aToReferenceFrame, and that that bug is hiding the fact that even without my change we are *already* invalidating the masked element in the two failing reftests. The only reason that the tests don't actually fail is because the preexisting bug results in the invalid area being offscreen. The fact that these tests should already currently be failing makes me think that they shouldn't block removing this old hack. We should remove it and be honest about the state of the tests by marking them as failing (filing a follow-up bug to fix that of course).

(In reply to Jonathan Watt [:jwatt] from comment #1) > https://searchfox.org/mozilla-central/source/layout/svg/ > nsSVGIntegrationUtils.cpp#328-330,334-341 Permalink: https://searchfox.org/mozilla-central/rev/881a3c5664ede5e08ee986d76433bc5c4b5680e6/layout/svg/nsSVGIntegrationUtils.cpp#328-330,334-341

I've spun off bug 1494110 for the preexisting invalidation issue.

This code is no longer necessary since we now invalidate using Display List Based Invalidation instead of using recursive calls up the frame tree. The tests that are marked as failing have only been passing due to a bug in the code that's being removed from nsSVGIntegrationUtils.cpp which coincidentally hides the fact that we are actually invalidating in those tests given their particular structure (which the tests are supposed to be checking we're not doing).

Comment on attachment 9011962 [details] Bug 1494092. Remove SVGFilterObserverList::IsInObserverLists. r?mattwoodrow Matt Woodrow (:mattwoodrow) has approved the revision.

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/d2218beee052 Remove SVGFilterObserverList::IsInObserverLists and related code. r=mattwoodrow