Bug 153090 – support wildcard plugin policy matching

• Component: Plug-ins
• Status: RESOLVED FIXED

<rdar://problem/24176917> Provide Webkit support for wildcard plugin policy matching

We need to allow matching appleweb.apple.com with *.apple.com when deciding plugin load client policy for the host.

Created attachment 268988 [details]
Patch for the fix.

[Tim Horton]

Comment on attachment 268988 [details]
Patch for the fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=268988&action=review

> Source/WebKit2/Platform/mac/StringUtilities.h:41
> +int getLengthOfSubtringMatchesWildcardString(const String&, const String&);

We use the get- prefix to indicate functions with out arguments, which this is not. Please rename this.

(In reply to comment #2)
> Comment on attachment 268988 [details]
> Patch for the fix.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268988&action=review
> 
> > Source/WebKit2/Platform/mac/StringUtilities.h:41
> > +int getLengthOfSubtringMatchesWildcardString(const String&, const String&);
> 
> We use the get- prefix to indicate functions with out arguments, which this
> is not. Please rename this.

Okay. I'll remove the get prefix for this and rename the following methods as well.

void getWildcardHosts(Vector<String>& wildcardHosts) const;
void getBestMatchedWildcardHostForHost(const String& host, String& wildcardHost) const;

[Tim Horton]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 268988 [details]
> > Patch for the fix.
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=268988&action=review
> > 
> > > Source/WebKit2/Platform/mac/StringUtilities.h:41
> > > +int getLengthOfSubtringMatchesWildcardString(const String&, const String&);
> > 
> > We use the get- prefix to indicate functions with out arguments, which this
> > is not. Please rename this.
> 
> Okay. I'll remove the get prefix for this and rename the following methods
> as well.
> 
> void getWildcardHosts(Vector<String>& wildcardHosts) const;
> void getBestMatchedWildcardHostForHost(const String& host, String&
> wildcardHost) const;

Nope, those two are using out arguments :D

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Comment on attachment 268988 [details]
> > > Patch for the fix.
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=268988&action=review
> > > 
> > > > Source/WebKit2/Platform/mac/StringUtilities.h:41
> > > > +int getLengthOfSubtringMatchesWildcardString(const String&, const String&);
> > > 
> > > We use the get- prefix to indicate functions with out arguments, which this
> > > is not. Please rename this.
> > 
> > Okay. I'll remove the get prefix for this and rename the following methods
> > as well.
> > 
> > void getWildcardHosts(Vector<String>& wildcardHosts) const;
> > void getBestMatchedWildcardHostForHost(const String& host, String&
> > wildcardHost) const;
> 
> Nope, those two are using out arguments :D

Ah now I see what you meant. I'll leave them as they are then.

[Anders Carlsson]

Comment on attachment 268988 [details]
Patch for the fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=268988&action=review

> Source/WebKit2/Platform/mac/StringUtilities.mm:77
> +int getLengthOfSubtringMatchesWildcardString(const String& string, const String& wildcardString)

I think this needs to explain what it does.
Also, I think if you can't find the wildcard then it'll end up creating and destroying strings unnecessarily.
Also, if it's only called from WebPlatformStrategies you can just put it there.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:249
> +void WebPlatformStrategies::getWildcardHosts(Vector<String>& wildcardHosts) const

I think this should return a vector instead of taking it as an out parameter.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:257
> +void WebPlatformStrategies::getBestMatchedWildcardHostForHost(const String& host, String& wildcardHost) const

This should return a String.

Comment on attachment 268988 [details]
Patch for the fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=268988&action=review

Thanks for reviewing! I'll send out another patch.

>> Source/WebKit2/Platform/mac/StringUtilities.mm:77
>> +int getLengthOfSubtringMatchesWildcardString(const String& string, const String& wildcardString)
> 
> I think this needs to explain what it does.
> Also, I think if you can't find the wildcard then it'll end up creating and destroying strings unnecessarily.
> Also, if it's only called from WebPlatformStrategies you can just put it there.

I'll rename this to lengthOfMatchedSubstringBetweenStringAndWildcardString and add a comment?
Will return early if there's no wildcard. 
And I'll move this to WebPlatformStrategies.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:249
>> +void WebPlatformStrategies::getWildcardHosts(Vector<String>& wildcardHosts) const
> 
> I think this should return a vector instead of taking it as an out parameter.

Now I look at this again, I think I'll remove this method since it's used only once.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:257
>> +void WebPlatformStrategies::getBestMatchedWildcardHostForHost(const String& host, String& wildcardHost) const
> 
> This should return a String.

Will fix that. 
Do we prefer returning the value to using an out parameter if possible?

(In reply to comment #7)
> Comment on attachment 268988 [details]
> Patch for the fix.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268988&action=review
> 
> Thanks for reviewing! I'll send out another patch.
> 
> >> Source/WebKit2/Platform/mac/StringUtilities.mm:77
> >> +int getLengthOfSubtringMatchesWildcardString(const String& string, const String& wildcardString)
> > 
> > I think this needs to explain what it does.
> > Also, I think if you can't find the wildcard then it'll end up creating and destroying strings unnecessarily.
> > Also, if it's only called from WebPlatformStrategies you can just put it there.
> 
> I'll rename this to lengthOfMatchedSubstringBetweenStringAndWildcardString
> and add a comment?
I don't like this lengthy name. I'll leave it as it is...

> Will return early if there's no wildcard. 
> And I'll move this to WebPlatformStrategies.
> 
> >> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:249
> >> +void WebPlatformStrategies::getWildcardHosts(Vector<String>& wildcardHosts) const
> > 
> > I think this should return a vector instead of taking it as an out parameter.
> 
> Now I look at this again, I think I'll remove this method since it's used
> only once.
> 
> >> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:257
> >> +void WebPlatformStrategies::getBestMatchedWildcardHostForHost(const String& host, String& wildcardHost) const
> > 
> > This should return a String.
> 
> Will fix that. 
> Do we prefer returning the value to using an out parameter if possible?

Created attachment 269126 [details]
Revised patch. Take 2.

[WebKit Commit Bot]

Attachment 269126 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:260:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 269126 [details]
Revised patch. Take 2.

>Index: Source/WebKit2/ChangeLog
>===================================================================
>--- Source/WebKit2/ChangeLog	(revision 195158)
>+++ Source/WebKit2/ChangeLog	(working copy)
>@@ -1,3 +1,30 @@
>[phone]  Tina Liu  <[e-mail]>
>+
>+        Implement wildcard matching for plug-in policy host.
>+        https://bugs.webkit.org/show_bug.cgi?id=153090
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        WebPlatformStrategies decides the plug-in load policy for a host by looking for a matched hostname
>+        in the list of plug-in policies sent by Safari. This patch adds support for wildcard matching --
>+        if there's a policy with hostname "*.apple.com," the policy for "appleweb.apple.com" would be replaced
>+        with that of "*.apple.com" if there's no policy for this hostname. If there is more than one wildcard
>+        hostname, the one with the longest matched substring will be used.
>+
>+        * WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:
>+        (WebKit::WebPlatformStrategies::lengthOfSubstringMatchingWildcardString):
>+        Add a helper method that returns the total length of the matched substrings between the wildcard expression
>+        and the string. The more specific the wildcard expression is, the longer the matched substring's length is.
>+        (WebKit::WebPlatformStrategies::bestMatchedWildcardHostForHost):
>+        Return the wildcard hostname whose matched substring with the host is the longest.
>+        (WebKit::WebPlatformStrategies::replaceHostWithMatchedWildcardHost):
>+        Replace the look-up host with the best matched wildcard host if there is a match and that the matched
>+        wildcard host's plug-in identifier is the same as that of the host.
>+        (WebKit::WebPlatformStrategies::pluginLoadClientPolicyForHost):
>+        Try to replace the look-up host with the wildcard host if there's no policy for the host.
>+
>+        * WebProcess/WebCoreSupport/WebPlatformStrategies.h:
>+
> 2016-01-15  Michael Catanzaro  <[e-mail]>
> 
>         Fix internal Windows build
>Index: Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp
>===================================================================
>--- Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp	(revision 194712)
>+++ Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp	(working copy)
>@@ -67,6 +67,12 @@
> #include <WebCore/URL.h>
> #include <wtf/Atomics.h>
> 
>+#if ENABLE(NETSCAPE_PLUGIN_API)
>+#if PLATFORM(MAC)
>+#include "StringUtilities.h"
>+#endif
>+#endif
>+
Ah.. forgot to remove this. Will do.

> using namespace WebCore;
> 
> namespace WebKit {
>@@ -239,16 +245,80 @@ void WebPlatformStrategies::clearPluginC
> 
> #if ENABLE(NETSCAPE_PLUGIN_API)
> #if PLATFORM(MAC)
>+int WebPlatformStrategies::lengthOfSubstringMatchingWildcardString(const String& string, const String& wildcardString) const
>+{
>+    // Returns the length of common substrings of the string and the wildcard expression.
>+    // The more specific the wildcard expression is, the longer the matched substring's length is.
>+
>+    if (!wildcardString.contains("*"))
>+        return 0;
>+
>+    int wildcardPosition = wildcardString.find("*");
>+
>+    String preWildcardSubstring = wildcardString.substring(0, wildcardPosition);
>+    String postWildcardSubstring = wildcardString.substring(wildcardPosition+1);
>+    if (string.contains(preWildcardSubstring) && string.contains(postWildcardSubstring))
>+        return preWildcardSubstring.length() + postWildcardSubstring.length();
>+    else
>+        return 0;
>+}
>+
>+String WebPlatformStrategies::bestMatchedWildcardHostForHost(const String& host) const
>+{
>+    Vector<String> wildcardHosts;
>+    for (String key : m_hostsToPluginIdentifierData.keys()) {
>+        if (key.contains("*") && key != "*")
>+            wildcardHosts.append(key);
>+    }
>+
>+    if (wildcardHosts.isEmpty())
>+        return String();
>+
>+    String longestMatchedWildcardHost;
>+    int maxLengthMatchedSubstring = 0;
>+    for (size_t i = 0; i < wildcardHosts.size(); ++i) {
>+        String wildcardHost = wildcardHosts[i];
>+        int matchedLength = lengthOfSubstringMatchingWildcardString(host, wildcardHost);
>+        if (maxLengthMatchedSubstring < matchedLength) {
>+            maxLengthMatchedSubstring = matchedLength;
>+            longestMatchedWildcardHost = wildcardHost;
>+        }
>+    }
>+    return longestMatchedWildcardHost;
>+}
>+
>+bool WebPlatformStrategies::replaceHostWithMatchedWildcardHost(String& host, const String& identifier) const
>+{
>+    String matchedWildcardHost = bestMatchedWildcardHostForHost(host);
>+
>+    if (matchedWildcardHost.isNull())
>+        return false;
>+    if (!m_hostsToPluginIdentifierData.contains(matchedWildcardHost))
>+        return false;
>+
>+    PluginPolicyMapsByIdentifier policiesByIdentifier = m_hostsToPluginIdentifierData.get(matchedWildcardHost);
>+    if (!policiesByIdentifier.contains(identifier))
>+        return false;
>+
>+    host = matchedWildcardHost;
>+    return true;
>+}
>+
> bool WebPlatformStrategies::pluginLoadClientPolicyForHost(const String& host, const PluginInfo& info, PluginLoadClientPolicy& policy) const
> {
>     String hostToLookUp = host;
>+    String identifier = info.bundleIdentifier;
>+
>+    PluginPolicyMapsByIdentifier policiesByIdentifier;
>+    if (!m_hostsToPluginIdentifierData.contains(hostToLookUp) && !identifier.isNull())
>+        replaceHostWithMatchedWildcardHost(hostToLookUp, identifier);
>+
>     if (!m_hostsToPluginIdentifierData.contains(hostToLookUp))
>         hostToLookUp = "*";
>     if (!m_hostsToPluginIdentifierData.contains(hostToLookUp))
>         return false;
> 
>-    PluginPolicyMapsByIdentifier policiesByIdentifier = m_hostsToPluginIdentifierData.get(hostToLookUp);
>-    String identifier = info.bundleIdentifier;
>+    policiesByIdentifier = m_hostsToPluginIdentifierData.get(hostToLookUp);
>     if (!identifier || !policiesByIdentifier.contains(identifier))
>         identifier = "*";
>     if (!policiesByIdentifier.contains(identifier))
>Index: Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h
>===================================================================
>--- Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h	(revision 194712)
>+++ Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h	(working copy)
>@@ -112,6 +112,9 @@ private:
> #if PLATFORM(MAC)
>     HashMap<String, PluginPolicyMapsByIdentifier> m_hostsToPluginIdentifierData;
>     bool pluginLoadClientPolicyForHost(const String&, const WebCore::PluginInfo&, WebCore::PluginLoadClientPolicy&) const;
>+    String bestMatchedWildcardHostForHost(const String& host) const;
>+    bool replaceHostWithMatchedWildcardHost(String& host, const String& identifier) const;
>+    int lengthOfSubstringMatchingWildcardString(const String&, const String&) const;
> #endif // PLATFORM(MAC)
> #endif // ENABLE(NETSCAPE_PLUGIN_API)
> };

Created attachment 269127 [details]
Fix style issue.

[Darin Adler]

Comment on attachment 269127 [details]
Fix style issue.

View in context: https://bugs.webkit.org/attachment.cgi?id=269127&action=review

A new feature like this needs test cases; can’t just land the code without a test. I see multiple problems in the code, but those problems would become clear if we found a way to test the code.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:244
> +    // Returns the length of common substrings of the string and the wildcard expression.

That comment is not at all what this current function does. The length this returns depends only on the wildcard string, not on the other string at all, except that the other string can make it return 0. It’s not going to return different lengths for different passed strings.

We need unit tests for this function to help us understand what this function actually does and does not do. To even document what it’s supposed to do!

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:250
> +    if (!wildcardString.contains("*"))
> +        return 0;
> +
> +    int wildcardPosition = wildcardString.find("*");

We should not do both contains and find; that results in searching twice. Instead, call find and compare the result of the find function with notFound.

Return value should be auto instead of int. It's easy to get the type wrong and then notFound will not work correctly.

    auto wildcardPosition = wildcardString.find('*');
    if (wildcardPosition == notFound)
        return 0;

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:253
> +    String postWildcardSubstring = wildcardString.substring(wildcardPosition+1);

WebKit coding style calls for spaces around "+".

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:254
> +    if (string.contains(preWildcardSubstring) && string.contains(postWildcardSubstring))

This code is mysterious. If we have a wildcardString that says "a*b" this will return "2" when passed the string "ba"; but as far as I can tell, "ba" doesn't match "a*b" at all. Is that really what we want? I am quite puzzled about what this function is intended to do. Without test cases we are probably not going to get it right.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:263
> +    for (String key : m_hostsToPluginIdentifierData.keys()) {

This churns the reference counts on the keys. The best type to use would be auto&, or if you don't like auto, String& or const String&.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:264
> +        if (key.contains("*") && key != "*")

Faster to use contains('*') than contains("*").

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:274
> +    for (size_t i = 0; i < wildcardHosts.size(); ++i) {
> +        String wildcardHost = wildcardHosts[i];

This should be written like this:

    for (auto& wildcardHost : wildcardHosts) {

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:281
> +    return longestMatchedWildcardHost;

The algorithm here is not good for the case where two hosts both have the same match length. In that case it’s hash table order in the m_hostsToPluginIdentifierData map that determines which wins; that’s essentially random and presumably not what we want.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:295
> +    if (!m_hostsToPluginIdentifierData.contains(matchedWildcardHost))
> +        return false;
> +
> +    PluginPolicyMapsByIdentifier policiesByIdentifier = m_hostsToPluginIdentifierData.get(matchedWildcardHost);
> +    if (!policiesByIdentifier.contains(identifier))
> +        return false;

This is inefficient. It does two hash lookups on the same map (one for contains and one for get), and it makes a local copy of the policiesByIdentifier map before calling contains on it. Instead it should be written like this:

    auto identifierData = m_hostsToPluginIdentifierData.find(matchedWildcardHost);
    if (identifierData ==  m_hostsToPluginIdentifierData.end() || identifierData->value.contains(identifier))
        return false;

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:117
> +    int lengthOfSubstringMatchingWildcardString(const String&, const String&) const;

This should be a static member function or a non-member function, not a non-static member function.

Comment on attachment 269127 [details]
Fix style issue.

View in context: https://bugs.webkit.org/attachment.cgi?id=269127&action=review

Thanks for all the feedback!

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:244
>> +    // Returns the length of common substrings of the string and the wildcard expression.
> 
> That comment is not at all what this current function does. The length this returns depends only on the wildcard string, not on the other string at all, except that the other string can make it return 0. It’s not going to return different lengths for different passed strings.
> 
> We need unit tests for this function to help us understand what this function actually does and does not do. To even document what it’s supposed to do!

I'm trying to return the length of the matched substring of the wildcard string. For example, passing (aabbb, a*b) returns 2, and passing (aabbb, aa*bb) returns 4, while passing (aaaabbbb, aa*bb) also returns 4. So the length of the return value really depends only on the wildcard string.

Where do we usually put our unit tests?

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:250
>> +    int wildcardPosition = wildcardString.find("*");
> 
> We should not do both contains and find; that results in searching twice. Instead, call find and compare the result of the find function with notFound.
> 
> Return value should be auto instead of int. It's easy to get the type wrong and then notFound will not work correctly.
> 
>     auto wildcardPosition = wildcardString.find('*');
>     if (wildcardPosition == notFound)
>         return 0;

Will fix this.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:253
>> +    String postWildcardSubstring = wildcardString.substring(wildcardPosition+1);
> 
> WebKit coding style calls for spaces around "+".

Will fix this.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:254
>> +    if (string.contains(preWildcardSubstring) && string.contains(postWildcardSubstring))
> 
> This code is mysterious. If we have a wildcardString that says "a*b" this will return "2" when passed the string "ba"; but as far as I can tell, "ba" doesn't match "a*b" at all. Is that really what we want? I am quite puzzled about what this function is intended to do. Without test cases we are probably not going to get it right.

I totally missed this case and this function indeed is not correct. I'll come up with another way to do this.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:263
>> +    for (String key : m_hostsToPluginIdentifierData.keys()) {
> 
> This churns the reference counts on the keys. The best type to use would be auto&, or if you don't like auto, String& or const String&.

Will fix this.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:264
>> +        if (key.contains("*") && key != "*")
> 
> Faster to use contains('*') than contains("*").

Fixed.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:274
>> +        String wildcardHost = wildcardHosts[i];
> 
> This should be written like this:
> 
>     for (auto& wildcardHost : wildcardHosts) {

Fixed.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:281
>> +    return longestMatchedWildcardHost;
> 
> The algorithm here is not good for the case where two hosts both have the same match length. In that case it’s hash table order in the m_hostsToPluginIdentifierData map that determines which wins; that’s essentially random and presumably not what we want.

I'll return the first match in alphabetical order?

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:295
>> +        return false;
> 
> This is inefficient. It does two hash lookups on the same map (one for contains and one for get), and it makes a local copy of the policiesByIdentifier map before calling contains on it. Instead it should be written like this:
> 
>     auto identifierData = m_hostsToPluginIdentifierData.find(matchedWildcardHost);
>     if (identifierData ==  m_hostsToPluginIdentifierData.end() || identifierData->value.contains(identifier))
>         return false;

Will fix this.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:117
>> +    int lengthOfSubstringMatchingWildcardString(const String&, const String&) const;
> 
> This should be a static member function or a non-member function, not a non-static member function.

I'll make this a non-member function.

Created attachment 269401 [details]
Revised patch.

[WebKit Commit Bot]

Attachment 269401 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2/mac/StringUtilities.mm:64:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
ERROR: Source/WebKit2/Platform/mac/StringUtilities.mm:44:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebKit2/Platform/mac/StringUtilities.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/Platform/mac/StringUtilities.h:38:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/Platform/mac/StringUtilities.h:39:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/Platform/mac/StringUtilities.h:40:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:70:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 7 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 269406 [details]
Revised patch.

[Darin Adler]

Comment on attachment 269401 [details]
Revised patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=269401&action=review

> Source/WebKit2/Platform/mac/StringUtilities.mm:45
> +    if (string.length == 0)
> +        return string;

Not sure why we need this special case, unless we want to handle null. The code below should work just fine with the empty string, returning "^$", which seems fine.

> Source/WebKit2/Platform/mac/StringUtilities.mm:52
> +    [regexPatternString replaceOccurrencesOfString:@"*" withString:@".*" options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];
> +    [regexPatternString replaceOccurrencesOfString:@"/" withString:@"\\/" options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];
> +    [regexPatternString insertString:@"^" atIndex:0];
> +    [regexPatternString appendString:@"$"];

This won’t work if the wildcard strings have any regular expression metacharacters in them. For example, if there is a period or question mark or a backslash somewhere in the string. Is there some reason we don’t have to worry about those? Periods, in particular, are going to be common and they will match any character rather than a single character.

I don’t understand why we are replacing "/" with "\/". A forward slash is not a regular expression metacharacter and so this is not helpful, although it’s also not harmful. You can remove that line of code without changing anything.

Converting a wildcard expression into a regular expression is trickier than this. This webpage <http://www.codeproject.com/Articles/11556/Converting-Wildcards-to-Regexes> talks about how to do it in C#, and the key step we are missing is to “escape” all the characters in the wildcard other than the "*" character (and "?" if you want to handle that one). To do that, we need to add a "\" before all regular expression metacharacters. Not super-hard to write, but requires a set of all the regular expression metacharacters and a loop to put a "\" before each one.

> Source/WebKit2/Platform/mac/StringUtilities.mm:65
> +    if ([wildcardRegex firstMatchInString:string options:0 range:NSMakeRange(0, string.length)] != nil)
> +        return true;
> +
> +    return false;

This:

    if (x) return true; return false;

Is the same as this:

    return x;

So there is no need for an if statement here.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:76
> +    return (string1.length() < string2.length());

This will result in an essentially random selection for strings that happen to be the same length. The comparator needs to compare the strings themselves if both are the same length.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:259
> +    if (wildcardHosts.isEmpty())
> +        return String();

No need for this early exit; the code below works fine without this additional check.

> Tools/TestWebKitAPI/Tests/WebKit2/mac/StringUtilities.mm:62
> +}

Here are a few of the many missing test cases covering various metacharacters that the current function will handle wrong:

    EXPECT_FALSE(WebKit::stringMatchesWildcardString("ab", "a+b"));
    EXPECT_TRUE(WebKit::stringMatchesWildcardString("a+b", "a+b"));

    EXPECT_FALSE(WebKit::stringMatchesWildcardString("abc", "a.c"));

    EXPECT_FALSE(WebKit::stringMatchesWildcardString("a", "a|c"));

    EXPECT_FALSE(WebKit::stringMatchesWildcardString("a", "a{1}"));

It’s hard to correctly convert a wildcard string to a regular expression.

[Darin Adler]

Comment on attachment 269406 [details]
Revised patch.

All my comments on the older patch apply to this one too.

Comment on attachment 269401 [details]
Revised patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=269401&action=review

Thanks for the feedback! The next revised patch is on its way.

>> Source/WebKit2/Platform/mac/StringUtilities.mm:45
>> +        return string;
> 
> Not sure why we need this special case, unless we want to handle null. The code below should work just fine with the empty string, returning "^$", which seems fine.

Removed.

>> Source/WebKit2/Platform/mac/StringUtilities.mm:52
>> +    [regexPatternString appendString:@"$"];
> 
> This won’t work if the wildcard strings have any regular expression metacharacters in them. For example, if there is a period or question mark or a backslash somewhere in the string. Is there some reason we don’t have to worry about those? Periods, in particular, are going to be common and they will match any character rather than a single character.
> 
> I don’t understand why we are replacing "/" with "\/". A forward slash is not a regular expression metacharacter and so this is not helpful, although it’s also not harmful. You can remove that line of code without changing anything.
> 
> Converting a wildcard expression into a regular expression is trickier than this. This webpage <http://www.codeproject.com/Articles/11556/Converting-Wildcards-to-Regexes> talks about how to do it in C#, and the key step we are missing is to “escape” all the characters in the wildcard other than the "*" character (and "?" if you want to handle that one). To do that, we need to add a "\" before all regular expression metacharacters. Not super-hard to write, but requires a set of all the regular expression metacharacters and a loop to put a "\" before each one.

The only reason that I didn't escape other metacharacters is because I forgot to... I'll fix this.
Replacing / with \/ is also unintentional. I was thinking of matching URLs and that's why I specifically (and mistakenly) included it.

I'll make sure that I escape all metacharacters.

>> Source/WebKit2/Platform/mac/StringUtilities.mm:65
>> +    return false;
> 
> This:
> 
>     if (x) return true; return false;
> 
> Is the same as this:
> 
>     return x;
> 
> So there is no need for an if statement here.

Will fix this!

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:76
>> +    return (string1.length() < string2.length());
> 
> This will result in an essentially random selection for strings that happen to be the same length. The comparator needs to compare the strings themselves if both are the same length.

I'll fix this by returning the one with higher alphabetical order if the lengths are the same.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:259
>> +        return String();
> 
> No need for this early exit; the code below works fine without this additional check.

Removed.

>> Tools/TestWebKitAPI/Tests/WebKit2/mac/StringUtilities.mm:62
>> +}
> 
> Here are a few of the many missing test cases covering various metacharacters that the current function will handle wrong:
> 
>     EXPECT_FALSE(WebKit::stringMatchesWildcardString("ab", "a+b"));
>     EXPECT_TRUE(WebKit::stringMatchesWildcardString("a+b", "a+b"));
> 
>     EXPECT_FALSE(WebKit::stringMatchesWildcardString("abc", "a.c"));
> 
>     EXPECT_FALSE(WebKit::stringMatchesWildcardString("a", "a|c"));
> 
>     EXPECT_FALSE(WebKit::stringMatchesWildcardString("a", "a{1}"));
> 
> It’s hard to correctly convert a wildcard string to a regular expression.

These should pass once I escape all the metacharacters! I'll include these in the test.

Created attachment 269469 [details]
Revised patch.

[WebKit Commit Bot]

Attachment 269469 [details] did not pass style-queue:


ERROR: Source/WebKit2/Platform/mac/StringUtilities.mm:32:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 269471 [details]
Revised patch.

[Darin Adler]

Comment on attachment 269471 [details]
Revised patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=269471&action=review

> Source/WebKit2/Platform/mac/StringUtilities.h:46
> +WK_EXPORT extern bool stringMatchesWildcardString(const String& stringToBeMatched, const String& wildcardString);
>  }

Normal WebKit formatting should have a space here.

> Source/WebKit2/Platform/mac/StringUtilities.mm:57
> +    [regexPatternString replaceOccurrencesOfString:@"." withString:@"\\." options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];
> +    [regexPatternString replaceOccurrencesOfString:@"|" withString:@"\\|" options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];
> +    [regexPatternString replaceOccurrencesOfString:@"+" withString:@"\\+" options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];
> +    [regexPatternString replaceOccurrencesOfString:@"?" withString:@"\\?" options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];
> +    [regexPatternString replaceOccurrencesOfString:@"(" withString:@"\\(" options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];
> +    [regexPatternString replaceOccurrencesOfString:@")" withString:@"\\)" options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];
> +    [regexPatternString replaceOccurrencesOfString:@"[" withString:@"\\[" options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];
> +    [regexPatternString replaceOccurrencesOfString:@"]" withString:@"\\]" options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];
> +    [regexPatternString replaceOccurrencesOfString:@"{" withString:@"\\{" options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];
> +    [regexPatternString replaceOccurrencesOfString:@"}" withString:@"\\}" options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];
> +    [regexPatternString replaceOccurrencesOfString:@"^" withString:@"\\^" options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];
> +    [regexPatternString replaceOccurrencesOfString:@"$" withString:@"\\$" options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];

I suppose it’s OK to do it written out like this, although it would be more elegant to have it use a loop to iterate through each character rather than unrolled code.

How did we check that we covered all the regular expression metacharacters supported by NSRegularExpression? Where did this list of characters come from?

> Source/WebKit2/Platform/mac/StringUtilities.mm:61
> +    return regexPatternString;

What happened to putting the "^" at the start and "$" at the end? I would have expected some tests to fail because we did not do this.

> Source/WebKit2/Platform/mac/StringUtilities.mm:67
> +    NSRegularExpression *wildcardRegex = [NSRegularExpression regularExpressionWithPattern:wildcardRegexPatternString options:NSRegularExpressionCaseInsensitive error:nullptr];

It occurs to me what we could use WebCore’s RegularExpression instead of NSRegularExpression. Then we would not have to convert the strings to NSString. But I suppose that might not be OK from the threading point of view?

> Source/WebKit2/Platform/mac/StringUtilities.mm:69
> +    return([wildcardRegex firstMatchInString:string options:0 range:NSMakeRange(0, string.length)] != nil);

Should remove parentheses here to match normal WebKit coding style and should have a space after the word "return" again.

> Source/WebKit2/Platform/mac/StringUtilities.mm:112
> +NSString *formattedPhoneNumberString(NSString*)

Tiny formatting mistake here, should be "NSString *".

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:38
> +#include "StringUtilities.h"

This include needs to be inside a appropriate #if since this is a platform-specific header. That’s why the GTK and EFL builds are broken.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:73
> +static bool comparatorByStringAscendingLength(const String& string1, const String& string2)

This should be inside the WebKit namespace, and inside the #if below. The iOS build is failing because it’s not inside the #if.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:79
> +        return string1[0] < string2[0];

This compares only the first character. We want to compare the whole string because we want a well defined ordering even if the first character is the same. Should be easy to fix by writing this:

    return string1 < string2;

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:81
> +    return (string1Length < string2Length);

No need for the parentheses here.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:273
> +    std::sort(matchedWildcardHosts.begin(), matchedWildcardHosts.end(), comparatorByStringAscendingLength);
> +    return matchedWildcardHosts.last();

This is a more efficient way to do the same thing:

    return *std::max_element(matchedWildcardHosts.begin(), matchedWildcardHosts.end(), comparatorByStringAscendingLength);

No need to sort the entire vector just to select the first item in it.

In fact, on reflection, this function does not need to create vectors of strings at all. It could be written as a single loop instead of three separate loops (one inside std::sort or std::max_element), and there is no need to store the intermediate values; just need to store the longest wildcard host seen so far in a local variable. Please rewrite this so it doesn’t use the vectors!

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:298
> +    if (!m_hostsToPluginIdentifierData.contains(hostToLookUp) && !identifier.isNull())
> +        replaceHostWithMatchedWildcardHost(hostToLookUp, identifier);

Slightly better to check identifier.isNull() first, since it’s a much cheaper check than the hash table lookup that contains has to do.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:305
> +    policiesByIdentifier = m_hostsToPluginIdentifierData.get(hostToLookUp);

It’s really inefficient that if the host is in the m_hostsToPluginIdentifierData collection, it will look it up four times, calling contains three times then get on the same string. We should restructure the function so that it will do a minimal number of hash table lookups. Just a single find in the common case, and a second one only if there is a wildcard match, and then a lookup of "*" if that fails. So a maximum of tree and only one in the normal case.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:308
>      if (!identifier || !policiesByIdentifier.contains(identifier))
>          identifier = "*";
>      if (!policiesByIdentifier.contains(identifier))

Same problem here where we call policiesByIdentifier.contains twice because of the way the function is structured.

Also, both above and below, we end up copying a map out of the other map because we use get, and copying a HashMap is quite inefficient. We should use find instead so we can use the map in place instead of copying it.

[Darin Adler]

Comment on attachment 269471 [details]
Revised patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=269471&action=review

>> Source/WebKit2/Platform/mac/StringUtilities.h:46
>>  }
> 
> Normal WebKit formatting should have a space here.

Sorry, that was unclear: I meant a blank line between the declaration and the brace ending the namespace.

Comment on attachment 269471 [details]
Revised patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=269471&action=review

Thanks for the feedback!

>> Source/WebKit2/Platform/mac/StringUtilities.mm:57
>> +    [regexPatternString replaceOccurrencesOfString:@"$" withString:@"\\$" options:NSLiteralSearch range:NSMakeRange(0, regexPatternString.length)];
> 
> I suppose it’s OK to do it written out like this, although it would be more elegant to have it use a loop to iterate through each character rather than unrolled code.
> 
> How did we check that we covered all the regular expression metacharacters supported by NSRegularExpression? Where did this list of characters come from?

I can rewrite these using a loop.

I got these from NSRegularExpression class reference, which follows ICU regular expression. 
https://developer.apple.com/library/ios/documentation/Foundation/Reference/NSRegularExpression_Class/

>> Source/WebKit2/Platform/mac/StringUtilities.mm:61
>> +    return regexPatternString;
> 
> What happened to putting the "^" at the start and "$" at the end? I would have expected some tests to fail because we did not do this.

We are only matching wildcard characters, so I thought it'll be more consistent to exclude these two characters as well.

>> Source/WebKit2/Platform/mac/StringUtilities.mm:67
>> +    NSRegularExpression *wildcardRegex = [NSRegularExpression regularExpressionWithPattern:wildcardRegexPatternString options:NSRegularExpressionCaseInsensitive error:nullptr];
> 
> It occurs to me what we could use WebCore’s RegularExpression instead of NSRegularExpression. Then we would not have to convert the strings to NSString. But I suppose that might not be OK from the threading point of view?

I asked around and was told that we do use WebCore's RegularExpression with multiple threads and it should be fine to use WebCore's RegularExpression.

>> Source/WebKit2/Platform/mac/StringUtilities.mm:69
>> +    return([wildcardRegex firstMatchInString:string options:0 range:NSMakeRange(0, string.length)] != nil);
> 
> Should remove parentheses here to match normal WebKit coding style and should have a space after the word "return" again.

I'll fix it.

>> Source/WebKit2/Platform/mac/StringUtilities.mm:112
>> +NSString *formattedPhoneNumberString(NSString*)
> 
> Tiny formatting mistake here, should be "NSString *".

Fixed.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:38
>> +#include "StringUtilities.h"
> 
> This include needs to be inside a appropriate #if since this is a platform-specific header. That’s why the GTK and EFL builds are broken.

I'll add a flag for it.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:73
>> +static bool comparatorByStringAscendingLength(const String& string1, const String& string2)
> 
> This should be inside the WebKit namespace, and inside the #if below. The iOS build is failing because it’s not inside the #if.

That's why! okay I'll do that.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:79
>> +        return string1[0] < string2[0];
> 
> This compares only the first character. We want to compare the whole string because we want a well defined ordering even if the first character is the same. Should be easy to fix by writing this:
> 
>     return string1 < string2;

Fixed.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:81
>> +    return (string1Length < string2Length);
> 
> No need for the parentheses here.

Fixed.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:273
>> +    return matchedWildcardHosts.last();
> 
> This is a more efficient way to do the same thing:
> 
>     return *std::max_element(matchedWildcardHosts.begin(), matchedWildcardHosts.end(), comparatorByStringAscendingLength);
> 
> No need to sort the entire vector just to select the first item in it.
> 
> In fact, on reflection, this function does not need to create vectors of strings at all. It could be written as a single loop instead of three separate loops (one inside std::sort or std::max_element), and there is no need to store the intermediate values; just need to store the longest wildcard host seen so far in a local variable. Please rewrite this so it doesn’t use the vectors!

I'll rewrite this.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:298
>> +        replaceHostWithMatchedWildcardHost(hostToLookUp, identifier);
> 
> Slightly better to check identifier.isNull() first, since it’s a much cheaper check than the hash table lookup that contains has to do.

Makes sense. Fixed.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:305
>> +    policiesByIdentifier = m_hostsToPluginIdentifierData.get(hostToLookUp);
> 
> It’s really inefficient that if the host is in the m_hostsToPluginIdentifierData collection, it will look it up four times, calling contains three times then get on the same string. We should restructure the function so that it will do a minimal number of hash table lookups. Just a single find in the common case, and a second one only if there is a wildcard match, and then a lookup of "*" if that fails. So a maximum of tree and only one in the normal case.

I'll rewrite this.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:308
>>      if (!policiesByIdentifier.contains(identifier))
> 
> Same problem here where we call policiesByIdentifier.contains twice because of the way the function is structured.
> 
> Also, both above and below, we end up copying a map out of the other map because we use get, and copying a HashMap is quite inefficient. We should use find instead so we can use the map in place instead of copying it.

I'll rewrite this too.

Comment on attachment 269471 [details]
Revised patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=269471&action=review

>>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:79
>>> +        return string1[0] < string2[0];
>> 
>> This compares only the first character. We want to compare the whole string because we want a well defined ordering even if the first character is the same. Should be easy to fix by writing this:
>> 
>>     return string1 < string2;
> 
> Fixed.

In fact, this wouldn't work because String didn't seem to override the < comparator. I might need to do this myself.

[Darin Adler]

Comment on attachment 269471 [details]
Revised patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=269471&action=review

>>>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:79
>>>> +        return string1[0] < string2[0];
>>> 
>>> This compares only the first character. We want to compare the whole string because we want a well defined ordering even if the first character is the same. Should be easy to fix by writing this:
>>> 
>>>     return string1 < string2;
>> 
>> Fixed.
> 
> In fact, this wouldn't work because String didn't seem to override the < comparator. I might need to do this myself.

That’s crazy. I’m really surprised we don’t implement operator< for String. We should do that at some point. In the mean time, you can write this:

    return codePointCompareLessThan(string1, string2);

That does the right thing.

Comment on attachment 269471 [details]
Revised patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=269471&action=review

>>>>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:79
>>>>> +        return string1[0] < string2[0];
>>>> 
>>>> This compares only the first character. We want to compare the whole string because we want a well defined ordering even if the first character is the same. Should be easy to fix by writing this:
>>>> 
>>>>     return string1 < string2;
>>> 
>>> Fixed.
>> 
>> In fact, this wouldn't work because String didn't seem to override the < comparator. I might need to do this myself.
> 
> That’s crazy. I’m really surprised we don’t implement operator< for String. We should do that at some point. In the mean time, you can write this:
> 
>     return codePointCompareLessThan(string1, string2);
> 
> That does the right thing.

I just realized that since we are not using vectors below, we wouldn't need this function at all! I'm going to remove this entirely.

[Darin Adler]

(In reply to comment #29)
> I just realized that since we are not using vectors below, we wouldn't need
> this function at all! I'm going to remove this entirely.

We will still need the exact same kind of logic to decide if a new match is better than an old one, if it's the same length as the old one. I think you will need something like this function.

Created attachment 269772 [details]
Revised patch.

[WebKit Commit Bot]

Attachment 269772 [details] did not pass style-queue:


ERROR: Source/WebKit2/Platform/mac/StringUtilities.mm:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/Platform/mac/StringUtilities.h:43:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:250:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 269772 [details]
Revised patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=269772&action=review

> Source/WTF/ChangeLog:3
> +        Add __bridge to allow compilation.

This should be the name of the overall bug. The comment you put here belongs on the individual function below.

> Source/WebKit2/Platform/mac/StringUtilities.h:43
> +WK_EXPORT extern String wildcardRegexPatternString(const String& string);

We should not put this in the header. We can test wildcard matching rather than testing this regex pattern string expansion directly and so it can be a private function just used by stringMatchesWildcardString.

If we were keeping it in the header: No need for "extern". No need for the argument name "string".

> Source/WebKit2/Platform/mac/StringUtilities.h:44
> +WK_EXPORT extern bool stringMatchesWildcardString(const String& stringToBeMatched, const String& wildcardString);

No need for "extern".

> Source/WebKit2/Platform/mac/StringUtilities.mm:52
> +    String wildcardRegexPattern = wildcardRegexPatternString(wildcardString);
> +    JSC::Yarr::RegularExpression *wildcardRegularExpression = new JSC::Yarr::RegularExpression(wildcardRegexPattern, TextCaseInsensitive);
> +
> +    int matchLength = 0;
> +    wildcardRegularExpression->match(string, 0, &matchLength);
> +
> +    return matchLength > 0;

This leaks the regular expression object every time it is called. We don’t want to use new at all; no reason to allocate the object on the heap. Here’s what we should write for this entire function:

    return JSC::Yarr::RegularExpression(wildcardRegexPatternString(wildcardString), TextCaseInsensitive).match(string) != -1;

> Source/WebKit2/Platform/mac/StringUtilities.mm:55
> +String wildcardRegexPatternString(const String& string)

Should use static to make this function have internal linkage. Probably want to move it first, before the function that’s currently above.

> Source/WebKit2/Platform/mac/StringUtilities.mm:63
> +    for (size_t i = 0 ; i < string.length(); i++) {

Unwanted space before the ";".

String length is unsigned, not size_t, so loop variable should be unsigned, not size_t.

> Source/WebKit2/Platform/mac/StringUtilities.mm:73
> +    return stringBuilder.toString();

I still think you need to put a "^" before and a "$" after to make his match only the entire string. Need to add test cases for that.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:249
> +    String longestMatchedHost = String();

No need for "= String()" since that’s already what a local variable of type String gets initialized two.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:250
> +    unsigned int matchedHostLength = 0;

Should be unsigned, not unsigned int.

But we do not need this. Instead just use longestMatchedHost.length() in the two places we were using this.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:253
> +        if ((key.contains('*') && key != "*") && stringMatchesWildcardString(host, key)) {

Don’t see the need for the parentheses here between all the && clauses.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:257
> +            } else if (key.length() == matchedHostLength && WTF::codePointCompareLessThan(key, longestMatchedHost))

Should not need WTF:: prefix here. Argument-dependent namespace lookup should find the codePointCompareLessThan for us.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:299
> +    PluginPolicyMapsByIdentifier policiesByIdentifier = policiesByIdentifierIterator->value;

This copies the hash map. To avoid copying it, write:

    auto& policiesByIdentifier = policiesByIdentifierIterator->value;

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:312
> +    PluginLoadClientPoliciesByBundleVersion versionsToPolicies = identifierPolicyIterator->value;

This copies the hash map. To avoid copying it, write:

    auto& versionsToPolicies = identifierPolicyIterator->value;

> Tools/TestWebKitAPI/Tests/WebKit2/mac/StringUtilities.mm:36
> +//    EXPECT_TRUE(true);

Please don’t check in this commented-out code.

> Tools/TestWebKitAPI/Tests/WebKit2/mac/StringUtilities.mm:39
> +//    EXPECT_EQ(WebKit::wildcardRegexPatternString("a*b"), "a.*b");
> +//    EXPECT_EQ("a.*b", WebKit::wildcardRegexPatternString("a*b"));

Please don’t check in this commented-out code.

> Tools/TestWebKitAPI/Tests/WebKit2/mac/StringUtilities.mm:94
> +    EXPECT_FALSE(WebKit::stringMatchesWildcardString("hello", "^hello$"));

Examples of test cases that will show you need to the ^ and $ above:

    EXPECT_FALSE(WebKit::stringMatchesWildcardString("bab", "a*");
    EXPECT_FALSE(WebKit::stringMatchesWildcardString("bab", "*a");
    EXPECT_FALSE(WebKit::stringMatchesWildcardString("bab", "a*b");
    EXPECT_FALSE(WebKit::stringMatchesWildcardString("abcd", "b*c");

[Darin Adler]

Getting really close. Almost ready to land, I think.

Created attachment 269885 [details]
Revised patch.

Comment on attachment 269772 [details]
Revised patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=269772&action=review

>> Source/WebKit2/Platform/mac/StringUtilities.h:43
>> +WK_EXPORT extern String wildcardRegexPatternString(const String& string);
> 
> We should not put this in the header. We can test wildcard matching rather than testing this regex pattern string expansion directly and so it can be a private function just used by stringMatchesWildcardString.
> 
> If we were keeping it in the header: No need for "extern". No need for the argument name "string".

I was testing this but didn't commit the test. I'll remove it.

>> Source/WebKit2/Platform/mac/StringUtilities.h:44
>> +WK_EXPORT extern bool stringMatchesWildcardString(const String& stringToBeMatched, const String& wildcardString);
> 
> No need for "extern".

Removed as well.

>> Source/WebKit2/Platform/mac/StringUtilities.mm:52
>> +    return matchLength > 0;
> 
> This leaks the regular expression object every time it is called. We don’t want to use new at all; no reason to allocate the object on the heap. Here’s what we should write for this entire function:
> 
>     return JSC::Yarr::RegularExpression(wildcardRegexPatternString(wildcardString), TextCaseInsensitive).match(string) != -1;

Fixed.

>> Source/WebKit2/Platform/mac/StringUtilities.mm:55
>> +String wildcardRegexPatternString(const String& string)
> 
> Should use static to make this function have internal linkage. Probably want to move it first, before the function that’s currently above.

Fixed.

>> Source/WebKit2/Platform/mac/StringUtilities.mm:63
>> +    for (size_t i = 0 ; i < string.length(); i++) {
> 
> Unwanted space before the ";".
> 
> String length is unsigned, not size_t, so loop variable should be unsigned, not size_t.

Fixed.

>> Source/WebKit2/Platform/mac/StringUtilities.mm:73
>> +    return stringBuilder.toString();
> 
> I still think you need to put a "^" before and a "$" after to make his match only the entire string. Need to add test cases for that.

Now I think about this, yes we should only match the entire string. I'll add beginning and ending characters.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:249
>> +    String longestMatchedHost = String();
> 
> No need for "= String()" since that’s already what a local variable of type String gets initialized two.

Removed.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:250
>> +    unsigned int matchedHostLength = 0;
> 
> Should be unsigned, not unsigned int.
> 
> But we do not need this. Instead just use longestMatchedHost.length() in the two places we were using this.

Ah yes! Removed.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:253
>> +        if ((key.contains('*') && key != "*") && stringMatchesWildcardString(host, key)) {
> 
> Don’t see the need for the parentheses here between all the && clauses.

Removed.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:257
>> +            } else if (key.length() == matchedHostLength && WTF::codePointCompareLessThan(key, longestMatchedHost))
> 
> Should not need WTF:: prefix here. Argument-dependent namespace lookup should find the codePointCompareLessThan for us.

Gone.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:299
>> +    PluginPolicyMapsByIdentifier policiesByIdentifier = policiesByIdentifierIterator->value;
> 
> This copies the hash map. To avoid copying it, write:
> 
>     auto& policiesByIdentifier = policiesByIdentifierIterator->value;

Fixed.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:312
>> +    PluginLoadClientPoliciesByBundleVersion versionsToPolicies = identifierPolicyIterator->value;
> 
> This copies the hash map. To avoid copying it, write:
> 
>     auto& versionsToPolicies = identifierPolicyIterator->value;

Fixed.

>> Tools/TestWebKitAPI/Tests/WebKit2/mac/StringUtilities.mm:39
>> +//    EXPECT_EQ("a.*b", WebKit::wildcardRegexPatternString("a*b"));
> 
> Please don’t check in this commented-out code.

Oops...I forgot to remove. Removed!

>> Tools/TestWebKitAPI/Tests/WebKit2/mac/StringUtilities.mm:94
>> +    EXPECT_FALSE(WebKit::stringMatchesWildcardString("hello", "^hello$"));
> 
> Examples of test cases that will show you need to the ^ and $ above:
> 
>     EXPECT_FALSE(WebKit::stringMatchesWildcardString("bab", "a*");
>     EXPECT_FALSE(WebKit::stringMatchesWildcardString("bab", "*a");
>     EXPECT_FALSE(WebKit::stringMatchesWildcardString("bab", "a*b");
>     EXPECT_FALSE(WebKit::stringMatchesWildcardString("abcd", "b*c");

Added.

Created attachment 269886 [details]
Revised patch.

The test that crashed, imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection-1.html, passed when running on my machine.

[Darin Adler]

Comment on attachment 269886 [details]
Revised patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=269886&action=review

> Source/WebKit2/Platform/mac/StringUtilities.h:43
> +WK_EXPORT bool stringMatchesWildcardString(const String& stringToBeMatched, const String& wildcardString);

Might want to double check with Anders Carlsson and Dan Bernstein if it’s OK to export something from WebKit just so that it can be unit tested or if there is some alternative that avoids that.

> Source/WebKit2/Platform/mac/StringUtilities.mm:34
>  #import <wtf/text/WTFString.h>

I believe you can now remove this import.

> Source/WebKit2/Platform/mac/StringUtilities.mm:71
> +    String wildcardRegexPattern = wildcardRegexPatternString(wildcardString);
> +
> +    return JSC::Yarr::RegularExpression(wildcardRegexPattern, TextCaseInsensitive).match(string) != -1;

I think this reads better on a single line without the local variable, but I suppose it’s also OK like this. Maybe it wold be better to just name the local variable “pattern”. The longer name makes things less clear, I think.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:255
> +            if (key.length() > longestMatchedHost.length()) {
> +                longestMatchedHost = key;
> +            } else if (key.length() == longestMatchedHost.length() && codePointCompareLessThan(key, longestMatchedHost))

Should remove these braces to match WebKit coding style.

Created attachment 269949 [details]
Fixed styling errors.

Comment on attachment 269886 [details]
Revised patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=269886&action=review

Thanks for reviewing them and great feedbacks!

>> Source/WebKit2/Platform/mac/StringUtilities.h:43
>> +WK_EXPORT bool stringMatchesWildcardString(const String& stringToBeMatched, const String& wildcardString);
> 
> Might want to double check with Anders Carlsson and Dan Bernstein if it’s OK to export something from WebKit just so that it can be unit tested or if there is some alternative that avoids that.

Anders said it's okay to do so. I think I'll leave it as it is.

>> Source/WebKit2/Platform/mac/StringUtilities.mm:34
>>  #import <wtf/text/WTFString.h>
> 
> I believe you can now remove this import.

Removed.

>> Source/WebKit2/Platform/mac/StringUtilities.mm:71
>> +    return JSC::Yarr::RegularExpression(wildcardRegexPattern, TextCaseInsensitive).match(string) != -1;
> 
> I think this reads better on a single line without the local variable, but I suppose it’s also OK like this. Maybe it wold be better to just name the local variable “pattern”. The longer name makes things less clear, I think.

Agreed. Removed.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:255
>> +            } else if (key.length() == longestMatchedHost.length() && codePointCompareLessThan(key, longestMatchedHost))
> 
> Should remove these braces to match WebKit coding style.

Fixed.

Created attachment 269950 [details]
Fixed styling errors.

[WebKit Commit Bot]

Comment on attachment 269950 [details]
Fixed styling errors.

Clearing flags on attachment: 269950

Committed r195650: <http://trac.webkit.org/changeset/195650>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 269950 [details]
Fixed styling errors.

View in context: https://bugs.webkit.org/attachment.cgi?id=269950&action=review

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:252
> +        if (key.contains('*') && key != "*" && stringMatchesWildcardString(host, key)) {

One thing I thought about after you landed this patch:

We should consider if what the value is of disallowing matching the string "*" but not disallowing the string "**". Maybe what we need to do is to disallow matching on any string that entirely consists of only "*" characters. We could write an isValidWildcardString function to implement this rule:

    static bool isValidWildcardString(StringView wildcardString)
    {
        unsigned starCount = 0;
        for (UChar character : wildcardString.codeUnits())
            starCount += character == '*';
        return startCount && startCount < wildcardString.length();
    }

Also would be nice to add a couple test cases for stringMatchesWildcardString to check the behavior of these kinds of "match everything" patterns, even though we are not using the function that way.

Comment on attachment 269950 [details]
Fixed styling errors.

View in context: https://bugs.webkit.org/attachment.cgi?id=269950&action=review

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:252
>> +        if (key.contains('*') && key != "*" && stringMatchesWildcardString(host, key)) {
> 
> One thing I thought about after you landed this patch:
> 
> We should consider if what the value is of disallowing matching the string "*" but not disallowing the string "**". Maybe what we need to do is to disallow matching on any string that entirely consists of only "*" characters. We could write an isValidWildcardString function to implement this rule:
> 
>     static bool isValidWildcardString(StringView wildcardString)
>     {
>         unsigned starCount = 0;
>         for (UChar character : wildcardString.codeUnits())
>             starCount += character == '*';
>         return startCount && startCount < wildcardString.length();
>     }
> 
> Also would be nice to add a couple test cases for stringMatchesWildcardString to check the behavior of these kinds of "match everything" patterns, even though we are not using the function that way.

Filed https://bugs.webkit.org/show_bug.cgi?id=153749 for this.