From cb507d3b884714a00445e21720a4c0c839654859 Mon Sep 17 00:00:00 2001 From: Jonathan Watt Date: Thu, 28 Jan 2016 09:30:47 +0000 Subject: [PATCH] Bug 1223644 - Clean up the nsSVGClipPathFrame reference loop detection code. r=longsonr --- layout/svg/nsSVGClipPathFrame.cpp | 32 +++++----- layout/svg/nsSVGClipPathFrame.h | 97 ++++++++++++++++++++++--------- 2 files changed, 84 insertions(+), 45 deletions(-) diff --git a/layout/svg/nsSVGClipPathFrame.cpp b/layout/svg/nsSVGClipPathFrame.cpp index a69d29a599f..cdf092d199f 100644 --- a/layout/svg/nsSVGClipPathFrame.cpp +++ b/layout/svg/nsSVGClipPathFrame.cpp @@ -39,8 +39,8 @@ nsSVGClipPathFrame::ApplyClipPath(gfxContext& aContext, DrawTarget& aDrawTarget = *aContext.GetDrawTarget(); - // No need for AutoClipPathReferencer since simple clip paths can't create a - // reference loop. + // No need for AutoReferenceLoopDetector since simple clip paths can't create + // a reference loop (they don't reference other clip paths). // Restore current transform after applying clip path: gfxContextMatrixAutoSaveRestore autoRestore(&aContext); @@ -90,14 +90,11 @@ nsSVGClipPathFrame::GetClipMask(gfxContext& aReferenceContext, DrawTarget& aReferenceDT = *aReferenceContext.GetDrawTarget(); - // If the flag is set when we get here, it means this clipPath frame - // has already been used painting the current clip, and the document - // has a clip reference loop. - if (mInUse) { - NS_WARNING("Clip loop detected!"); + AutoReferenceLoopDetector loopDetector; + if (!loopDetector.MarkAsInUse(this)) { + // Reference loop! This reference should be ignored, so return nullptr. return nullptr; } - AutoClipPathReferencer clipRef(this); IntRect devSpaceClipExtents; { @@ -242,14 +239,12 @@ bool nsSVGClipPathFrame::PointIsInsideClipPath(nsIFrame* aClippedFrame, const gfxPoint &aPoint) { - // If the flag is set when we get here, it means this clipPath frame - // has already been used in hit testing against the current clip, - // and the document has a clip reference loop. - if (mInUse) { - NS_WARNING("Clip loop detected!"); - return false; + AutoReferenceLoopDetector loopDetector; + if (!loopDetector.MarkAsInUse(this)) { + // Reference loop! This reference is ignored, so return true (point not + // clipped out). + return true; } - AutoClipPathReferencer clipRef(this); gfxMatrix matrix = GetClipPathTransform(aClippedFrame); if (!matrix.Invert()) { @@ -328,11 +323,10 @@ nsSVGClipPathFrame::IsTrivial(nsISVGChildFrame **aSingleChild) bool nsSVGClipPathFrame::IsValid() { - if (mInUse) { - NS_WARNING("Clip loop detected!"); - return false; + AutoReferenceLoopDetector loopDetector; + if (!loopDetector.MarkAsInUse(this)) { + return false; // Reference loop! } - AutoClipPathReferencer clipRef(this); bool isOK = true; nsSVGEffects::GetEffectProperties(this).GetClipPathFrame(&isOK); diff --git a/layout/svg/nsSVGClipPathFrame.h b/layout/svg/nsSVGClipPathFrame.h index 00f037ad9c0..84ec21c6f83 100644 --- a/layout/svg/nsSVGClipPathFrame.h +++ b/layout/svg/nsSVGClipPathFrame.h @@ -130,35 +130,80 @@ public: */ gfxMatrix GetClipPathTransform(nsIFrame* aClippedFrame); - private: - // A helper class to allow us to paint clip paths safely. The helper - // automatically sets and clears the mInUse flag on the clip path frame - // (to prevent nasty reference loops). It's easy to mess this up - // and break things, so this helper makes the code far more robust. - class MOZ_RAII AutoClipPathReferencer - { - public: - explicit AutoClipPathReferencer(nsSVGClipPathFrame *aFrame - MOZ_GUARD_OBJECT_NOTIFIER_PARAM) - : mFrame(aFrame) { - MOZ_GUARD_OBJECT_NOTIFIER_INIT; - NS_ASSERTION(!mFrame->mInUse, "reference loop!"); - mFrame->mInUse = true; - } - ~AutoClipPathReferencer() { - mFrame->mInUse = false; - } - private: - nsSVGClipPathFrame *mFrame; - MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER - }; - - gfxMatrix mMatrixForChildren; - // recursion prevention flag - bool mInUse; +private: // nsSVGContainerFrame methods: virtual gfxMatrix GetCanvasTM() override; + + /** + * SVG content may contain reference loops where an SVG effect (a clipPath, + * say) may reference itself (directly or indirectly via a reference chain). + * This helper class allows us to detect and break such reference loops when + * applying an effect so that we can safely do so without the reference loop + * causing us to recurse until we run out of stack space and crash. + * The helper automatically sets and clears the mInUse flag on the frame. + */ + class MOZ_RAII AutoReferenceLoopDetector + { + public: + explicit AutoReferenceLoopDetector() + : mFrame(nullptr) +#ifdef DEBUG + , mMarkAsInUseCalled(false) +#endif + {} + + ~AutoReferenceLoopDetector() { + MOZ_ASSERT(mMarkAsInUseCalled, + "Instances of this class are useless if MarkAsInUse() is " + "not called on them"); + if (mFrame) { + mFrame->mInUse = false; + } + } + + /** + * Returns true on success (no reference loop), else returns false on + * failure (aFrame is already in use; that is, there is a reference loop). + */ + MOZ_WARN_UNUSED_RESULT bool MarkAsInUse(nsSVGClipPathFrame* aFrame) { +#ifdef DEBUG + MOZ_ASSERT(!mMarkAsInUseCalled, "Must only be called once"); + mMarkAsInUseCalled = true; +#endif + if (aFrame->mInUse) { + // XXX This is an error in the document, not in Mozilla code, so stop + // using NS_WARNING and send a message to the console instead. + NS_WARNING("clipPath reference loop!"); + return false; + } + aFrame->mInUse = true; + mFrame = aFrame; + return true; + } + + private: + nsSVGClipPathFrame* mFrame; + DebugOnly mMarkAsInUseCalled; + }; + + // Set, during a GetClipMask() call, to the transform that still needs to be + // concatenated to the transform of the DrawTarget that was passed to + // GetClipMask in order to establish the coordinate space that the clipPath + // establishes for its contents (i.e. including applying 'clipPathUnits' and + // any 'transform' attribute set on the clipPath) specifically for clipping + // the frame that was passed to GetClipMask at that moment in time. This is + // set so that if our GetCanvasTM method is called while GetClipMask is + // painting its children, the returned matrix will include the transforms + // that should be used when creating the mask for the frame passed to + // GetClipMask. + // + // Note: The removal of GetCanvasTM is nearly complete, so our GetCanvasTM + // may not even be called soon/any more. + gfxMatrix mMatrixForChildren; + + // Flag used by AutoReferenceLoopDetector to protect against reference loops: + bool mInUse; }; #endif