Closed Bug 1494092 Opened Last year Closed Last year

Get rid of SVGFilterObserverList::IsInObserverLists

Categories

(Core :: SVG, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file)

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).
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.
Attachment #9011962 - Flags: review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2218beee052
Remove SVGFilterObserverList::IsInObserverLists and related code. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/d2218beee052
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1517197
Depends on: 1517458
Depends on: 1519427
Depends on: 1519838

(In reply to Jonathan Watt [:jwatt] from comment #2)

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.

That's wrong. In fact it's the other way around - we should not be adding in aToReferenceFrame.

You need to log in before you can comment on or make changes to this bug.