Fix gradient and pattern code to use nsReferencedElement

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Summary says it all really. This will make paint servers respect ID-element-map changes and fix some bugs. I just need to dig the patch out of my branch. The patch fixes some missing invalidation issues too.
Posted patch merged to trunk (obsolete) — Splinter Review
This patch is merged to trunk and compiles, but doesn't actually work. I need to hack on it some more.
Posted patch actual working patch (obsolete) — Splinter Review
-- Rename NS_FRAME_MAY_BE_TRANSFORMED to NS_FRAME_MAY_BE_TRANSFORMED_OR_HAVE_RENDERING_OBSERVERS, which is slightly clumsy but honest about how we're overloading this flag. Overloading the flag is ugly but there are no spare flag bits and in the future, when any element can be used as a background-image (or some similar feature exists) and hence have rendering observers, it actually makes sense because both CSS transforms and rendering observers have to hook paths in nsIFrame::InvalidateInternal.

-- nsSVGPropertyBase is a vague name so rename it to nsSVGRenderingObserver. (This object represents a dependency of one frame on the rendering of another frame identified by a URI. One or more of them are attached as frame properties to the observing frame.)

-- Maintain an nsSVGRenderingObserverList for each observed frame which records which nsSVGRenderingObservers are observing it. This is stored as a dynamic frame property.

-- When something relevant in the observed frame (or descendant) changes, it calls nsSVGEffects::InvalidateRenderingObservers. This calls InvalidateViaReferencedFrame on all nsSVGRenderingObservers and breaks the observer relationships. The idea is that a relationship will be reestablished the next time the observing frame needs to reference the observed frame. This "one-shot" behaviour makes the code a lot simpler; for example we can treat frame destruction just the same way as a style change.

-- When an observing frame is destroyed, the associated nsSVGRenderingObserver properties are destroyed so that lets us remove the nsSVGRenderingObservers from the observed frame's nsSVGRenderingObserverList ... unless the presshell is being torn down, in which case we destroy all frames before destroying all properties so the observed frame pointer is bogus. So we have to detect when the observed frame's presshell is being torn down and avoid doing work in that case.

-- When a notification propagates from an observed frame to an observing frame, we automatically propagate the notification if the observing frame is itself observed...

-- Add code to invalidate non-SVG frames when their nsSVGRenderingObservers change...

-- We don't need to watch ParentChainChanged DOM changes for the referenced/observed content because that's just telling us when the content is removed from the document, which causes an ID-element-map change anyway.

-- Clip-path and mask nsSVGRenderingObservers are basically the same: when things change, we just need to repaint. Since the same goes for fill and stroke as well, make them all use a single nsSVGPaintingProperty subclass of nsSVGRenderingObserver.

-- Treat fill and stroke URI references much like clip-path/mask/filter URI references, using an nsSVGPaintingProperty to track relationships. In particular, style changes are handled with an UpdateEffects style-change-hint which calls nsSVGUtils::UpdateEffects which clears cached properties

-- Note that the href 'inheritance' feature of patterns and gradients also uses an nsSVGPaintingProperty so we get all the liveness goodness for that too.

-- This gets rid of a ton of code in the paint servers as you can see.

-- I got rid of HasStroke and HasFill in favour of just returning booleans from Setup methods to indicate whether there was anything to set up.

-- I refactored gradients to simplify things a bit but mainly to eliminate the caching of mNextGrad, which isn't really compatible with the use of nsSVGPaintingProperty to track the relationship. Because I seem to recall that gradient painting needs to be really fast, I do cache a value mNoHRefURI (by far the common case) so we can very quickly avoid doing any work when there's no href set.

-- Patterns get similar treatment. The refactoring provides even more simplifications there.
Attachment #339572 - Flags: superreview?(mats.palmgren)
Attachment #339572 - Flags: review?(tor)
Oh, one of the driving forces behind the one-shot design for nsSVGRenderingObserver(List) is to avoid infinite invalidation loops when you have a cyclic reference structure. One-shot just completely solves that issue. (The existing loop detection flags avoid infinite recursion for painting.)

After this we should give markers the same treatment as paint servers get here. We may need a new nsSVGRenderingObserver subclass because markers can affect bounding boxes, much like filters, but I'm not sure.

The reason nsSVGRenderingObserverList is stored as a property on the frame instead of directly in the SVG frames that currently need it is that eventually I want to allow any frame to support it (e.g. when I allow any element to be a background-image for another element). It's also fairly simple compared to adding a virtual function to get the storage, implemented in each SVG frame type that can have observers.

Comment 4

11 years ago
Looks like a nice simplification of the current logic.  A few things came up when looking through the patch:

nsStyleSVG::CalcDifference - linecap and linejoin can change the bounding box of the geometry, necessitating more than just a repaint.

nsStyleSVGReset::CalcDifference - dominantBaseline.  ditto (I think).

Using nsReferencedElement doesn't allow us to get rid of the mutation observers?

You have a comment saying that gradients hardly ever refer to one another.  Back when the gradient code was implemented, we saw that inkscape used gradient cross-references frequently.

nsSVGPathGeometryFrame::UpdateCoveredRegion - you missed deleting the now extraneous call to SetupCairoStrokeGeometry.
(In reply to comment #4)
> Looks like a nice simplification of the current logic.  A few things came up
> when looking through the patch:
> 
> nsStyleSVG::CalcDifference - linecap and linejoin can change the bounding box
> of the geometry, necessitating more than just a repaint.
> 
> nsStyleSVGReset::CalcDifference - dominantBaseline.  ditto (I think).

OK, I'll look into what's required there.

> Using nsReferencedElement doesn't allow us to get rid of the mutation
> observers?

They're still needed in some situations to catch DOM changes in the referenced subtree that should repaint the referring element. Ideally they would go away and we'd use UpdateGraphic and InvalidateRenderingObservers via nsIFrame::AttributeChanged etc whenever rendering could change due to DOM changes.

> You have a comment saying that gradients hardly ever refer to one another. 
> Back when the gradient code was implemented, we saw that inkscape used gradient
> cross-references frequently.

OK, I'll remove the comment. Performance should still be good for that case since nsReferencedElement caches the referenced frame, so the only extra cost I'm adding here is the property lookup.

> nsSVGPathGeometryFrame::UpdateCoveredRegion - you missed deleting the now
> extraneous call to SetupCairoStrokeGeometry.

OK.
(In reply to comment #5)
> (In reply to comment #4)
> > Looks like a nice simplification of the current logic.  A few things came up
> > when looking through the patch:
> > 
> > nsStyleSVG::CalcDifference - linecap and linejoin can change the bounding box
> > of the geometry, necessitating more than just a repaint.
> > 
> > nsStyleSVGReset::CalcDifference - dominantBaseline.  ditto (I think).
> 
> OK, I'll look into what's required there.

Currently linecap and linejoin are handled because nsSVGPathGeometryFrame::DidSetStyleContext calls UpdateGraphic. Not sure about dominantBaseline, but I'm not changing any of this stuff anyway, so if it's broken that's a separate bug.
Posted patch fix v2 (obsolete) — Splinter Review
Tor's comments addressed.
Attachment #339572 - Attachment is obsolete: true
Attachment #339872 - Flags: superreview?(mats.palmgren)
Attachment #339872 - Flags: review?(tor)
Attachment #339572 - Flags: superreview?(mats.palmgren)
Attachment #339572 - Flags: review?(tor)

Comment 8

11 years ago
Comment on attachment 339872 [details] [diff] [review]
fix v2

Nice work!

There's a problem with one of the crash tests though:
  layout/svg/crashtests/308917-1.svg
leads to a circular call sequence that blows up the stack:

nsSVGUtils::PaintChildWithEffects
nsSVGDisplayContainerFrame::PaintSVG
nsSVGUtils::PaintChildWithEffects
nsSVGPatternFrame::PaintPattern
nsSVGPatternFrame::SetupPaintServer
nsSVGGeometryFrame::SetupCairoStroke
nsSVGGlyphFrame::PaintSVG


Other than that, a few nits you may want to address:

> nsSVGClipPathFrame.h
> nsSVGGradientFrame.h
> nsSVGFilterFrame.h
> nsSVGMaskFrame.h
> nsSVGPaintServerFrame.h

+#include "nsSVGEffects.h"

No need to add this #include in those files.


> nsSVGPatternFrame.cpp

This file should have a #include "nsSVGEffects.h"


> nsSVGContainerFrame.h
+class nsSVGRenderingObserverList;
+

This declaration isn't needed.


> nsSVGEffects.cpp

+nsIFrame*
+nsSVGRenderingObserver::GetReferencedFrame(nsIAtom* aFrameType, PRBool* aOK)
+{
+  nsIFrame* frame = GetReferencedFrame();
+  if (frame && frame->GetType() != aFrameType) {
+    frame = nsnull;
+  }
+  if (aOK && !frame) {
+    *aOK = PR_FALSE;
+  }
+  return frame;
+}

I like the old version better, like so:

nsIFrame*
nsSVGRenderingObserver::GetReferencedFrame(nsIAtom* aFrameType, PRBool* aOK)
{
  nsIFrame* frame = GetReferencedFrame();
  if (frame && frame->GetType() == aFrameType) {
    return frame;
  }
  if (aOK) {
    *aOK = PR_FALSE;
  }
  return nsnull;
}


> nsSVGEffects.h

Is there a reason to make nsSVGRenderingObserver::DoUpdate() public?

 
> nsSVGGeometryFrame.cpp

+nsSVGGeometryFrame::GetPaintServer(const nsStyleSVGPaint *aPaint,
...
   nsIURI *uri;
   uri = aPaint->mPaint.mPaintServer;
   if (!uri)
     return nsnull;
 
+  nsSVGPaintingProperty *property =
+    nsSVGEffects::GetPaintingProperty(uri, this, aType);
+  nsIFrame *result = property->GetReferencedFrame();

You could skip the explicit 'uri' null-check and write:
   nsSVGPaintingProperty *property =
     nsSVGEffects::GetPaintingProperty(aPaint->mPaint.mPaintServer,
                                       this, aType);

(as in nsSVGEffects::GetEffectProperties())
But even if you keep the existing 'uri' null-check you still need to
null-check 'property' before using it (OOM).


> nsSVGGradientFrame.cpp
 
In nsSVGGradientFrame::GetGradientTransform()
+  nsSVGGradientElement *element =
+    GetGradientWithAttr(nsGkAtoms::gradientTransform, mContent);

Looks like GetGradientWithAttr() returns null in some loop case,
so we need to null-check 'element' here.
(or did you intend GetGradientWithAttr to return 'aDefault' in that case?)

In nsSVGGradientFrame::GetSpreadMethod()
In nsSVGLinearGradientFrame::GradientLookupAttribute()
In nsSVGGradientFrame::GetGradientUnits()

Ditto.

Please also check callers of GetLinearGradientWithAttr() and
GetRadialGradientWithAttr().


+nsSVGGradientFrame::GetReferencedGradient()
+    property = nsSVGEffects::GetPaintingProperty(targetURI, this, nsGkAtoms::href);

Need an early return if GetPaintingProperty() fails and return null.
(not just OOM in this case, but also if the URI is invalid)


> nsSVGPatternFrame.cpp
+nsSVGPatternFrame::GetReferencedPattern()

Ditto.

 
> nsSVGStopFrame.cpp
In nsSVGStopFrame::DidSetStyleContext()

Should probably call "nsSVGStopFrameBase::DidSetStyleContext()"
just in case someone makes it do something in the future.


> nsSVGUtils.h
 
NS_STATE_SVG_PROPAGATE_TRANSFORM can now be 0x01000000 ?
Attachment #339872 - Flags: superreview?(mats.palmgren) → superreview-
> Looks like GetGradientWithAttr() returns null in some loop case,
> so we need to null-check 'element' here.
> (or did you intend GetGradientWithAttr to return 'aDefault' in that case?)

It should return 'aDefault' in that case, so I'll do that.
This crashtest is interesting. First I found a different crash, which is due to the fact that we aren't wiping out nsSVGRenderingObservers on a frame which is being removed from the document as part of an entire subtree being removed. That was relatively easy to fix, by calling InvalidateDirectRenderingObservers from nsFrame::Destroy.

The next crash is a simple case of a pattern that contains itself. It looks to me like we've never checked for this; the crashtest didn't fail probably because we weren't tracking ID references correctly as the nodes enter and leave the document.
Posted patch fix v3 (obsolete) — Splinter Review
Updated for Mats' comments plus the crash fix as indicated.
Attachment #339405 - Attachment is obsolete: true
Attachment #339872 - Attachment is obsolete: true
Attachment #340043 - Flags: superreview?(mats.palmgren)
Attachment #340043 - Flags: review?(tor)
Attachment #339872 - Flags: review?(tor)

Comment 12

11 years ago
Comment on attachment 340043 [details] [diff] [review]
fix v3

Looks good.  sr=mats

I few nits:

> nsFrame.cpp

+  nsSVGEffects::InvalidateDirectRenderingObservers(this);

Would this work:

   if (!shell->IsDestroying())
     nsSVGEffects::InvalidateDirectRenderingObservers(this);


> nsSVGContainerFrame.cpp

+#include "nsSVGEffects.h"

Not necessary anymore.


> nsSVGGradientFrame.cpp

In nsSVGGradientFrame::GetGradientWithAttr (2nd one):
+    return static_cast<nsSVGGradientElement *>(aDefault);

return grad;


==========

FYI, I get the assertion in bug 443538 when reloading a page with mutually
recursive <pattern>s.  It seems to only occur with the patch.
Attachment #340043 - Flags: superreview?(mats.palmgren) → superreview+

Comment 13

11 years ago
Posted image Testcase
STEPS TO REPRODUCE
1. click the Reload button repeatedly

ACTUAL RESULT
###!!! ASSERTION: No callbacks should be registered while we set up this entry: '!entry->HasContentChangeCallback()', file nsDocument.cpp, line 3292
(In reply to comment #12)
> Would this work:
> 
>    if (!shell->IsDestroying())
>      nsSVGEffects::InvalidateDirectRenderingObservers(this);

It would work for now, while the rendering observers have to be in the same presshell as the observed frames, but soon Boris will land his external references patch and that won't be true anymore. Anyway, the extra check doesn't buy you much --- InvalidateDirectRenderingObservers doesn't make any virtual calls or property lookups until it's checked the NS_FRAME_MAY_BE_TRANSFORMED_OR_HAVE_RENDERING_OBSERVERS state bit.

Comment 15

11 years ago
Fair enough, I wasn't so much concerned with the property lookup,
I just had a feeling that delaying it until the shell deletes its
property table would reduce the work.

So, in an effort to understand why it wouldn't work I had a fresh
look at frame/shell/property destruction and I'm afraid I don't see
how the current patch can work for cross-shell references either.

Consider shell A with child frames x and y, shell B (in y) with
frame z, and let z observe x:

A
  x
  y
    B
      z

Now destroy A. In x->Destroy() we will InvalidateViaReferencedFrame()
on z's rendering observer, nulling its mReferencedFrame, then DoUpdate()
where mFramePresShell (B) is still alive but mReferencedFrame is now
null so we don't RemoveRenderingObserver().
Then A continues to destroy y, which destroys B, which destroys
its frames and after that z's observer property: so we run
~nsSVGRenderingObserver() where mReferencedFrame is null and again
we avoid the RemoveRenderingObserver() call.

With the frame tree destroyed, A now destroys its property table which
contains x's observer list property: in ~nsSVGRenderingObserverList()
we call InvalidateAll() which will access z's deleted observer object.

Did I miss something?
nsSVGRenderingObserverList::InvalidateAll actually removes all observers from mObservers (by returning PL_DHASH_REMOVE from GatherEnumerator). So x->Destroy() will actually wipe out all observers from the list, and when we get around to ~nsSVGRenderingObserverList on the property, InvalidateAll won't do anything.

Another thing, which doesn't really matter here but is worth mentioning, is that SVG external references don't refer to contained subdocuments but to a different sort of document, "external documents" which are (or rather, will be) attached to the "display document" and indexed by URI rather than having an element associated with them. So your scenario can't happen the way you described. (But at some point we might introduce features that would lead to your scenario, so I want to be able to handle it.)
Blocks: 454945
Blocks: 455314

Comment 17

11 years ago
Ah, that makes it very robust then.  Thanks for your patience explaining it.
BTW, the comment on nsSVGRenderingObserverList still mentions
nsSVGContainerFrame::RemoveFrame.
Yeah, I'll fix that comment. Just need a review from tor (nudge :-) ).
Comment on attachment 340043 [details] [diff] [review]
fix v3

tor sounds busy --- Robert, if you get to this first, go for it

(Also, reading this patch would give you a head start on fixing up your markers patch)
Attachment #340043 - Flags: review?(longsonr)
Attachment #340043 - Flags: review?(tor)
Attachment #340043 - Flags: review?(longsonr)
Attachment #340043 - Flags: review+
Comment on attachment 340043 [details] [diff] [review]
fix v3


>--- a/layout/svg/base/src/Makefile.in
>+++ b/layout/svg/base/src/Makefile.in
>@@ -99,20 +99,21 @@ CPPSRCS		= \
> 		$(NULL)
> 
> include $(topsrcdir)/config/config.mk
> 
> # we don't want the shared lib, but we want to force the creation of a static lib.
> FORCE_STATIC_LIB = 1
> 
> EXPORTS = \
>+  nsSVGEffects.h \
>+  nsSVGFilterInstance.h \
>+  nsSVGForeignObjectFrame.h \
> 	nsSVGIntegrationUtils.h \
> 	nsSVGUtils.h \
>-	nsSVGFilterInstance.h \
>-	nsSVGForeignObjectFrame.h \
> 	$(NULL)

Presumably the lines above should contain tabs rather than spaces.

>-void
>-nsSVGFilterProperty::DoUpdate()
>+static void
>+InvalidateAllContinuations(nsIFrame* aFrame)
> {
>-  nsSVGOuterSVGFrame *outerSVGFrame = nsSVGUtils::GetOuterSVGFrame(mFrame);
>-  if (outerSVGFrame) {
>-    outerSVGFrame->Invalidate(mFilterRect);
>-    UpdateRect();
>-    outerSVGFrame->Invalidate(mFilterRect);
>+  for (nsIFrame* f = aFrame; f; f = f->GetNextContinuation()) {
>+    f->Invalidate(f->GetOverflowRect());

f->InvalidateOverflowRect();

>-void
>-nsSVGClipPathProperty::DoUpdate()
>-{

>+    InvalidateAllContinuations(mFrame);
>+    // Reflow so that changes in the filter overflow area get picked up
>+    mFrame->PresContext()->PresShell()->FrameNeedsReflow(
>+         mFrame, nsIPresShell::eResize, NS_FRAME_IS_DIRTY);

Why not mFramePresShell->FrameNeedsReflow(... ?

>+  /**
>+   * @param aOK this is only for the convenience of callers. We set *aOK to false
>+   * if this function returns null.
>+   */
>+  nsIFrame* GetReferencedFrame(nsIAtom* aFrameType, PRBool* aOK);
>+

Doesn't aOK distinguish between the cases:
a) the frame does not exist because the effect has not been configured
b) a frame exists but is invalid e.g. mask attribute pointing to a clipPath frame. In this case we should not draw the referencing frame.

>+++ b/layout/svg/base/src/nsSVGPathGeometryFrame.cpp

>-  if (!isHit && (mask & HITTEST_MASK_STROKE)) {
>-    SetupCairoStrokeHitGeometry(&context);
>-    isHit = context.PointInStroke(userSpacePoint);
>+  if (!isHit && (mask & HITTEST_MASK_STROKE) &&
>+      SetupCairoStrokeHitGeometry(&context)) {
>+      isHit = context.PointInStroke(userSpacePoint);

Last line indented two spaces too much.

>+nsSVGPatternFrame *
>+nsSVGPatternFrame::GetReferencedPattern()
>+{
>+  if (mNoHRefURI)
>+    return nsnull;
>+
>+  nsSVGPaintingProperty *property =
>+    static_cast<nsSVGPaintingProperty*>(GetProperty(nsGkAtoms::href));
>+
>+  if (!property) {
>+    // Fetch our gradient element's xlink:href attribute

s/gradient/pattern/

> void
> nsSVGUtils::UpdateEffects(nsIFrame *aFrame)
> {
>   aFrame->DeleteProperty(nsGkAtoms::filter);
>   aFrame->DeleteProperty(nsGkAtoms::mask);
>   aFrame->DeleteProperty(nsGkAtoms::clipPath);
>+  aFrame->DeleteProperty(nsGkAtoms::fill);
>+  aFrame->DeleteProperty(nsGkAtoms::stroke);
> }

This method exists in nsSVGUtils and nsSVGEffects. Could you just pick one and ditch the other. As far as I can see the only caller to either of these is in nsCSSFrameConstructor.cpp.

r=longsonr assuming review comments addressed.
(In reply to comment #20)
> Doesn't aOK distinguish between the cases:
> a) the frame does not exist because the effect has not been configured
> b) a frame exists but is invalid e.g. mask attribute pointing to a clipPath
> frame. In this case we should not draw the referencing frame.

By this I meant that I think that the method comment should say something along these lines.
(In reply to comment #20)
> >+  /**
> >+   * @param aOK this is only for the convenience of callers. We set *aOK to false
> >+   * if this function returns null.
> >+   */
> >+  nsIFrame* GetReferencedFrame(nsIAtom* aFrameType, PRBool* aOK);
> >+
> 
> Doesn't aOK distinguish between the cases:
> a) the frame does not exist because the effect has not been configured
> b) a frame exists but is invalid e.g. mask attribute pointing to a clipPath
> frame. In this case we should not draw the referencing frame.

It does, in nsSVGEffects::GetClipPathFrame/GetMaskFrame/GetFilterFrame. But here in nsSVGFilterProperty, it's just what I said in the comment --- the nsSVGFilterProperty wouldn't exist if a filter hadn't been requested.

So I just removed aOK from that method to avoid confusion.
This is the patch I want to check in.
Attachment #340043 - Attachment is obsolete: true
Attachment #341044 - Flags: superreview+
Attachment #341044 - Flags: review+
Pushed 1c681814281d
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
This was backed out, on suspicion of causing regressions, but then relanded again today. Looks good this time.

Updated

11 years ago
Depends on: 458068
Duplicate of this bug: 372579
Depends on: 523481
You need to log in before you can comment on or make changes to this bug.