Bug 219257

Summary: [GTK] Allow WebKitTestServer to run non-loopback addresses for API tests
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: Page LoadingAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: REOPENED ---    
Severity: Normal CC: beidson, cgarcia, commit-queue, mcatanzaro, pgriffis, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 219160, 220922, 222273    
Bug Blocks: 171934, 218623, 219198    
Attachments:
Description Flags
Tentative patch (untested)
mcatanzaro: review+
Patch (reverted)
none
Patch none

Description Frédéric Wang (:fredw) 2020-11-23 06:31:35 PST
This is needed to test WEBKIT_INSECURE_CONTENT_RUN and WEBKIT_INSECURE_CONTENT_DISPLAYED APIs if localhost & loopback IP addresses are no longer mixed content. Unfortunately, that does not seem possible until bug 219198 is fixed. Meanwhile, let's try to move from "127.0.0.1" to "localhost" as Michael suggested elsewhere.
Comment 1 Frédéric Wang (:fredw) 2020-11-23 06:36:28 PST
Created attachment 414801 [details]
Tentative patch (untested)
Comment 2 Frédéric Wang (:fredw) 2020-11-23 07:31:20 PST
Comment on attachment 414801 [details]
Tentative patch (untested)

It looks like the API tests pass after this change, so I'm asking review now. Will update the changelog later, but basically this is doing Michael's suggestion in bug 218623 comment 15, so that the test won't be broken when we stop treating loopback IP address as mixed content.
Comment 3 Michael Catanzaro 2020-11-23 08:08:16 PST
Comment on attachment 414801 [details]
Tentative patch (untested)

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

> Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:35
> +        ServerNonLocalHost = 1 << 3,

If you call it ServerNonLoopback instead, this would be a lot less weird. Because passing ServerNonLocalHost to cause the server to use localhost is weird. ;)
Comment 4 Michael Catanzaro 2020-11-23 08:12:00 PST
Comment on attachment 414801 [details]
Tentative patch (untested)

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

LGTM. I see TestSSL now needs two HTTP servers instead of one, but that's fine.

I bet Carlos Garcia will want to review this before it lands.

> Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:38
> -    WebKitTestServer(ServerOptions = ServerHTTP);
> +    WebKitTestServer(uint8_t options = ServerHTTP);

I don't like this... why are you removing its nice type?
Comment 5 Frédéric Wang (:fredw) 2020-11-23 09:15:27 PST
(In reply to Michael Catanzaro from comment #3)
> Comment on attachment 414801 [details]
> Tentative patch (untested)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414801&action=review
> 
> > Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:35
> > +        ServerNonLocalHost = 1 << 3,
> 
> If you call it ServerNonLoopback instead, this would be a lot less weird.
> Because passing ServerNonLocalHost to cause the server to use localhost is
> weird. ;)

Right, but ultimately I want to call it ServerNonLocalHost as we want to stop treating localhost as mixed content too. I'm fine keeping this name for now... but maybe people would add new tests assuming it's just not loopback.

(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 414801 [details]
> Tentative patch (untested)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414801&action=review
> 
> LGTM. I see TestSSL now needs two HTTP servers instead of one, but that's
> fine.

Actually three (2 HTTPS and HTTP). I can also change the tests to always use the new version HTTPS version, but not sure that really makes a difference.
 
> I bet Carlos Garcia will want to review this before it lands.
> 
> > Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:38
> > -    WebKitTestServer(ServerOptions = ServerHTTP);
> > +    WebKitTestServer(uint8_t options = ServerHTTP);
> 
> I don't like this... why are you removing its nice type?

ServerOptions is currently an enum of bit flags, so it can't accept a combination ServerHttps | ServerNonLoopback.
Comment 6 Michael Catanzaro 2020-11-23 10:31:43 PST
(In reply to Frédéric Wang (:fredw) from comment #5)
> ServerOptions is currently an enum of bit flags, so it can't accept a
> combination ServerHttps | ServerNonLoopback.

Try: static_cast<ServerOptions>(ServerHttps | ServerNonLoopback)
Comment 7 Michael Catanzaro 2020-11-23 10:32:21 PST
Or better: try std::bitset!
Comment 8 Frédéric Wang (:fredw) 2020-11-24 05:58:55 PST
Created attachment 414835 [details]
Patch (reverted)
Comment 9 Frédéric Wang (:fredw) 2020-11-24 06:01:15 PST
(In reply to Michael Catanzaro from comment #3)
> If you call it ServerNonLoopback instead, this would be a lot less weird.
> Because passing ServerNonLocalHost to cause the server to use localhost is
> weird. ;)

Done

(In reply to Michael Catanzaro from comment #4)
> LGTM. I see TestSSL now needs two HTTP servers instead of one, but that's
> fine.

Merged to use only one HTTPS server, let's see if that does not break any test...

(In reply to Michael Catanzaro from comment #7)
> Or better: try std::bitset!

Done.
Comment 10 EWS 2020-11-26 16:41:21 PST
Committed r270170: <https://trac.webkit.org/changeset/270170>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414835 [details].
Comment 11 WebKit Commit Bot 2021-01-25 07:06:48 PST
Re-opened since this is blocked by bug 220922
Comment 12 Radar WebKit Bug Importer 2021-01-25 07:07:06 PST
<rdar://problem/73570185>
Comment 13 Frédéric Wang (:fredw) 2021-02-22 14:08:12 PST
(In reply to Frédéric Wang (:fredw) from comment #9)
> (In reply to Michael Catanzaro from comment #7)
> > Or better: try std::bitset!
> 
> Done.

This change was problematic, std::bitset's constructor takes a bit mask but its setter/getter relies on the bit's position instead. I've extracted this change to bug 222273.
Comment 14 Frédéric Wang (:fredw) 2021-02-24 07:44:18 PST
Created attachment 421409 [details]
Patch

I tried a bit to rebase, but was not able to make the certificate stuff work. Probably some special config is needed for the domain name...