Bug 1494820. Stop leaking SVGObserverUtils implementation details into CanvasRenderingContext2D. r=longsonr
authorJonathan Watt <jwatt@jwatt.org>
Mon, 03 Sep 2018 11:24:27 +0100
changeset 438707 426cd9eb299b36219bd300d482cd63822078d60f
parent 438706 71c746601621b203ba1c8d81db53fa30631c8d29
child 438708 d563efb11c415afd6fbec8bacad247b5ed9065b6
push id70086
push usertoros@mozilla.com
push dateFri, 28 Sep 2018 15:41:28 +0000
treeherderautoland@dbe2506cadd0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslongsonr
bugs1494820
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1494820. Stop leaking SVGObserverUtils implementation details into CanvasRenderingContext2D. r=longsonr Differential Revision: https://phabricator.services.mozilla.com/D7148
dom/canvas/CanvasRenderingContext2D.cpp
dom/canvas/CanvasRenderingContext2D.h
layout/svg/SVGObserverUtils.cpp
layout/svg/SVGObserverUtils.h
--- a/dom/canvas/CanvasRenderingContext2D.cpp
+++ b/dom/canvas/CanvasRenderingContext2D.cpp
@@ -953,68 +953,39 @@ public:
   {
     mContext = nullptr;
   }
 
 private:
   CanvasRenderingContext2D* mContext;
 };
 
-class SVGFilterObserverListForCanvas final : public SVGFilterObserverList
-{
-public:
-  SVGFilterObserverListForCanvas(nsTArray<nsStyleFilter>& aFilters,
-                                 Element* aCanvasElement,
-                                 CanvasRenderingContext2D* aContext)
-    : SVGFilterObserverList(aFilters, aCanvasElement)
-    , mContext(aContext)
-  {
-  }
-
-  virtual void OnRenderingChange() override
-  {
-    if (!mContext) {
-      MOZ_CRASH("GFX: This should never be called without a context");
-    }
-    // Refresh the cached FilterDescription in mContext->CurrentState().filter.
-    // If this filter is not at the top of the state stack, we'll refresh the
-    // wrong filter, but that's ok, because we'll refresh the right filter
-    // when we pop the state stack in CanvasRenderingContext2D::Restore().
-    RefPtr<CanvasRenderingContext2D> kungFuDeathGrip(mContext);
-    kungFuDeathGrip->UpdateFilter();
-  }
-
-  void DetachFromContext() { mContext = nullptr; }
-
-private:
-  CanvasRenderingContext2D* mContext;
-};
-
 NS_IMPL_CYCLE_COLLECTING_ADDREF(CanvasRenderingContext2D)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(CanvasRenderingContext2D)
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(CanvasRenderingContext2D)
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(CanvasRenderingContext2D)
   // Make sure we remove ourselves from the list of demotable contexts (raw pointers),
   // since we're logically destructed at this point.
   CanvasRenderingContext2D::RemoveDemotableContext(tmp);
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mCanvasElement)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mDocShell)
   for (uint32_t i = 0; i < tmp->mStyleStack.Length(); i++) {
     ImplCycleCollectionUnlink(tmp->mStyleStack[i].patternStyles[Style::STROKE]);
     ImplCycleCollectionUnlink(tmp->mStyleStack[i].patternStyles[Style::FILL]);
     ImplCycleCollectionUnlink(tmp->mStyleStack[i].gradientStyles[Style::STROKE]);
     ImplCycleCollectionUnlink(tmp->mStyleStack[i].gradientStyles[Style::FILL]);
-    auto filterObserverList =
-      static_cast<SVGFilterObserverListForCanvas*>(tmp->mStyleStack[i].filterObserverList.get());
-    if (filterObserverList) {
-      filterObserverList->DetachFromContext();
+    auto autoSVGFiltersObserver = tmp->mStyleStack[i].autoSVGFiltersObserver.get();
+    if (autoSVGFiltersObserver) {
+      // XXXjwatt: I don't think this call achieves anything.  See the comment
+      // that documents this function.
+      SVGObserverUtils::DetachFromCanvasContext(autoSVGFiltersObserver);
     }
-    ImplCycleCollectionUnlink(tmp->mStyleStack[i].filterObserverList);
+    ImplCycleCollectionUnlink(tmp->mStyleStack[i].autoSVGFiltersObserver);
   }
   for (size_t x = 0 ; x < tmp->mHitRegionsOptions.Length(); x++) {
     RegionInfo& info = tmp->mHitRegionsOptions[x];
     if (info.mElement) {
       ImplCycleCollectionUnlink(info.mElement);
     }
   }
   NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
@@ -1023,17 +994,17 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(CanvasRenderingContext2D)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCanvasElement)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDocShell)
   for (uint32_t i = 0; i < tmp->mStyleStack.Length(); i++) {
     ImplCycleCollectionTraverse(cb, tmp->mStyleStack[i].patternStyles[Style::STROKE], "Stroke CanvasPattern");
     ImplCycleCollectionTraverse(cb, tmp->mStyleStack[i].patternStyles[Style::FILL], "Fill CanvasPattern");
     ImplCycleCollectionTraverse(cb, tmp->mStyleStack[i].gradientStyles[Style::STROKE], "Stroke CanvasGradient");
     ImplCycleCollectionTraverse(cb, tmp->mStyleStack[i].gradientStyles[Style::FILL], "Fill CanvasGradient");
-    ImplCycleCollectionTraverse(cb, tmp->mStyleStack[i].filterObserverList, "Filter Observer List");
+    ImplCycleCollectionTraverse(cb, tmp->mStyleStack[i].autoSVGFiltersObserver, "RAII SVG Filters Observer");
   }
   for (size_t x = 0 ; x < tmp->mHitRegionsOptions.Length(); x++) {
     RegionInfo& info = tmp->mHitRegionsOptions[x];
     if (info.mElement) {
       ImplCycleCollectionTraverse(cb, info.mElement, "Hit region fallback element");
     }
   }
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
@@ -2821,19 +2792,19 @@ CanvasRenderingContext2D::ParseFilter(co
 void
 CanvasRenderingContext2D::SetFilter(const nsAString& aFilter, ErrorResult& aError)
 {
   nsTArray<nsStyleFilter> filterChain;
   if (ParseFilter(aFilter, filterChain, aError)) {
     CurrentState().filterString = aFilter;
     filterChain.SwapElements(CurrentState().filterChain);
     if (mCanvasElement) {
-      CurrentState().filterObserverList =
-        new SVGFilterObserverListForCanvas(CurrentState().filterChain,
-                                           mCanvasElement, this);
+      CurrentState().autoSVGFiltersObserver =
+        SVGObserverUtils::ObserveFiltersForCanvasContext(this, mCanvasElement,
+                                                   CurrentState().filterChain);
       UpdateFilter();
     }
   }
 }
 
 class CanvasUserSpaceMetrics : public UserSpaceMetricsWithSize
 {
 public:
--- a/dom/canvas/CanvasRenderingContext2D.h
+++ b/dom/canvas/CanvasRenderingContext2D.h
@@ -40,17 +40,16 @@ class SourceSurface;
 
 namespace dom {
 class HTMLImageElementOrSVGImageElementOrHTMLCanvasElementOrHTMLVideoElementOrImageBitmap;
 typedef HTMLImageElementOrSVGImageElementOrHTMLCanvasElementOrHTMLVideoElementOrImageBitmap CanvasImageSource;
 class ImageData;
 class StringOrCanvasGradientOrCanvasPattern;
 class OwningStringOrCanvasGradientOrCanvasPattern;
 class TextMetrics;
-class SVGFilterObserverListForCanvas;
 class CanvasPath;
 
 extern const mozilla::gfx::Float SIGMA_MAX;
 
 template<typename T> class Optional;
 
 struct CanvasBidiProcessor;
 class CanvasRenderingContext2DUserData;
@@ -557,16 +556,23 @@ public:
   // return true and fills in the bound rect if element has a hit region.
   bool GetHitRegionRect(Element* aElement, nsRect& aRect) override;
 
   void OnShutdown();
 
   // Check the global setup, as well as the compositor type:
   bool AllowOpenGLCanvas() const;
 
+  /**
+   * Update CurrentState().filter with the filter description for
+   * CurrentState().filterChain.
+   * Flushes the PresShell, so the world can change if you call this function.
+   */
+  void UpdateFilter();
+
 protected:
   nsresult GetImageDataArray(JSContext* aCx, int32_t aX, int32_t aY,
                              uint32_t aWidth, uint32_t aHeight,
                              nsIPrincipal& aSubjectPrincipal,
                              JSObject** aRetval);
 
   nsresult PutImageData_explicit(int32_t aX, int32_t aY, uint32_t aW, uint32_t aH,
                                  dom::Uint8ClampedArray* aArray,
@@ -723,23 +729,16 @@ protected:
   mozilla::gfx::SurfaceFormat GetSurfaceFormat() const;
 
   /**
    * Returns true if we know for sure that the pattern for a given style is opaque.
    * Usefull to know if we can discard the content below in certain situations.
    */
   bool PatternIsOpaque(Style aStyle) const;
 
-  /**
-   * Update CurrentState().filter with the filter description for
-   * CurrentState().filterChain.
-   * Flushes the PresShell, so the world can change if you call this function.
-   */
-  void UpdateFilter();
-
   nsLayoutUtils::SurfaceFromElementResult
     CachedSurfaceFromElement(Element* aElement);
 
   void DrawImage(const CanvasImageSource& aImgElt,
                  double aSx, double aSy, double aSw, double aSh,
                  double aDx, double aDy, double aDw, double aDh,
                  uint8_t aOptional_argc, mozilla::ErrorResult& aError);
 
@@ -1047,17 +1046,17 @@ protected:
           dash(aOther.dash),
           dashOffset(aOther.dashOffset),
           op(aOther.op),
           fillRule(aOther.fillRule),
           lineCap(aOther.lineCap),
           lineJoin(aOther.lineJoin),
           filterString(aOther.filterString),
           filterChain(aOther.filterChain),
-          filterObserverList(aOther.filterObserverList),
+          autoSVGFiltersObserver(aOther.autoSVGFiltersObserver),
           filter(aOther.filter),
           filterAdditionalImages(aOther.filterAdditionalImages),
           filterSourceGraphicTainted(aOther.filterSourceGraphicTainted),
           imageSmoothingEnabled(aOther.imageSmoothingEnabled),
           fontExplicitLanguage(aOther.fontExplicitLanguage)
     { }
 
     void SetColorStyle(Style aWhichStyle, nscolor aColor)
@@ -1125,17 +1124,19 @@ protected:
 
     mozilla::gfx::CompositionOp op;
     mozilla::gfx::FillRule fillRule;
     mozilla::gfx::CapStyle lineCap;
     mozilla::gfx::JoinStyle lineJoin;
 
     nsString filterString;
     nsTArray<nsStyleFilter> filterChain;
-    RefPtr<SVGFilterObserverList> filterObserverList;
+    // RAII object that we obtain when we start to observer SVG filter elements
+    // for rendering changes.  When released we stop observing the SVG elements.
+    nsCOMPtr<nsISupports> autoSVGFiltersObserver;
     mozilla::gfx::FilterDescription filter;
     nsTArray<RefPtr<mozilla::gfx::SourceSurface>> filterAdditionalImages;
 
     // This keeps track of whether the canvas was "tainted" or not when
     // we last used a filter. This is a security measure, whereby the
     // canvas is flipped to write-only if a cross-origin image is drawn to it.
     // This is to stop bad actors from reading back data they shouldn't have
     // access to.
@@ -1157,17 +1158,16 @@ protected:
     return mStyleStack[mStyleStack.Length() - 1];
   }
 
   inline const ContextState& CurrentState() const {
     return mStyleStack[mStyleStack.Length() - 1];
   }
 
   friend class CanvasGeneralPattern;
-  friend class SVGFilterObserverListForCanvas;
   friend class AdjustedTarget;
   friend class AdjustedTargetForShadow;
   friend class AdjustedTargetForFilter;
 
   // other helpers
   void GetAppUnitsValues(int32_t* aPerDevPixel, int32_t* aPerCSSPixel)
   {
     // If we don't have a canvas element, we just return something generic.
--- a/layout/svg/SVGObserverUtils.cpp
+++ b/layout/svg/SVGObserverUtils.cpp
@@ -3,16 +3,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 // Main header first:
 #include "SVGObserverUtils.h"
 
 // Keep others in (case-insensitive) order:
+#include "mozilla/dom/CanvasRenderingContext2D.h"
 #include "mozilla/RestyleManager.h"
 #include "nsCSSFrameConstructor.h"
 #include "nsISupportsImpl.h"
 #include "nsSVGClipPathFrame.h"
 #include "nsSVGPaintServerFrame.h"
 #include "nsSVGFilterFrame.h"
 #include "nsSVGMaskFrame.h"
 #include "nsIReflowCallback.h"
@@ -388,16 +389,47 @@ SVGFilterObserverListForCSSProp::OnRende
   // Don't need to request UpdateOverflow if we're being reflowed.
   if (!(frame->GetStateBits() & NS_FRAME_IN_REFLOW)) {
     changeHint |= nsChangeHint_UpdateOverflow;
   }
   frame->PresContext()->RestyleManager()->PostRestyleEvent(
     frame->GetContent()->AsElement(), nsRestyleHint(0), changeHint);
 }
 
+
+class SVGFilterObserverListForCanvasContext final : public SVGFilterObserverList
+{
+public:
+  SVGFilterObserverListForCanvasContext(CanvasRenderingContext2D* aContext,
+                                        Element* aCanvasElement,
+                                        nsTArray<nsStyleFilter>& aFilters)
+    : SVGFilterObserverList(aFilters, aCanvasElement)
+    , mContext(aContext)
+  {}
+
+  virtual void OnRenderingChange() override
+  {
+    if (!mContext) {
+      MOZ_CRASH("GFX: This should never be called without a context");
+    }
+    // Refresh the cached FilterDescription in mContext->CurrentState().filter.
+    // If this filter is not at the top of the state stack, we'll refresh the
+    // wrong filter, but that's ok, because we'll refresh the right filter
+    // when we pop the state stack in CanvasRenderingContext2D::Restore().
+    RefPtr<CanvasRenderingContext2D> kungFuDeathGrip(mContext);
+    kungFuDeathGrip->UpdateFilter();
+  }
+
+  void DetachFromContext() { mContext = nullptr; }
+
+private:
+  CanvasRenderingContext2D* mContext;
+};
+
+
 void
 SVGMarkerObserver::OnRenderingChange()
 {
   nsSVGRenderingObserverProperty::OnRenderingChange();
 
   nsIFrame* frame = mFrameReference.Get();
   if (!frame)
     return;
@@ -645,16 +677,33 @@ SVGObserverUtils::GetAndObserveFilters(n
     if (aFilterFrames) {
       aFilterFrames->AppendElement(filter);
     }
   }
 
   return eHasRefsAllValid;
 }
 
+already_AddRefed<nsISupports>
+SVGObserverUtils::ObserveFiltersForCanvasContext(CanvasRenderingContext2D* aContext,
+                                                 Element* aCanvasElement,
+                                                 nsTArray<nsStyleFilter>& aFilters)
+{
+  return do_AddRef(new SVGFilterObserverListForCanvasContext(aContext,
+                                                             aCanvasElement,
+                                                             aFilters));
+}
+
+void
+SVGObserverUtils::DetachFromCanvasContext(nsISupports* aAutoObserver)
+{
+  static_cast<SVGFilterObserverListForCanvasContext*>(aAutoObserver)->
+    DetachFromContext();
+}
+
 SVGGeometryElement*
 SVGObserverUtils::GetTextPathsReferencedPath(nsIFrame* aTextPathFrame)
 {
   SVGTextPathObserver* property =
     aTextPathFrame->GetProperty(SVGObserverUtils::HrefAsTextPathProperty());
 
   if (!property) {
     nsIContent* content = aTextPathFrame->GetContent();
--- a/layout/svg/SVGObserverUtils.h
+++ b/layout/svg/SVGObserverUtils.h
@@ -31,16 +31,17 @@ class nsIURI;
 class nsSVGClipPathFrame;
 class nsSVGMarkerFrame;
 class nsSVGPaintServerFrame;
 class nsSVGFilterFrame;
 class nsSVGMaskFrame;
 namespace mozilla {
 class SVGFilterObserverList;
 namespace dom {
+class CanvasRenderingContext2D;
 class SVGGeometryElement;
 }
 }
 
 namespace mozilla {
 
 /*
  * This class contains URL and referrer information (referrer and referrer
@@ -525,16 +526,17 @@ public:
 
 private:
   nsTHashtable<nsPtrHashKey<SVGRenderingObserver>> mObservers;
 };
 
 class SVGObserverUtils
 {
 public:
+  typedef mozilla::dom::CanvasRenderingContext2D CanvasRenderingContext2D;
   typedef mozilla::dom::Element Element;
   typedef dom::SVGGeometryElement SVGGeometryElement;
   typedef nsInterfaceHashtable<nsRefPtrHashKey<URLAndReferrerInfo>,
     nsIMutationObserver> URIObserverHashtable;
 
   using PaintingPropertyDescriptor =
     const mozilla::FramePropertyDescriptor<nsSVGPaintingProperty>*;
   using URIObserverHashtablePropertyDescriptor =
@@ -726,16 +728,48 @@ public:
    * bugs, or cause later invalidation problems.  However, let's not change
    * that behavior just yet due to the regression potential.
    */
   static ReferenceState
   GetAndObserveFilters(nsIFrame* aFilteredFrame,
                        nsTArray<nsSVGFilterFrame*>* aFilterFrames);
 
   /**
+   * Starts observing filters for a <canvas> element's CanvasRenderingContext2D.
+   *
+   * Returns a RAII object that the caller should make sure is released once
+   * the CanvasRenderingContext2D is no longer using them (that is, when the
+   * CanvasRenderingContext2D "drawing style state" on which the filters were
+   * set is destroyed or has its filter style reset).
+   *
+   * XXXjwatt: It's a bit unfortunate that both we and
+   * CanvasRenderingContext2D::UpdateFilter process the list of nsStyleFilter
+   * objects separately.  It would be better to refactor things so that we only
+   * do that work once.
+   */
+  static already_AddRefed<nsISupports>
+  ObserveFiltersForCanvasContext(CanvasRenderingContext2D* aContext,
+                                 Element* aCanvasElement,
+                                 nsTArray<nsStyleFilter>& aFilters);
+
+  /**
+   * Called when cycle collecting CanvasRenderingContext2D, and requires the
+   * RAII object returned from ObserveFiltersForCanvasContext to be passed in.
+   *
+   * XXXjwatt: I don't think this is doing anything useful.  All we do under
+   * this function is clear a raw C-style (i.e. not strong) pointer.  That's
+   * clearly not helping in breaking any cycles.  The fact that we MOZ_CRASH
+   * in OnRenderingChange if that pointer is null indicates that this isn't
+   * even doing anything useful in terms of preventing further invalidation
+   * from any observed filters.
+   */
+  static void
+  DetachFromCanvasContext(nsISupports* aAutoObserver);
+
+  /**
    * Get the SVGGeometryElement that is referenced by aTextPathFrame, and make
    * aTextPathFrame start observing rendering changes to that element.
    */
   static SVGGeometryElement*
   GetTextPathsReferencedPath(nsIFrame* aTextPathFrame);
 
   /**
    * Make aTextPathFrame stop observing rendering changes to the