Bug 219257 – [GTK] Allow WebKitTestServer to run non-loopback addresses for API tests

• Status: REOPENED
• Component: Page Loading

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 414801 [details]
Tentative patch (untested)

[Frédéric Wang (:fredw)]

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.

[Michael Catanzaro]

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. ;)

[Michael Catanzaro]

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?

[Frédéric Wang (:fredw)]

(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.

[Michael Catanzaro]

(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)

[Michael Catanzaro]

Or better: try std::bitset!

[Frédéric Wang (:fredw)]

Created attachment 414835 [details]
Patch (reverted)

[Frédéric Wang (:fredw)]

(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.

[EWS]

Committed r270170: <https://trac.webkit.org/changeset/270170>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414835 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 220922

[Radar WebKit Bug Importer]

<rdar://problem/73570185>

[Frédéric Wang (:fredw)]

(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.

[Frédéric Wang (:fredw)]

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...

[Frédéric Wang (:fredw)]

Removing myself from assignee since I'm not working on this anymore.