I made things I think can never happen into assertions. I had some questions, though. They're in the first patch.
Created attachment 249048 [details] Patch
Comment on attachment 249048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249048&action=review > Source/WebCore/contentextensions/URLFilterParser.cpp:481 > + ASSERT(m_openGroups.isEmpty()); > + ASSERT(m_subtreeStart != m_subtreeEnd); Let's make them into ASSERT_WITH_MESSAGE to say why they should not happen. > Source/WebCore/contentextensions/URLFilterParser.cpp:527 > - if (!m_floatingTerm.isValid()) > - fail(URLFilterParser::MisplacedQuantifier); > + ASSERT(m_floatingTerm.isValid()); Wouldn't this happen with greedy quantifiers? > Source/WebCore/contentextensions/URLFilterParser.cpp:-727 > - ASSERT(!pattern.isEmpty()); > - Those are no longer excluded by the JSON parser? > Source/WebCore/contentextensions/URLFilterParser.h:44 > enum ParseStatus { This thing is really weird. > Tools/TestWebKitAPI/Tests/WebCore/ContentExtensions.cpp:355 > + testPatternStatus("(?!)", ContentExtensions::URLFilterParser::ParseStatus::Group); > + testPatternStatus("(?=)", ContentExtensions::URLFilterParser::ParseStatus::Group); Please also test that those two produce errors with a non-empty term inside. > Tools/TestWebKitAPI/Tests/WebCore/ContentExtensions.cpp:370 > + // I think these should all be YarrErrors, but they're not. Should we do something about this? What the heck. I have no explanation.
Comment on attachment 249048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249048&action=review >> Source/WebCore/contentextensions/URLFilterParser.cpp:527 >> + ASSERT(m_floatingTerm.isValid()); > > Wouldn't this happen with greedy quantifiers? I'm adding .*a and .*?a, but they do not hit this assert. >> Source/WebCore/contentextensions/URLFilterParser.cpp:-727 >> - > > Those are no longer excluded by the JSON parser? I added a test case that tests an empty string. They probably are excluded by the JSON parser, but the following if statement will catch them.
https://trac.webkit.org/r181762