610594 - "ABORT: F.6.6.3 should prevent this. Will sqrt(-num)!"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ABORT: F.6.6.3 should prevent this. Will sqrt(-num)!: 'numerator >= 0', file content/svg/content/src/SVGPathData.cpp, line 609

Comment 1

[Robert Longson [:longsonr]]

FWIW, this is what we have now.

      if (numerator < 0.0) {
        // F.6.6 step 3 - |numerator < 0.0| is equivalent to the result of
        // F.6.6.2 (lamedh) being greater than one. What we have here is radii
        // that do not reach between segStart and segEnd, so we need to correct
        // them.
        float lamedh = 1.0 - numerator/(rx*rx*ry*ry); // equiv to eqn F.6.6.2
        float s = sqrt(lamedh);
        rx *= s;  // F.6.6.3
        ry *= s;
        // rx and ry changed, so we have to recompute numerator
        numerator = rx*rx*ry*ry - rx*rx*y1p*y1p - ry*ry*x1p*x1p;
        NS_ABORT_IF_FALSE(numerator >= 0,
                          "F.6.6.3 should prevent this. Will sqrt(-num)!");
      }
      root = sqrt(numerator/(rx*rx*y1p*y1p + ry*ry*x1p*x1p));

The code it replaced simply set root to 0 in this case rather than recomputing the numerator...

  if (numerator < 0.0) {
    //  If mRx , mRy and are such that there is no solution (basically,
    //  the ellipse is not big enough to reach from (x1, y1) to (x2,
    //  y2)) then the ellipse is scaled up uniformly until there is
    //  exactly one solution (until the ellipse is just big enough).

    // -> find factor s, such that numerator' with mRx'=s*mRx and
    //    mRy'=s*mRy becomes 0 :
    float s = (float)sqrt(1.0 - numerator/(mRx*mRx*mRy*mRy));

    mRx *= s;
    mRy *= s;
    root = 0.0;
  }
  else {
    root = (largeArcFlag == sweepFlag ? -1.0 : 1.0) *
      sqrt( numerator/(mRx*mRx*y1dash*y1dash + mRy*mRy*x1dash*x1dash) );
  }

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

The problem is rounding error. Using double instead of float for the intermediary calculations helps in some cases, but in the attached testcase the value still ends up being a very small negative number. Anyway, this reverts to doing what the old code used to do and just setting |root| to 0.0. That at least gets us back to where we were, although I'm still wondering about the values of rx/ry.

Comment 3

[Robert Longson [:longsonr]]

Comment on attachment 498753 [details] [diff] [review]
patch


Again, please check in the testcase as a crashtest when landing.

Comment 4

[Jonathan Watt [:jwatt]]

http://hg.mozilla.org/mozilla-central/rev/6a8cca708a48