572689 - nsSVGEffects changes for -moz-element

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[Markus Stange [:mstange]]

Attachment: part 1: element accessor and recursion guards

-- Add an mInUse flag to nsSVGRenderingObserver
-- Add AcquireReferencedFrame() and ReleaseReferencedFrame() setting it to true and false
-- Add GetReferencedElement() because we want to use nsSVGRenderingObserver even for elements without frames.

Comment 2

[Markus Stange [:mstange]]

Attachment: part 2: Add BackgroundImageProperty and multiple observers per property

Rendering observers are put into the frame's property table - one observer per property. That's enough for normal SVG properties.
However, background-image can reference multiple elements. So instead of putting a rendering observer directly into the property, I'm creating a hashtable of observers, keyed by observation URI, into the property.

This adds some minor code duplication. In theory, the other properties wouldn't notice if they were put in such a hashtable, too. Should I convert all of them to use the hashtable?

Comment 3

[Markus Stange [:mstange]]

Attachment: part 3: pass aReferenceImage = PR_TRUE to nsReferenceElement::Reset for the BackgroundImageProperty

This makes use of an additional parameter to nsReferencedElement::Reset that I added in bug 572688 - it means that mozSetImageElement overrides should be honored. It's only set to true for aProperty == nsSVGEffects::BackgroundImageProperty().

Comment 4

[Markus Stange [:mstange]]

Attachment: part 4: listen for "load" and "timeupdate" on the referenced element

This is to make images and videos without frames work. Instead of only doing AddMutationObserver, call StartMonitoring() which also listens for "load" and "timeupdate" events.

The lifecycle considerations here are a little tricky. nsSVGRenderingObservers should only have a right to live while the frame they were created for is alive. That was the case up to now: After being created, they're put in the frame's property table, and when the frame dies, the property table is emptied, which calls our DestroySupports method, which releases the one and only reference to the observer, and the observer dies. However, when the observer listens for events, the monitored element takes another reference to the observer. So we need to make sure these references are unlinked before our frame goes away - otherwise we'd leak the observer, or we'd have a stale mFrame pointer in it which might crash. (There's a crashtest somewhere that found this.)

I'm calling StartMonitoring() after the call to aCreate. We can't call it directly from the constructor because we pass "this" to things that want to AddRef() us, and calling virtual methods is hairy while you're in the superclass constructor.

Comment 5

[Robert Longson [:longsonr]]

Comment on attachment 451953 [details] [diff] [review]
part 1: element accessor and recursion guards

>diff --git a/layout/svg/base/src/nsSVGEffects.cpp b/layout/svg/base/src/nsSVGEffects.cpp
>--- a/layout/svg/base/src/nsSVGEffects.cpp
>+++ b/layout/svg/base/src/nsSVGEffects.cpp
>@@ -72,17 +72,18 @@ NS_IMPL_ISUPPORTS1(nsSVGRenderingObserve
> #pragma warning(push)
> #pragma warning(disable:4355)
> #endif
> nsSVGRenderingObserver::nsSVGRenderingObserver(nsIURI *aURI,
>                                                nsIFrame *aFrame)
>   : mElement(this), mFrame(aFrame),
>     mFramePresShell(aFrame->PresContext()->PresShell()),
>     mReferencedFrame(nsnull),
>-    mReferencedFramePresShell(nsnull)
>+    mReferencedFramePresShell(nsnull),
>+    mInUse(PR_FALSE)

class member initialisation and declaration should be in the same order. The order above should be fixed.

>   nsIPresShell *mFramePresShell;
>   nsIFrame *mReferencedFrame;
>   nsIPresShell *mReferencedFramePresShell;
>+  // True if this property reference is currently "in use" for painting.
>+  // This is used to prevent reentry and infinite recursion.
>+  PRPackedBool mInUse;
> };

Comment 6

[Markus Stange [:mstange]]

Huh? Forgive me if I'm blind, but aren't they already in the same order?

Comment 7

[Robert Longson [:longsonr]]

Yes they are. I don't know what I was thinking. Sorry aboutbthat.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 451953 [details] [diff] [review]
part 1: element accessor and recursion guards

Currently SVG patterns have a flag in the pattern frame that prevents infinite recursion. I think we need to be consistent and either fix patterns to rely on nsSVGRenderingObserver::mInUse instead, or use the patternframe approach instead of mInUse here.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #2)
> This adds some minor code duplication. In theory, the other properties wouldn't
> notice if they were put in such a hashtable, too. Should I convert all of them
> to use the hashtable?

Yeah, let's just have a single hash and get rid of this plethora of frame properties! Good idea!

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, why do we need part 4? Shouldn't loads and video frame updates invalidate the rendering observers by calling nsSVGRenderingObserverList::InvalidateAll? Elements that don't have frames but still get used (images, canvases, videos) should call that directly when they update, while for elements with frames I presume somewhere in your patch queue is the patch that makes nsFrame::InvalidateInternal notifying rendering observers automatically?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Shouldn't loads and video frame updates invalidate
> the rendering observers by calling nsSVGRenderingObserverList::InvalidateAll?

I mean, instead of part 4, I think we should have something like that.

Comment 12

[Markus Stange [:mstange]]

(In reply to comment #9)
> (In reply to comment #2)
> > This adds some minor code duplication. In theory, the other properties wouldn't
> > notice if they were put in such a hashtable, too. Should I convert all of them
> > to use the hashtable?
> 
> Yeah, let's just have a single hash and get rid of this plethora of frame
> properties! Good idea!

That's actually not what I meant. I meant creating a hashtable for each frame property, and for all properties that are not BackgroundImageProperty this hashtable would always only contain one entry. That's not very efficient but it would simplify the code.

Putting all types of rendering observers in a single hash sounds good but I'm not sure how it would work. The hashtable is keyed by URI, so you couldn't have two different svg properties (say mask and filter) listen to the same element, could you? These properties need different concrete classes (nsSVGPaintingProperty vs nsSVGFilterProperty).

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess we'd need one hashtable per type. It still sounds like a net win, if it's simple enough.

Comment 14

[Markus Stange [:mstange]]

(In reply to comment #8)
> (From update of attachment 451953 [details] [diff] [review])
> Currently SVG patterns have a flag in the pattern frame that prevents infinite
> recursion. I think we need to be consistent and either fix patterns to rely on
> nsSVGRenderingObserver::mInUse instead,

I think I'll do that.

> or use the patternframe approach instead of mInUse here.

Where would that flag live, then? On nsIFrame?

Does the SVG spec define what should happen in the recursive case? Right now patterns that are already in use just draw nothing, but if something references a pattern frame that doesn't exist, it will use a fallback color (see SetupFallbackOrPaintColor in nsSVGGeometryFrame.cpp). If I converted patterns to use AcquireFrame(), an in-use observer would return null, so we'd go the fallback color route.

Comment 15

[Markus Stange [:mstange]]

(In reply to comment #10)
> Hmm, why do we need part 4? Shouldn't loads and video frame updates invalidate
> the rendering observers by calling nsSVGRenderingObserverList::InvalidateAll?
> Elements that don't have frames but still get used (images, canvases, videos)
> should call that directly when they update

Unfortunately, rendering observers operate on nsIFrames at the moment, and the observer list is stored in the frame's property table. Should I convert that to operate on elements instead? I see that nsINode has a property table, too, so I could use that.

> while for elements with frames I
> presume somewhere in your patch queue is the patch that makes
> nsFrame::InvalidateInternal notifying rendering observers automatically?

Exactly, that's in bug 506826 part 4.

Comment 16

[Robert Longson [:longsonr]]

(In reply to comment #14)
> 
> Does the SVG spec define what should happen in the recursive case? Right now
> patterns that are already in use just draw nothing, but if something references
> a pattern frame that doesn't exist, it will use a fallback color (see
> SetupFallbackOrPaintColor in nsSVGGeometryFrame.cpp). If I converted patterns
> to use AcquireFrame(), an in-use observer would return null, so we'd go the
> fallback color route.

That counts as an error and the specification says don't draw objects that are in error.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #14)
> (In reply to comment #8)
> > (From update of attachment 451953 [details] [diff] [review] [details])
> > Currently SVG patterns have a flag in the pattern frame that prevents infinite
> > recursion. I think we need to be consistent and either fix patterns to rely on
> > nsSVGRenderingObserver::mInUse instead,
> 
> I think I'll do that.

Sounds good.

> Does the SVG spec define what should happen in the recursive case? Right now
> patterns that are already in use just draw nothing, but if something references
> a pattern frame that doesn't exist, it will use a fallback color (see
> SetupFallbackOrPaintColor in nsSVGGeometryFrame.cpp). If I converted patterns
> to use AcquireFrame(), an in-use observer would return null, so we'd go the
> fallback color route.

That sounds OK to me. I don't know that the spec says what to do for this kind of recursion.

(In reply to comment #15)
> Unfortunately, rendering observers operate on nsIFrames at the moment, and the
> observer list is stored in the frame's property table. Should I convert that to
> operate on elements instead? I see that nsINode has a property table, too, so I
> could use that.

Good point. I think we could put an nsSVGRenderingObserverList in that table, actually.

Comment 18

[Markus Stange [:mstange]]

(In reply to comment #16)
> (In reply to comment #14)
> > 
> > Does the SVG spec define what should happen in the recursive case? Right now
> > patterns that are already in use just draw nothing, but if something references
> > a pattern frame that doesn't exist, it will use a fallback color (see
> > SetupFallbackOrPaintColor in nsSVGGeometryFrame.cpp). If I converted patterns
> > to use AcquireFrame(), an in-use observer would return null, so we'd go the
> > fallback color route.
> 
> That counts as an error and the specification says don't draw objects that are
> in error.

But why is the fallback used in the case where the referenced frame doesn't exist, then? Isn't that also an error?

Comment 19

[Markus Stange [:mstange]]

Comment on attachment 451954 [details] [diff] [review]
part 2: Add BackgroundImageProperty and multiple observers per property

(In reply to comment #13)
> I guess we'd need one hashtable per type. It still sounds like a net win, if
> it's simple enough.

Looks like I was wrong all along... keying them by URI instead of by property name doesn't seem to be such a good idea after all. You'd have to supply the URI every time you want to get the property, and things like GetClipPathFrame are called from places where the URI isn't readily available.

Comment 20

[Markus Stange [:mstange]]

(In reply to comment #17)
> (In reply to comment #15)
> > Unfortunately, rendering observers operate on nsIFrames at the moment, and the
> > observer list is stored in the frame's property table. Should I convert that to
> > operate on elements instead? I see that nsINode has a property table, too, so I
> > could use that.
> 
> Good point. I think we could put an nsSVGRenderingObserverList in that table,
> actually.

I've started doing this and hit a small problem: Moving the observers to the node means that I have to add a node flag for may_have_rendering_observers. Unfortunately, it looks like all node bits are taken. What should I do?

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should have 64-bit flags. On 64-bit machines we already allocate 64 bits for PtrBits, so this would only add 32 bits to each node on 32bit machines.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 451954 [details] [diff] [review]
part 2: Add BackgroundImageProperty and multiple observers per property

+    if (prop && !hashtable->Put(aURI, prop)) {
+      delete prop;
+      return nsnull;
+    }

I don't think we need to handle Put failures in the new infallible malloc world.

Comment 23

[Boris Zbarsky [:bzbarsky]]

I'd be ok with 64-bit flags, I think.  I can also free up a flag by merging some of the lazy frame stuff in with style system stuff; it'd just take a few days.  Either way works for me....

The other thing we could do is just move the slots pointer and flags apart; that costs us space on both architectures but speeds up flag access and frees up the has_slots flag immediately.

Comment 24

[Markus Stange [:mstange]]

Filed bug 581177 on making node flags 64-bit.

Comment 25

[Markus Stange [:mstange]]

(In reply to comment #20)
> > Good point. I think we could put an nsSVGRenderingObserverList in that table,
> > actually.
> 
> I've started doing this and hit a small problem: Moving the observers to the
> node means that I have to add a node flag for may_have_rendering_observers.

There's another option: we could just stop using a flag and always check for the observer list property. So instead of checking HasFlag(...) we check GetProperty(...) != nsnull. This might be slightly slower, and as far as I can tell the only place where it would hurt us would be frame destruction (nsFrame::DestroyFrom calls InvalidateDirectRenderingObservers).

What performance tests exercise frame destruction?

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know if any of our tests exercise frame destruction much (other than when we tear down the whole frame tree).

I think 64-bit node flags is the way to go.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> What performance tests exercise frame destruction?

Our performance tests?  Or in-the-wild ones?  For our tests, I think comment 26 is correct.  In general, tests that involve DOM or style manipulation would tend to test it.

Comment 28

[Markus Stange [:mstange]]

Attachment: part 1: Make SVG observers observe elements instead of frames

Comment 29

[Markus Stange [:mstange]]

Attachment: part 3: pass aReferenceImage = PR_TRUE to nsReferenceElement::Reset for the BackgroundImageProperty

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What patch adds that extra parameter added to Reset? What does it mean?

Comment 31

[Markus Stange [:mstange]]

The patch in bug 572688 - it means that mozSetImageElement should be allowed to override the ID target. We only want this to be the case for the BackgroundImageProperty, not for other properties.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 464010 [details] [diff] [review]
part 3: pass aReferenceImage = PR_TRUE to nsReferenceElement::Reset for the BackgroundImageProperty

Ah, of course.

Call your parameters here aReferenceImage instead of aWatchImage, I think.

Comment 33

[Markus Stange [:mstange]]

Attachment: part 3: pass aReferenceImage = PR_TRUE to nsReferenceElement::Reset for the BackgroundImageProperty

Renamed the paramater.

Comment 34

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/77d39c005c6f
http://hg.mozilla.org/mozilla-central/rev/1bf9a4c8c8b4
http://hg.mozilla.org/mozilla-central/rev/8b129680a3bb