REGRESSION(r270074): [WPE][GTK] Problems when creating SoupServer in WebKitTestServer::WebKitTestServer

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Using libsoup (built from latest commit on gnome-3-38 branch), I notice that WebKitTestServer crashes whenever it is constructed, which prevents running API tests that use it. The error is:

Failed to start HTTP server: Can’t create a TLS server without a TLS certificate

Problem is here:

SoupServerListenOptions serverOptions = static_cast<SoupServerListenOptions>(options[ServerHTTPS] ? SOUP_SERVER_LISTEN_IPV4_ONLY : SOUP_SERVER_LISTEN_IPV4_ONLY | SOUP_SERVER_LISTEN_HTTPS);

Notice that the logic was inverted by mistake in r270074. We pass SOUP_SERVER_LISTEN_HTTPS only when options[ServerHTTPS] is not set. That is, whenever we should not be listening for HTTPS, we listen for HTTPS. And whenever we should be listening for HTTPS, we don't.

This leads me to question: how does anything currently work? I see API tests are actually currently passing on the release bots, so... ?????

I notice we also currently pass SOUP_SERVER_LISTEN_IPV4_ONLY to vanilla soup_server_listen(), which does nothing. We should probably stop doing that, so my suggested fix would be:

diff --git a/Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.cpp b/Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.cpp
index 2a0720fc3fdd..6baaaddad01d 100644
--- a/Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.cpp
+++ b/Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.cpp
@@ -45,14 +45,13 @@ WebKitTestServer::WebKitTestServer(ServerOptionsBitSet options)
         SOUP_SERVER_SSL_KEY_FILE, sslKeyFile.get(), nullptr));
 
     GUniqueOutPtr<GError> error;
-    SoupServerListenOptions serverOptions = static_cast<SoupServerListenOptions>(options[ServerHTTPS] ? SOUP_SERVER_LISTEN_IPV4_ONLY : SOUP_SERVER_LISTEN_IPV4_ONLY | SOUP_SERVER_LISTEN_HTTPS);
     bool serverStarted = false;
     if (options[ServerNonLoopback]) {
         GRefPtr<SoupAddress> address = adoptGRef(soup_address_new("localhost", SOUP_ADDRESS_ANY_PORT));
         soup_address_resolve_sync(address.get(), nullptr);
-        serverStarted = soup_server_listen(m_soupServer.get(), soup_address_get_gsockaddr(address.get()), serverOptions, &error.outPtr());
+        serverStarted = soup_server_listen(m_soupServer.get(), soup_address_get_gsockaddr(address.get()), options[ServerHTTPS] ? SOUP_SERVER_LISTEN_HTTPS : static_cast<SoupServerListenOptions>(0), &error.outPtr());
     } else
-        serverStarted = soup_server_listen_local(m_soupServer.get(), SOUP_ADDRESS_ANY_PORT, serverOptions, &error.outPtr());
+        serverStarted = soup_server_listen_local(m_soupServer.get(), SOUP_ADDRESS_ANY_PORT, options[ServerHTTPS] ? static_cast<SoupServerListenOptions>(SOUP_SERVER_LISTEN_IPV4_ONLY | SOUP_SERVER_LISTEN_HTTPS) : SOUP_SERVER_LISTEN_IPV4_ONLY, &error.outPtr());
     if (!serverStarted) {
         WTFLogAlways("Failed to start HTTP server: %s", error->message);
         CRASH();

Of course, if that worked, I would submit a proper patch. It doesn't work. serverStarted is true, so the listen call succeeds, but soup_server_get_uris() returns NULL, and then we crash. (A smaller patch that just swaps SOUP_SERVER_LISTEN_HTTPS around crashes the same way.) It smells like a libsoup bug. Surely if the call to listen() succeeds, then soup_server_get_uris() should return results?

[Carlos Garcia Campos]

The soup server impl is indeed very broken. I also don't understand how it can work in the bots and EWS. I'm not sure it's worth reverting the original commits or just fix current impl at this point. In any case we should make sure the bots are running the actual code and not an old version or whatever is going on.

Michael's patch looks good, but it0's not enough:

1- We are assuming soup_server_get_uris() returns a list og objects, but SoupURI is not an object. That means we should use soup_uri_copy instead of g_object_ref when setting m_baseURI (or steal the uri somehow) and soup_uri_free instead of g_object_ref when freeing the list.

2- We are still calling soup_server_run_async() that should only be called from the old API. We can just omit it, it's not needed with the new API.

Michael, could you submit a new patch with those things fixed, please?

Diego, Lauro or Carlos, could any of you check why tests are running fine in the bots, please?

Thanks!

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #1)
> Michael, could you submit a new patch with those things fixed, please?

Sure. That indeed seems to be enough to fix WebKitTestServer for me locally. I guess we should probably not land this until we know what's happening with the bots, though.

[Michael Catanzaro]

Created attachment 418046 [details]
Patch

[Michael Catanzaro]

Comment on attachment 418046 [details]
Patch

Looks like API tests EWS really hates this patch.

[Carlos Garcia Campos]

I think there are two issues. We are still using soup_server_get_port() which is also "old" api, and I think https server is somehow broken too.

[Michael Catanzaro]

I don't have time to go further. We might need to revert r270074 and r270170. They were needed for bug #171934, but that's stalled anyway.

[Diego Pino]

(In reply to Carlos Garcia Campos from comment #1)

> Diego, Lauro or Carlos, could any of you check why tests are running fine in
> the bots, please?
> 
> Thanks!

I built libsoup gnome3-38 branch, it builds libsoup2.72.0 which corresponds to this commit:

$ git log -1 2.72.0
commit ae1632c176c60b7fe832024c0a958f4079767c44 (tag: 2.72.0)
Author: Patrick Griffis <[e-mail]>
Date:   Sun Sep 13 15:53:40 2020 -0700

    2.72.0

On the other hand, WebKitGTK, both with jhbuild or Flatpak SDK, links using libsoup2.71.0, which corresponds to this commit:

$ git log -1 2.71.0
commit d7b8e46510e6a4d1dbcd38d3330e83c41086cc2f (tag: 2.71.0)
Author: Patrick Griffis <[e-mail]>
Date:   Tue Jul 7 15:23:44 2020 -0700

    2.71.0

So I think this might be the reason why the tests are passing in the bots. To test the use case mentioned by Michael we would need to update the libsoup version in jhbuild modules and run the tests.

[Carlos Garcia Campos]

The libsoup version shouldn't matter. We are mixing new and old API that is present in the minimum libsoup version and used unconditionally. We are also handling SoupURI as a GObject. That should make all the tests crash in the bots.

[Carlos Garcia Campos]

std::bitset is also wrongly used. There are too many issues to fix here. I'm going to revert the commits and I'll work on removing deprecated libsoup api as part of the preparation for libsoup3 migration.

[Michael Catanzaro]

Changes reverted in bug #220922. Closing.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> Using libsoup (built from latest commit on gnome-3-38 branch), I notice that
> WebKitTestServer crashes whenever it is constructed, which prevents running
> API tests that use it. The error is:
> 
> Failed to start HTTP server: Can’t create a TLS server without a TLS
> certificate

Also this error has been around for years, so it's not something that would have changed between libsoup 2.70 and libsoup 2.72. That suggests something is seriously wrong with the API test bots; it's as if they were running the code from before this change, rather than the current code.

And yet the EWS in this bug *did* pick up my changes and turn red, so... I don't know.

[Michael Catanzaro]

The adventure continues in bug #220922. Incredibly, the revert has broken all the tests that use WebKitTestServer. Sigh.

[Alicia Boya García]

(In reply to Michael Catanzaro from comment #11)
> (In reply to Michael Catanzaro from comment #0)
> > Using libsoup (built from latest commit on gnome-3-38 branch), I notice that
> > WebKitTestServer crashes whenever it is constructed, which prevents running
> > API tests that use it. The error is:
> > 
> > Failed to start HTTP server: Can’t create a TLS server without a TLS
> > certificate
> 
> Also this error has been around for years, so it's not something that would
> have changed between libsoup 2.70 and libsoup 2.72. That suggests something
> is seriously wrong with the API test bots; it's as if they were running the
> code from before this change, rather than the current code.
> 
> And yet the EWS in this bug *did* pick up my changes and turn red, so... I
> don't know.

The API test runner has a bug where it doesn't report crashing test suites. https://bugs.webkit.org/show_bug.cgi?id=220863