Bug 184239 – Add infrastructure to relax SSL for allowed hosts in DumpRenderTree and WebKitTestRunner

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

Add infrastructure to relax SSL for allowed hosts in DumpRenderTree and WebKitTestRunner.

[Daniel Bates]

Created attachment 336995 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/WebKitTestRunner/TestController.cpp:1763:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


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

[Alexey Proskuryakov]

Comment on attachment 336995 [details]
Patch

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

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1253
> +            [NSURLRequest setAllowsAnyHTTPSCertificate:YES forHost:[[NSString alloc] initWithUTF8String:host.c_str()]];

This leaks the string (as we don't seem to be using ARC in DRT).

> Tools/WebKitTestRunner/InjectedBundle/ios/InjectedBundleIOS.mm:57
> +        [NSURLRequest setAllowsAnyHTTPSCertificate:YES forHost:host];

What are the iOS versions where we still perform network loading from the WebContent process?

> Tools/WebKitTestRunner/InjectedBundle/mac/InjectedBundleMac.mm:99
> +    for (auto& host : m_allowedHosts)
> +        [NSURLRequest setAllowsAnyHTTPSCertificate:YES forHost:host];

What are the macOS versions where we still perform network loading from the WebContent process?

> Tools/WebKitTestRunner/TestController.cpp:1763
> +            return std::any_of(m_allowedHosts.begin(), m_allowedHosts.end(), [host] (const std::string& allowedHost) { return host == allowedHost; });

Wouldn't m_allowedHosts.find() work here?

[Daniel Bates]

(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 336995 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336995&action=review
> 
> > Tools/DumpRenderTree/mac/DumpRenderTree.mm:1253
> > +            [NSURLRequest setAllowsAnyHTTPSCertificate:YES forHost:[[NSString alloc] initWithUTF8String:host.c_str()]];
> 
> This leaks the string (as we don't seem to be using ARC in DRT).
> 

Oops! Will use -autorelease.

> > Tools/WebKitTestRunner/InjectedBundle/ios/InjectedBundleIOS.mm:57
> > +        [NSURLRequest setAllowsAnyHTTPSCertificate:YES forHost:host];
> 
> What are the iOS versions where we still perform network loading from the
> WebContent process?
> 
> > Tools/WebKitTestRunner/InjectedBundle/mac/InjectedBundleMac.mm:99
> > +    for (auto& host : m_allowedHosts)
> > +        [NSURLRequest setAllowsAnyHTTPSCertificate:YES forHost:host];
> 
> What are the macOS versions where we still perform network loading from the
> WebContent process?
> 

I do not know. I hope you do not mind that I defer cleaning this up.

> > Tools/WebKitTestRunner/TestController.cpp:1763
> > +            return std::any_of(m_allowedHosts.begin(), m_allowedHosts.end(), [host] (const std::string& allowedHost) { return host == allowedHost; });
> 
> Wouldn't m_allowedHosts.find() work here?

Will change m_allowedHosts from a std::vector to a std::set and use std::set::find().

[Daniel Bates]

(In reply to Alexey Proskuryakov from comment #3)
> > Tools/WebKitTestRunner/InjectedBundle/ios/InjectedBundleIOS.mm:57
> > +        [NSURLRequest setAllowsAnyHTTPSCertificate:YES forHost:host];
> 
> What are the iOS versions where we still perform network loading from the
> WebContent process?
> 

I don't know. I hope you do not mind that I defer cleaning this up.

[Daniel Bates]

(In reply to Daniel Bates from comment #5)
> (In reply to Alexey Proskuryakov from comment #3)
> > > Tools/WebKitTestRunner/InjectedBundle/ios/InjectedBundleIOS.mm:57
> > > +        [NSURLRequest setAllowsAnyHTTPSCertificate:YES forHost:host];
> > 
> > What are the iOS versions where we still perform network loading from the
> > WebContent process?
> > 
> 
> I don't know. I hope you do not mind that I defer cleaning this up.

We no longer need this code. We remove this code in bug #184242.

(In reply to Daniel Bates from comment #4)
> > > Tools/WebKitTestRunner/InjectedBundle/mac/InjectedBundleMac.mm:99
> > > +    for (auto& host : m_allowedHosts)
> > > +        [NSURLRequest setAllowsAnyHTTPSCertificate:YES forHost:host];
> > 
> > What are the macOS versions where we still perform network loading from the
> > WebContent process?
> > 
> 
> I do not know. I hope you do not mind that I defer cleaning this up.

We no longer need this code. We remove this code in bug #184242.

[Daniel Bates]

(In reply to Daniel Bates from comment #4)
> (In reply to Alexey Proskuryakov from comment #3)
> > Comment on attachment 336995 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=336995&action=review
> > 
> > > Tools/DumpRenderTree/mac/DumpRenderTree.mm:1253
> > > +            [NSURLRequest setAllowsAnyHTTPSCertificate:YES forHost:[[NSString alloc] initWithUTF8String:host.c_str()]];
> > 
> > This leaks the string (as we don't seem to be using ARC in DRT).
> > 
> 
> Oops! Will use -autorelease.
> 

Even better, will use the convenience API -stringWithUTF8String as it calls -autorelease and using it simplifies this code.

[Daniel Bates]

Committed r230192: <https://trac.webkit.org/changeset/230192>

[Radar WebKit Bug Importer]

<rdar://problem/39122881>