Extended error reporting for SVG path parsing

third_party/WebKit/Source/core/svg/SVGPathStringSource.cpp

 /*
  * Copyright (C) Research In Motion Limited 2010. All rights reserved.
  * Copyright (C) 2013 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Library General Public License for more details.
  *
  * You should have received a copy of the GNU Library General Public License
  * along with this library; see the file COPYING.LIB.  If not, write to
  * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
  * Boston, MA 02110-1301, USA.
  */
 
 #include "core/svg/SVGPathStringSource.h"
 
 #include "core/svg/SVGParserUtilities.h"
 #include "platform/geometry/FloatPoint.h"
 
 namespace blink {
 
 SVGPathStringSource::SVGPathStringSource(const String& string)
-    : m_string(string)
-    , m_is8BitSource(string.is8Bit())
-    , m_seenError(false)
+    : m_is8BitSource(string.is8Bit())
     , m_previousCommand(PathSegUnknown)
+    , m_string(string)
 {
     ASSERT(!string.isNull());
 
     if (m_is8BitSource) {
         m_current.m_character8 = string.characters8();
         m_end.m_character8 = m_current.m_character8 + string.length();
     } else {
         m_current.m_character16 = string.characters16();
         m_end.m_character16 = m_current.m_character16 + string.length();
     }
@@ skipping 74 matching lines @@
         if (previousCommand == PathSegMoveToRel) {
             nextCommand = PathSegLineToRel;
             return true;
         }
         nextCommand = previousCommand;
         return true;
     }
     return false;
 }
 
+void SVGPathStringSource::setErrorMark(SVGParseStatus status)
+{
+    if (m_error.status() != SVGParseStatus::NoError)
+        return;
+    size_t locus = m_is8BitSource
+        ? m_current.m_character8 - m_string.characters8()
+        : m_current.m_character16 - m_string.characters16();
+    m_error = SVGParsingError(status, locus);
+}
+
 float SVGPathStringSource::parseNumberWithError()
 {
     float numberValue = 0;
+    bool error;
     if (m_is8BitSource)
-        m_seenError |= !parseNumber(m_current.m_character8, m_end.m_character8, numberValue);
+        error = !parseNumber(m_current.m_character8, m_end.m_character8, numberValue);
     else
-        m_seenError |= !parseNumber(m_current.m_character16, m_end.m_character16, numberValue);
+        error = !parseNumber(m_current.m_character16, m_end.m_character16, numberValue);
+    if (UNLIKELY(error))
+        setErrorMark(SVGParseStatus::ExpectedNumber);
     return numberValue;
 }
 
 bool SVGPathStringSource::parseArcFlagWithError()
 {
     bool flagValue = false;
+    bool error;
     if (m_is8BitSource)
-        m_seenError |= !parseArcFlag(m_current.m_character8, m_end.m_character8, flagValue);
+        error = !parseArcFlag(m_current.m_character8, m_end.m_character8, flagValue);
     else
-        m_seenError |= !parseArcFlag(m_current.m_character16, m_end.m_character16, flagValue);
+        error = !parseArcFlag(m_current.m_character16, m_end.m_character16, flagValue);
+    if (UNLIKELY(error))
+        setErrorMark(SVGParseStatus::ExpectedArcFlag);
     return flagValue;
 }
 
 SVGPathSegType SVGPathStringSource::peekSegmentType()
 {
     ASSERT(hasMoreData());
     // This won't work in all cases because of the state required to "detect" implicit commands.
     unsigned lookahead = m_is8BitSource ? *m_current.m_character8 : *m_current.m_character16;
-    return parseSVGSegmentTypeHelper(lookahead);
+    SVGPathSegType segmentType = parseSVGSegmentTypeHelper(lookahead);
+    // Here we assume that this is only called via SVGPathParser::initialCommandIsMoveTo.
+    // TODO(fs): It ought to be possible to refactor away this entirely, and
+    // just handle this in parseSegment (which we sort of do already...) The
+    // only user is the method mentioned above.

[f(malita), 2016/01/27 14:51:13]

Yeah, it would be nice to get rid of this assumption.  You could even move the
check into initialCommandIsMoveTo() - but that requires a public setErrorMark
IIUC.  I'll leave it up to you (for a follow-up).

[fs, 2016/01/27 14:57:56]

I have some sort of (outline of a) plan for this [1] - didn't want to mix any
greater logic changes into this CL though.

[1] Which your recent CL(s) may have simplified a bit =)

+    if (segmentType != PathSegMoveToAbs && segmentType != PathSegMoveToRel)
+        setErrorMark(SVGParseStatus::ExpectedMoveToCommand);
+    return segmentType;
 }
 
 PathSegmentData SVGPathStringSource::parseSegment()
 {
     ASSERT(hasMoreData());
     PathSegmentData segment;
     unsigned lookahead = m_is8BitSource ? *m_current.m_character8 : *m_current.m_character16;
     SVGPathSegType command = parseSVGSegmentTypeHelper(lookahead);
     if (command == PathSegUnknown) {
         // Possibly an implicit command. Not allowed if this is the first command.
-        if (m_previousCommand == PathSegUnknown)
+        if (m_previousCommand == PathSegUnknown) {
+            setErrorMark(SVGParseStatus::ExpectedMoveToCommand);
             return segment;
-        if (!nextCommandHelper(lookahead, m_previousCommand, command))
+        }
+        if (!nextCommandHelper(lookahead, m_previousCommand, command)) {
+            setErrorMark(SVGParseStatus::ExpectedPathCommand);
             return segment;
+        }
     } else {
         // Valid explicit command.
         if (m_is8BitSource)
             m_current.m_character8++;
         else
             m_current.m_character16++;
     }
 
     segment.command = m_previousCommand = command;
 
-    ASSERT(!m_seenError);
+    ASSERT(m_error.status() == SVGParseStatus::NoError);
 
     switch (segment.command) {
     case PathSegCurveToCubicRel:
     case PathSegCurveToCubicAbs:
         segment.point1.setX(parseNumberWithError());
         segment.point1.setY(parseNumberWithError());
         /* fall through */
     case PathSegCurveToCubicSmoothRel:
     case PathSegCurveToCubicSmoothAbs:
         segment.point2.setX(parseNumberWithError());
@@ skipping 33 matching lines @@
         segment.setArcAngle(parseNumberWithError());
         segment.arcLarge = parseArcFlagWithError();
         segment.arcSweep = parseArcFlagWithError();
         segment.targetPoint.setX(parseNumberWithError());
         segment.targetPoint.setY(parseNumberWithError());
         break;
     case PathSegUnknown:
         ASSERT_NOT_REACHED();
     }
 
-    if (UNLIKELY(m_seenError))
+    if (UNLIKELY(m_error.status() != SVGParseStatus::NoError))
         segment.command = PathSegUnknown;
     return segment;
 }
 
 } // namespace blink