Skip to content

Commit

Permalink
Sync for validator cpp engine and cpp htmlparser (#35952)
Browse files Browse the repository at this point in the history
* Fix a bug where the CSS parser was not correctly accounting for the possibility
of calc() in the media expression.

PiperOrigin-RevId: 394490119

* Remove .proto.h from parse-css.cc imports. The .pb.h version works with bazel.

PiperOrigin-RevId: 394774272
  • Loading branch information
Greg Grothaus committed Sep 3, 2021
1 parent c6baa0e commit e16bbda
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 9 deletions.
48 changes: 39 additions & 9 deletions validator/cpp/htmlparser/css/parse-css.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
#include "logging.h"
#include "strings.h"

using absl::c_find;
using absl::make_unique;
using absl::StrCat;
using absl::WrapUnique;
Expand All @@ -23,7 +22,10 @@ using std::vector;

namespace htmlparser::css {

static constexpr int kMaximumCssRecursion = 100;

namespace internal {

std::string_view StripVendorPrefix(absl::string_view prefixed_string) {
// Checking for '-' is an optimization.
if (!prefixed_string.empty() && prefixed_string[0] == '-') {
Expand Down Expand Up @@ -950,7 +952,7 @@ unique_ptr<ErrorToken> CreateParseErrorTokenAt(
//
TokenStream::TokenStream(vector<unique_ptr<Token>> tokens)
: tokens_(std::move(tokens)), pos_(-1) {
CHECK(tokens_.size() > 0, "empty tokens");
CHECK(!tokens_.empty(), "empty tokens");
CHECK(tokens_.back()->Type() == TokenType::EOF_TOKEN, tokens_.back()->Type());

// Since the last element in |tokens| may get released, we make a
Expand Down Expand Up @@ -1212,8 +1214,6 @@ class Canonicalizer {
declarations->emplace_back(std::move(decl));
}

static constexpr int kMaximumCssRecursion = 100;

// Consumes one or more tokens from |s|, appending them to |tokens|.
// If exceeds depth, returns false;
static bool ConsumeAComponentValue(TokenStream* s,
Expand Down Expand Up @@ -1427,7 +1427,7 @@ class UrlFunctionVisitor : public RuleVisitor {
void LeaveAtRule(const AtRule& at_rule) override { at_rule_scope_.clear(); }

void VisitDeclaration(const Declaration& declaration) override {
CHECK(declaration.value().size() > 0, "");
CHECK(!declaration.value().empty(), "");
CHECK(declaration.value().back()->Type() == TokenType::EOF_TOKEN, "");
for (int ii = 0; ii < declaration.value().size() - 1;) {
const Token& token = *declaration.value()[ii];
Expand Down Expand Up @@ -1560,6 +1560,28 @@ class MediaQueryVisitor : public RuleVisitor {
return false;
}

// token_stream->Current() must be a FUNCTION_TOKEN. Consumes all Tokens up
// to and including the matching closing paren for that FUNCTION_TOKEN.
// If false, recursion exceeded maximum depth.
bool ConsumeAFunction(TokenStream* token_stream, int depth = 0) {
if (depth > kMaximumCssRecursion) return false;
if (token_stream->Current().Type() != TokenType::FUNCTION_TOKEN)
return false;
token_stream->Consume(); // FUNCTION_TOKEN
while (token_stream->Current().Type() != TokenType::EOF_TOKEN) {
TokenType::Code type = token_stream->Current().Type();
if (type == TokenType::FUNCTION_TOKEN) {
if (!ConsumeAFunction(token_stream, depth + 1)) return false;
} else if (type == TokenType::CLOSE_PAREN) {
token_stream->Consume();
return true;
} else {
token_stream->Consume();
}
}
return false; // EOF before function CLOSE_PAREN
}

bool ParseAMediaExpression(TokenStream* token_stream) {
// : '(' S* media_feature S* [ ':' S* expr ]? ')' S*
// ;
Expand All @@ -1582,9 +1604,16 @@ class MediaQueryVisitor : public RuleVisitor {
// "Media features only accept single values: one keyword, one number,
// or a number with a unit identifier. (The only exceptions are the
// ‘aspect-ratio’ and ‘device-aspect-ratio’ media features.)
while (token_stream->Current().Type() != TokenType::EOF_TOKEN &&
token_stream->Current().Type() != TokenType::CLOSE_PAREN)
token_stream->Consume();
while (token_stream->Current().Type() != TokenType::EOF_TOKEN) {
TokenType::Code type = token_stream->Current().Type();
if (type == TokenType::CLOSE_PAREN) {
break;
} else if (type == TokenType::FUNCTION_TOKEN) {
if (!ConsumeAFunction(token_stream)) return false;
} else {
token_stream->Consume();
}
}
}
if (token_stream->Current().Type() != TokenType::CLOSE_PAREN) return false;
token_stream->Consume(); // ')'
Expand Down Expand Up @@ -2383,7 +2412,8 @@ ErrorTokenOr<SimpleSelectorSequence> ParseASimpleSelectorSequence(
start.CopyStartPositionTo(error.get());
return error;
}
// If no type selector is given then the universal selector is implied.
// If no type selector is given then the universal selector is
// implied.
type_selector = make_unique<TypeSelector>(
/*namespace_prefix=*/nullptr, /*element_name=*/"*");
start.CopyStartPositionTo(type_selector.get());
Expand Down
17 changes: 17 additions & 0 deletions validator/cpp/htmlparser/css/parse-css_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2431,6 +2431,23 @@ TEST(ParseCssTest, ParseMediaQueries_SemicolonTerminatedQuery) {
EXPECT_EQ(media_errors.size(), 0);
}

TEST(ParseCssTest, ParseMediaQueries_IncludesFunction) {
// https://github.com/ampproject/amphtml/issues/35793
vector<char32_t> css = htmlparser::Strings::Utf8ToCodepoints(
"@media (min-width: calc(840px - 48px));");
vector<unique_ptr<ErrorToken>> parse_errors;
vector<unique_ptr<Token>> tokens =
Tokenize(&css, /*line=*/1, /*col=*/0, &parse_errors);
unique_ptr<Stylesheet> stylesheet =
ParseAStylesheet(&tokens, AmpCssParsingConfig(), &parse_errors);
EXPECT_EQ(JsonFromList(parse_errors), "[]");

std::vector<unique_ptr<ErrorToken>> media_errors;
std::vector<unique_ptr<Token>> media_types, media_features;
ParseMediaQueries(*stylesheet, &media_types, &media_features, &media_errors);
EXPECT_EQ(media_errors.size(), 0);
}

unique_ptr<Stylesheet> MediaQueryStyleSheet(const std::string& media_query) {
vector<char32_t> css = htmlparser::Strings::Utf8ToCodepoints(
absl::StrCat("@media ", media_query, " {}"));
Expand Down

0 comments on commit e16bbda

Please sign in to comment.