Use Optional<T> and StringPiece liberally in base::JSONParser.

This should make the parser more robust by joining the can-consume checks
with the actual input byte consumption, preventing the internal parser state
from becoming inconsistent.

Bug: 489301
Change-Id: I33a0f5d9c5daa0b0ad186570393ceaf8fdba8a70
Reviewed-on: https://chromium-review.googlesource.com/1005675
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550026}

base/json/json_parser.h

@@ -31,17 +31,16 @@ class JSONParserTest;
 // to be used directly; it encapsulates logic that need not be exposed publicly.
 //
 // This parser guarantees O(n) time through the input string. Iteration happens
-// on the byte level, with the functions CanConsume and NextChar. The conversion
-// from byte to JSON token happens without advancing the parser in
+// on the byte level, with the functions ConsumeChars() and ConsumeChar(). The
+// conversion from byte to JSON token happens without advancing the parser in
 // GetNextToken/ParseToken, that is tokenization operates on the current parser
 // position without advancing.
 //
 // Built on top of these are a family of Consume functions that iterate
 // internally. Invariant: on entry of a Consume function, the parser is wound
-// to the first byte of a valid JSON token. On exit, it is on the last byte
-// of a token, such that the next iteration of the parser will be at the byte
-// immediately following the token, which would likely be the first byte of the
-// next token.
+// to the first byte of a valid JSON token. On exit, it is on the first byte
+// after the token that was just consumed, which would likely be the first byte
+// of the next token.
 class BASE_EXPORT JSONParser {
  public:
   JSONParser(int options, int max_depth = JSONReader::kStackMaxDepth);
@@ -128,21 +127,23 @@ class BASE_EXPORT JSONParser {
     base::Optional<std::string> string_;
   };
 
-  // Quick check that the stream has capacity to consume |length| more bytes.
-  bool CanConsume(int length);
+  // Returns the next |count| bytes of the input stream, or nullopt if fewer
+  // than |count| bytes remain.
+  Optional<StringPiece> PeekChars(int count);
 
-  // Consumes one byte of the input stream by advancing the parser's position.
-  void NextChar();
+  // Calls PeekChars() with a |count| of 1.
+  Optional<char> PeekChar();
 
-  // Performs the equivalent of NextChar N times.
-  void NextNChars(int n);
+  // Returns the next |count| bytes of the input stream, or nullopt if fewer
+  // than |count| bytes remain, and advances the parser position by |count|.
+  Optional<StringPiece> ConsumeChars(int count);
+
+  // Calls ConsumeChars() with a |count| of 1.
+  Optional<char> ConsumeChar();
 
   // Returns a pointer to the current character position.
   const char* pos();
 
-  // Returns the character at the current position.
-  char current_char();
-
   // Skips over whitespace and comments to find the next token in the stream.
   // This does not advance the parser for non-whitespace or comment chars.
   Token GetNextToken();

base/json/json_parser_unittest.cc

@@ -40,11 +40,10 @@ class JSONParserTest : public testing::Test {
   }
 
   void TestLastThree(JSONParser* parser) {
-    parser->NextChar();
-    EXPECT_EQ(',', parser->current_char());
-    parser->NextChar();
-    EXPECT_EQ('|', parser->current_char());
-    parser->NextChar();
+    EXPECT_EQ(',', *parser->PeekChar());
+    parser->ConsumeChar();
+    EXPECT_EQ('|', *parser->PeekChar());
+    parser->ConsumeChar();
     EXPECT_EQ('\0', *parser->pos());
     EXPECT_EQ(static_cast<size_t>(parser->index_), parser->input_.length());
   }
@@ -56,10 +55,10 @@ TEST_F(JSONParserTest, NextChar) {
 
   EXPECT_EQ('H', *parser->pos());
   for (size_t i = 1; i < input.length(); ++i) {
-    parser->NextChar();
-    EXPECT_EQ(input[i], parser->current_char());
+    parser->ConsumeChar();
+    EXPECT_EQ(input[i], *parser->PeekChar());
   }
-  parser->NextChar();
+  parser->ConsumeChar();
   EXPECT_EQ('\0', *parser->pos());
   EXPECT_EQ(static_cast<size_t>(parser->index_), parser->input_.length());
 }
@@ -68,7 +67,7 @@ TEST_F(JSONParserTest, ConsumeString) {
   std::string input("\"test\",|");
   std::unique_ptr<JSONParser> parser(NewTestParser(input));
   Optional<Value> value(parser->ConsumeString());
-  EXPECT_EQ('"', *parser->pos());
+  EXPECT_EQ(',', *parser->pos());
 
   TestLastThree(parser.get());
 
@@ -82,7 +81,7 @@ TEST_F(JSONParserTest, ConsumeList) {
   std::string input("[true, false],|");
   std::unique_ptr<JSONParser> parser(NewTestParser(input));
   Optional<Value> value(parser->ConsumeList());
-  EXPECT_EQ(']', *parser->pos());
+  EXPECT_EQ(',', *parser->pos());
 
   TestLastThree(parser.get());
 
@@ -96,7 +95,7 @@ TEST_F(JSONParserTest, ConsumeDictionary) {
   std::string input("{\"abc\":\"def\"},|");
   std::unique_ptr<JSONParser> parser(NewTestParser(input));
   Optional<Value> value(parser->ConsumeDictionary());
-  EXPECT_EQ('}', *parser->pos());
+  EXPECT_EQ(',', *parser->pos());
 
   TestLastThree(parser.get());
 
@@ -113,7 +112,7 @@ TEST_F(JSONParserTest, ConsumeLiterals) {
   std::string input("true,|");
   std::unique_ptr<JSONParser> parser(NewTestParser(input));
   Optional<Value> value(parser->ConsumeLiteral());
-  EXPECT_EQ('e', *parser->pos());
+  EXPECT_EQ(',', *parser->pos());
 
   TestLastThree(parser.get());
 
@@ -126,7 +125,7 @@ TEST_F(JSONParserTest, ConsumeLiterals) {
   input = "false,|";
   parser.reset(NewTestParser(input));
   value = parser->ConsumeLiteral();
-  EXPECT_EQ('e', *parser->pos());
+  EXPECT_EQ(',', *parser->pos());
 
   TestLastThree(parser.get());
 
@@ -138,7 +137,7 @@ TEST_F(JSONParserTest, ConsumeLiterals) {
   input = "null,|";
   parser.reset(NewTestParser(input));
   value = parser->ConsumeLiteral();
-  EXPECT_EQ('l', *parser->pos());
+  EXPECT_EQ(',', *parser->pos());
 
   TestLastThree(parser.get());
 
@@ -151,7 +150,7 @@ TEST_F(JSONParserTest, ConsumeNumbers) {
   std::string input("1234,|");
   std::unique_ptr<JSONParser> parser(NewTestParser(input));
   Optional<Value> value(parser->ConsumeNumber());
-  EXPECT_EQ('4', *parser->pos());
+  EXPECT_EQ(',', *parser->pos());
 
   TestLastThree(parser.get());
 
@@ -164,7 +163,7 @@ TEST_F(JSONParserTest, ConsumeNumbers) {
   input = "-1234,|";
   parser.reset(NewTestParser(input));
   value = parser->ConsumeNumber();
-  EXPECT_EQ('4', *parser->pos());
+  EXPECT_EQ(',', *parser->pos());
 
   TestLastThree(parser.get());
 
@@ -176,7 +175,7 @@ TEST_F(JSONParserTest, ConsumeNumbers) {
   input = "12.34,|";
   parser.reset(NewTestParser(input));
   value = parser->ConsumeNumber();
-  EXPECT_EQ('4', *parser->pos());
+  EXPECT_EQ(',', *parser->pos());
 
   TestLastThree(parser.get());
 
@@ -189,7 +188,7 @@ TEST_F(JSONParserTest, ConsumeNumbers) {
   input = "42e3,|";
   parser.reset(NewTestParser(input));
   value = parser->ConsumeNumber();
-  EXPECT_EQ('3', *parser->pos());
+  EXPECT_EQ(',', *parser->pos());
 
   TestLastThree(parser.get());
 
@@ -201,7 +200,7 @@ TEST_F(JSONParserTest, ConsumeNumbers) {
   input = "314159e-5,|";
   parser.reset(NewTestParser(input));
   value = parser->ConsumeNumber();
-  EXPECT_EQ('5', *parser->pos());
+  EXPECT_EQ(',', *parser->pos());
 
   TestLastThree(parser.get());
 
@@ -213,7 +212,7 @@ TEST_F(JSONParserTest, ConsumeNumbers) {
   input = "0.42e+3,|";
   parser.reset(NewTestParser(input));
   value = parser->ConsumeNumber();
-  EXPECT_EQ('3', *parser->pos());
+  EXPECT_EQ(',', *parser->pos());
 
   TestLastThree(parser.get());
 
@@ -324,7 +323,7 @@ TEST_F(JSONParserTest, ErrorMessages) {
 
   root = JSONReader::ReadAndReturnError(("[\"\\ufffe\"]"), JSON_PARSE_RFC,
                                         &error_code, &error_message);
-  EXPECT_EQ(JSONParser::FormatErrorMessage(1, 7, JSONReader::kInvalidEscape),
+  EXPECT_EQ(JSONParser::FormatErrorMessage(1, 8, JSONReader::kInvalidEscape),
             error_message);
   EXPECT_EQ(JSONReader::JSON_INVALID_ESCAPE, error_code);
 }

base/json/json_reader_unittest.cc

@@ -516,6 +516,7 @@ TEST(JSONReaderTest, InvalidUTF16Escapes) {
 
 TEST(JSONReaderTest, LiteralRoots) {
   std::unique_ptr<Value> root = JSONReader::Read("null");
+  ASSERT_TRUE(root);
   EXPECT_TRUE(root->is_none());
 
   root = JSONReader::Read("true");