Bug 1164766 - Clean up error checking in CanvasRenderingContext2D. r=mats, a=2.1s
authorMarkus Stange <mstange@themasta.com>
Wed, 26 Aug 2015 13:22:01 -0400
changeset 222376 b4135ecf90a3aca815098197f8426621dbe282cc
parent 222375 77e5c0f928eaa330bf93238bdaa827055eca867c
child 222377 b76c8843149c98e5bf63704e8e002a188a2608a6
push id239
push userryanvm@gmail.com
push dateThu, 27 Aug 2015 11:11:07 +0000
treeherdermozilla-b2g34_v2_1s@b4135ecf90a3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats, 2
bugs1164766
milestone34.0
Bug 1164766 - Clean up error checking in CanvasRenderingContext2D. r=mats, a=2.1s
content/html/content/src/HTMLCanvasElement.cpp
dom/canvas/CanvasRenderingContext2D.cpp
dom/canvas/CanvasRenderingContext2D.h
--- a/content/html/content/src/HTMLCanvasElement.cpp
+++ b/content/html/content/src/HTMLCanvasElement.cpp
@@ -788,31 +788,33 @@ HTMLCanvasElement::MozGetIPCContext(cons
 nsresult
 HTMLCanvasElement::UpdateContext(JSContext* aCx, JS::Handle<JS::Value> aNewContextOptions)
 {
   if (!mCurrentContext)
     return NS_OK;
 
   nsIntSize sz = GetWidthHeight();
 
-  nsresult rv = mCurrentContext->SetIsOpaque(HasAttr(kNameSpaceID_None, nsGkAtoms::moz_opaque));
+  nsCOMPtr<nsICanvasRenderingContextInternal> currentContext = mCurrentContext;
+
+  nsresult rv = currentContext->SetIsOpaque(HasAttr(kNameSpaceID_None, nsGkAtoms::moz_opaque));
   if (NS_FAILED(rv)) {
     mCurrentContext = nullptr;
     mCurrentContextId.Truncate();
     return rv;
   }
 
-  rv = mCurrentContext->SetContextOptions(aCx, aNewContextOptions);
+  rv = currentContext->SetContextOptions(aCx, aNewContextOptions);
   if (NS_FAILED(rv)) {
     mCurrentContext = nullptr;
     mCurrentContextId.Truncate();
     return rv;
   }
 
-  rv = mCurrentContext->SetDimensions(sz.width, sz.height);
+  rv = currentContext->SetDimensions(sz.width, sz.height);
   if (NS_FAILED(rv)) {
     mCurrentContext = nullptr;
     mCurrentContextId.Truncate();
     return rv;
   }
 
   return rv;
 }
--- a/dom/canvas/CanvasRenderingContext2D.cpp
+++ b/dom/canvas/CanvasRenderingContext2D.cpp
@@ -617,17 +617,17 @@ CanvasRenderingContext2D::ParseColor(con
     return false;
   }
 
   if (value.IsNumericColorUnit()) {
     // if we already have a color we can just use it directly
     *aColor = value.GetColorValue();
   } else {
     // otherwise resolve it
-    nsIPresShell* presShell = GetPresShell();
+    nsCOMPtr<nsIPresShell> presShell = GetPresShell();
     nsRefPtr<nsStyleContext> parentContext;
     if (mCanvasElement && mCanvasElement->IsInDoc()) {
       // Inherit from the canvas element.
       parentContext = nsComputedDOMStyle::GetStyleContextForElement(
         mCanvasElement, nullptr, presShell);
     }
 
     unused << nsRuleNode::ComputeColor(
@@ -2212,104 +2212,111 @@ CreateFontStyleRule(const nsAString& aFo
   rule.forget(aResult);
   return NS_OK;
 }
 
 void
 CanvasRenderingContext2D::SetFont(const nsAString& font,
                                   ErrorResult& error)
 {
+  SetFontInternal(font, error);
+}
+
+bool
+CanvasRenderingContext2D::SetFontInternal(const nsAString& font,
+                                          ErrorResult& error)
+{
   /*
     * If font is defined with relative units (e.g. ems) and the parent
     * style context changes in between calls, setting the font to the
     * same value as previous could result in a different computed value,
     * so we cannot have the optimization where we check if the new font
     * string is equal to the old one.
     */
 
   if (!mCanvasElement && !mDocShell) {
     NS_WARNING("Canvas element must be non-null or a docshell must be provided");
     error.Throw(NS_ERROR_FAILURE);
-    return;
-  }
-
-  nsIPresShell* presShell = GetPresShell();
+    return false;
+  }
+
+  nsCOMPtr<nsIPresShell> presShell = GetPresShell();
   if (!presShell) {
     error.Throw(NS_ERROR_FAILURE);
-    return;
+    return false;
   }
   nsIDocument* document = presShell->GetDocument();
 
   nsRefPtr<css::StyleRule> rule;
   error = CreateFontStyleRule(font, document, getter_AddRefs(rule));
 
   if (error.Failed()) {
-    return;
+    return false;
   }
 
   css::Declaration *declaration = rule->GetDeclaration();
   // The easiest way to see whether we got a syntax error or whether
   // we got 'inherit' or 'initial' is to look at font-size-adjust,
   // which the shorthand resets to either 'none' or
   // '-moz-system-font'.
   // We know the declaration is not !important, so we can use
   // GetNormalBlock().
   const nsCSSValue *fsaVal =
     declaration->GetNormalBlock()->ValueFor(eCSSProperty_font_size_adjust);
   if (!fsaVal || (fsaVal->GetUnit() != eCSSUnit_None &&
                   fsaVal->GetUnit() != eCSSUnit_System_Font)) {
       // We got an all-property value or a syntax error.  The spec says
       // this value must be ignored.
-    return;
+    return false;
   }
 
   nsTArray< nsCOMPtr<nsIStyleRule> > rules;
   rules.AppendElement(rule);
 
   nsStyleSet* styleSet = presShell->StyleSet();
 
   // have to get a parent style context for inherit-like relative
   // values (2em, bolder, etc.)
   nsRefPtr<nsStyleContext> parentContext;
 
   if (mCanvasElement && mCanvasElement->IsInDoc()) {
-      // inherit from the canvas element
-      parentContext = nsComputedDOMStyle::GetStyleContextForElement(
-              mCanvasElement,
-              nullptr,
-              presShell);
+    // inherit from the canvas element
+    parentContext = nsComputedDOMStyle::GetStyleContextForElement(
+            mCanvasElement,
+            nullptr,
+            presShell);
   } else {
     // otherwise inherit from default (10px sans-serif)
     nsRefPtr<css::StyleRule> parentRule;
     error = CreateFontStyleRule(NS_LITERAL_STRING("10px sans-serif"),
                                 document,
                                 getter_AddRefs(parentRule));
 
     if (error.Failed()) {
-      return;
+      return false;
     }
 
     nsTArray< nsCOMPtr<nsIStyleRule> > parentRules;
     parentRules.AppendElement(parentRule);
     parentContext = styleSet->ResolveStyleForRules(nullptr, parentRules);
   }
 
   if (!parentContext) {
     error.Throw(NS_ERROR_FAILURE);
-    return;
+    return false;
   }
 
   // add a rule to prevent text zoom from affecting the style
   rules.AppendElement(new nsDisableTextZoomStyleRule);
 
   nsRefPtr<nsStyleContext> sc =
       styleSet->ResolveStyleForRules(parentContext, rules);
   if (!sc) {
     error.Throw(NS_ERROR_FAILURE);
-    return;
+    return false;
   }
 
   const nsStyleFont* fontStyle = sc->StyleFont();
 
   NS_ASSERTION(fontStyle, "Could not obtain font style");
 
   nsIAtom* language = sc->StyleFont()->mLanguage;
   if (!language) {
@@ -2350,16 +2357,18 @@ CanvasRenderingContext2D::SetFont(const 
   NS_ASSERTION(CurrentState().fontGroup, "Could not get font group");
   CurrentState().fontGroup->SetTextPerfMetrics(c->GetTextPerfMetrics());
 
   // The font getter is required to be reserialized based on what we
   // parsed (including having line-height removed).  (Older drafts of
   // the spec required font sizes be converted to pixels, but that no
   // longer seems to be required.)
   declaration->GetValue(eCSSProperty_font, CurrentState().font);
+
+  return true;
 }
 
 void
 CanvasRenderingContext2D::SetTextAlign(const nsAString& ta)
 {
   if (ta.EqualsLiteral("start"))
     CurrentState().textAlign = TextAlign::START;
   else if (ta.EqualsLiteral("end"))
@@ -2835,17 +2844,22 @@ CanvasRenderingContext2D::DrawOrMeasureT
 
     isRTL = canvasStyle->StyleVisibility()->mDirection ==
       NS_STYLE_DIRECTION_RTL;
   } else {
     isRTL = GET_BIDI_OPTION_DIRECTION(document->GetBidiOptions()) == IBMBIDI_TEXTDIRECTION_RTL;
   }
 
   gfxFontGroup* currentFontStyle = GetCurrentFontStyle();
-  NS_ASSERTION(currentFontStyle, "font group is null");
+  if (!currentFontStyle) {
+    return NS_ERROR_FAILURE;
+  }
+
+  MOZ_ASSERT(!presShell->IsDestroying(),
+             "GetCurrentFontStyle() should have returned null if the presshell is being destroyed");
 
   // ensure user font set is up to date
   currentFontStyle->
     SetUserFontSet(presShell->GetPresContext()->GetUserFontSet());
 
   if (currentFontStyle->GetStyle()->size == 0.0F) {
     if (aWidth) {
       *aWidth = 0;
@@ -3011,37 +3025,35 @@ CanvasRenderingContext2D::DrawOrMeasureT
 
 gfxFontGroup *CanvasRenderingContext2D::GetCurrentFontStyle()
 {
   // use lazy initilization for the font group since it's rather expensive
   if (!CurrentState().fontGroup) {
     ErrorResult err;
     NS_NAMED_LITERAL_STRING(kDefaultFontStyle, "10px sans-serif");
     static float kDefaultFontSize = 10.0;
-    SetFont(kDefaultFontStyle, err);
-    if (err.Failed()) {
+    nsCOMPtr<nsIPresShell> presShell = GetPresShell();
+    bool fontUpdated = SetFontInternal(kDefaultFontStyle, err);
+    if (err.Failed() || !fontUpdated) {
       gfxFontStyle style;
       style.size = kDefaultFontSize;
       CurrentState().fontGroup =
         gfxPlatform::GetPlatform()->CreateFontGroup(FontFamilyList(eFamily_sans_serif),
                                                     &style,
                                                     nullptr);
       if (CurrentState().fontGroup) {
         CurrentState().font = kDefaultFontStyle;
-
-        nsIPresShell* presShell = GetPresShell();
-        if (presShell) {
+        if (presShell && !presShell->IsDestroying()) {
           CurrentState().fontGroup->SetTextPerfMetrics(
             presShell->GetPresContext()->GetTextPerfMetrics());
         }
       } else {
         NS_ERROR("Default canvas font is invalid");
       }
     }
-
   }
 
   return CurrentState().fontGroup;
 }
 
 //
 // line caps/joins
 //
--- a/dom/canvas/CanvasRenderingContext2D.h
+++ b/dom/canvas/CanvasRenderingContext2D.h
@@ -610,16 +610,19 @@ protected:
   void GetStyleAsUnion(OwningStringOrCanvasGradientOrCanvasPattern& aValue,
                        Style aWhichStyle);
 
   // Returns whether a color was successfully parsed.
   bool ParseColor(const nsAString& aString, nscolor* aColor);
 
   static void StyleColorToString(const nscolor& aColor, nsAString& aStr);
 
+  // Returns whether the font was successfully updated.
+  bool SetFontInternal(const nsAString& font, mozilla::ErrorResult& error);
+
   /**
    * Creates the error target, if it doesn't exist
    */
   static void EnsureErrorTarget();
 
   /* This function ensures there is a writable pathbuilder available, this
    * pathbuilder may be working in user space or in device space or
    * device space.