Revert "Revert URLPattern wildcard to again match some invalid URLs"

This reverts commit 6fbca674.

Reason for revert: This was a short-term bandaid pending a riskier
but architecturally better fix that has now landed (crrev.com/c/2023047
by Lukasz).

Original change's description:
> Revert URLPattern wildcard to again match some invalid URLs
>
> This is a partial revert of r715320, which changed
> URLPattern::MatchesURL to decline to match all invalid
> (!GURL::is_valid) URLs. This was an inadvertent behavior change;
> it wasn't clear what the intended behavior was in this case.
>
> Reverting this seems to resolve crbug.com/1041880, a bug involving
> some PDF loads from blob: URLs getting stuck as gray boxes. While
> r715320 was originally made to resolve a crash---namely, MatchesURL
> would crash on URLs with empty paths---the follow-up commit r718423
> adds logic that will still suffice to prevent this crash (this
> is tested by ExtensionURLPatternTest.WildcardMatchesPathlessUrl and
> ExtensionURLPatternTest.NonwildcardDoesntMatchPathlessUrl).
>
> Changes:
> - URLPattern: revert the behavior change and update documentation
> to reflect that intended behavior is actually to allow the wildcard
> '<all_urls>' pattern to match invalid URLs
> - URLPattern tests: update to reflect the intended new behavior
>
> Bug: 1041880
>
> R=​devlin
>
> Change-Id: I4f4571c70d0f0dbd7b52cdadbf96a7ea6854cfe0
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2022251
> Reviewed-by: Charlie Reis <creis@chromium.org>
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Commit-Queue: David Van Cleve <davidvc@chromium.org>
> Commit-Queue: Charlie Reis <creis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#735510}

R=rdevlin.cronin@chromium.org
CC=​creis@chromium.org,lukasza@chromium.org

Bug: 1046397
Change-Id: I3fd02ac96e3120eb15e1e8bf43ce7dad8185aa03
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2036792Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: David Van Cleve <davidvc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738357}

extensions/common/url_pattern.cc

@@ -414,6 +414,10 @@ bool URLPattern::SetPort(base::StringPiece port) {
 }
 
 bool URLPattern::MatchesURL(const GURL& test) const {
+  // Invalid URLs can never match.
+  if (!test.is_valid())
+    return false;
+
   const GURL* test_url = &test;
   bool has_inner_url = test.inner_url() != nullptr;
 

extensions/common/url_pattern.h

@@ -27,11 +27,8 @@ class GURL;
 //
 // * Host is not used when the scheme is 'file'.
 // * The path can have embedded '*' characters which act as glob wildcards.
-// * '<all_urls>' is a special pattern that matches any (possibly invalid) URL
-//   that contains a valid scheme (as specified by valid_schemes_).
-//   TODO(crbug.com/1041880): Having the wildcard match invalid URLs is part of
-//   a temporary bugfix that is expected to be reverted as part of the
-//   follow-up, longer-term (but longer-bake-time) fix.
+// * '<all_urls>' is a special pattern that matches any valid URL that contains
+//   a valid scheme (as specified by valid_schemes_).
 // * The '*' scheme pattern excludes file URLs.
 //
 // Examples of valid patterns:
@@ -155,14 +152,8 @@ class URLPattern {
   // false otherwise. Uses valid_schemes_ to determine validity.
   bool IsValidScheme(base::StringPiece scheme) const;
 
-  // Returns true if this instance matches the specified URL. If the pattern
-  // is the wildcard '<all_urls>', this may return true for invalid URLs
-  // (!test.is_valid()) that nonetheless have valid schemes (as determined
-  // by MatchesScheme).
-  //
-  // TODO(crbug.com/1041880): Having the wildcard match invalid URLs is part of
-  // a temporary bugfix that is expected to be reverted as part of the
-  // follow-up, longer-term (but longer-bake-time) fix.
+  // Returns true if this instance matches the specified URL. Always returns
+  // false for invalid URLs.
   bool MatchesURL(const GURL& test) const;
 
   // Returns true if this instance matches the specified security origin.

extensions/common/url_pattern_unittest.cc

@@ -370,22 +370,11 @@ TEST(ExtensionURLPatternTest, Match12) {
       GURL("data:text/html;charset=utf-8,<html>asdf</html>")));
 }
 
-TEST(ExtensionURLPatternTest, MatchInvalid) {
+TEST(ExtensionURLPatternTest, DoesntMatchInvalid) {
   URLPattern pattern(kAllSchemes);
-  // The all_urls pattern should match even an invalid URL.
-  // TODO(crbug.com/1041880): Having the wildcard match invalid URLs is part of
-  // a temporary bugfix that is expected to be reverted as part of the
-  // follow-up, longer-term (but longer-bake-time) fix.
+  // Even the all_urls pattern shouldn't match an invalid URL.
   EXPECT_EQ(URLPattern::ParseResult::kSuccess,
             pattern.Parse(URLPattern::kAllUrlsPattern));
-  EXPECT_TRUE(pattern.MatchesURL(GURL("http:")));
-}
-
-TEST(ExtensionURLPatternTest, DoesntMatchInvalidIfNotWildcard) {
-  URLPattern pattern(kAllSchemes);
-  // A non-all_urls pattern shouldn't match an invalid URL,
-  // even if the scheme matches.
-  EXPECT_EQ(URLPattern::ParseResult::kSuccess, pattern.Parse("*://*/*"));
   EXPECT_FALSE(pattern.MatchesURL(GURL("http:")));
 }