[WPE][GTK] Unsafe g_unsetenv() use in WebProcessPool::platformInitialize
authormcatanzaro@igalia.com <mcatanzaro@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Feb 2019 18:45:31 +0000 (18:45 +0000)
committermcatanzaro@igalia.com <mcatanzaro@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Feb 2019 18:45:31 +0000 (18:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194370

Reviewed by Darin Adler.

Source/JavaScriptCore:

Change a couple WTFLogAlways to use g_warning, for good measure. Of course this isn't
necessary, but it will make errors more visible.

* inspector/remote/glib/RemoteInspectorGlib.cpp:
(Inspector::RemoteInspector::start):
(Inspector::dbusConnectionCallAsyncReadyCallback):
* inspector/remote/glib/RemoteInspectorServer.cpp:
(Inspector::RemoteInspectorServer::start):

Source/WebKit:

It is incorrect to use g_unsetenv() here because it is MT-Unsafe. We know that it is
impossible and unreasonable to expect the application has not started other threads at this
point, and threads will be calling getenv(). WebKit itself has probably already started
threads of its own.

Fortunately, the remote inspector in the web process is already prepared to deal with
failure to connect to the inspector server, so we don't need to do anything except stop
messing with the environment.

Note these files are copies of each other. I'll merge them together in a follow-up patch.

* UIProcess/gtk/WebProcessPoolGtk.cpp:
(WebKit::initializeRemoteInspectorServer):
(WebKit::WebProcessPool::platformInitialize):
* UIProcess/wpe/WebProcessPoolWPE.cpp:
(WebKit::initializeRemoteInspectorServer):
(WebKit::WebProcessPool::platformInitialize):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@241304 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp
Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp
Source/WebKit/UIProcess/wpe/WebProcessPoolWPE.cpp

index cc97c16..7551479 100644 (file)
@@ -1,3 +1,19 @@
+2019-02-12  Michael Catanzaro  <mcatanzaro@igalia.com>
+
+        [WPE][GTK] Unsafe g_unsetenv() use in WebProcessPool::platformInitialize
+        https://bugs.webkit.org/show_bug.cgi?id=194370
+
+        Reviewed by Darin Adler.
+
+        Change a couple WTFLogAlways to use g_warning, for good measure. Of course this isn't
+        necessary, but it will make errors more visible.
+
+        * inspector/remote/glib/RemoteInspectorGlib.cpp:
+        (Inspector::RemoteInspector::start):
+        (Inspector::dbusConnectionCallAsyncReadyCallback):
+        * inspector/remote/glib/RemoteInspectorServer.cpp:
+        (Inspector::RemoteInspectorServer::start):
+
 2019-02-12  Andy Estes  <aestes@apple.com>
 
         [iOSMac] Enable Parental Controls Content Filtering
index b73455a..c075698 100644 (file)
@@ -79,7 +79,7 @@ void RemoteInspector::start()
             if (GRefPtr<GDBusConnection> connection = adoptGRef(g_dbus_connection_new_for_address_finish(result, &error.outPtr())))
                 inspector->setupConnection(WTFMove(connection));
             else if (!g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED))
-                WTFLogAlways("RemoteInspector failed to connect to inspector server at: %s: %s", g_getenv("WEBKIT_INSPECTOR_SERVER"), error->message);
+                g_warning("RemoteInspector failed to connect to inspector server at: %s: %s", g_getenv("WEBKIT_INSPECTOR_SERVER"), error->message);
     }, this);
 }
 
@@ -178,7 +178,7 @@ static void dbusConnectionCallAsyncReadyCallback(GObject* source, GAsyncResult*
     GUniqueOutPtr<GError> error;
     GRefPtr<GVariant> resultVariant = adoptGRef(g_dbus_connection_call_finish(G_DBUS_CONNECTION(source), result, &error.outPtr()));
     if (!resultVariant && !g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED))
-        WTFLogAlways("RemoteInspector failed to send DBus message: %s", error->message);
+        g_warning("RemoteInspector failed to send DBus message: %s", error->message);
 }
 
 TargetListing RemoteInspector::listingForInspectionTarget(const RemoteInspectionTarget& target) const
index dc09c72..453e3a0 100644 (file)
@@ -197,7 +197,7 @@ bool RemoteInspectorServer::start(const char* address, unsigned port)
     m_dbusServer = adoptGRef(g_dbus_server_new_sync(dbusAddress.get(), G_DBUS_SERVER_FLAGS_AUTHENTICATION_ALLOW_ANONYMOUS, uid.get(), nullptr, m_cancellable.get(), &error.outPtr()));
     if (!m_dbusServer) {
         if (!g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED))
-            WTFLogAlways("Failed to start remote inspector server on %s: %s\n", dbusAddress.get(), error->message);
+            g_warning("Failed to start remote inspector server on %s: %s\n", dbusAddress.get(), error->message);
         return false;
     }
 
index ce65a9e..fca2de3 100644 (file)
@@ -1,3 +1,28 @@
+2019-02-12  Michael Catanzaro  <mcatanzaro@igalia.com>
+
+        [WPE][GTK] Unsafe g_unsetenv() use in WebProcessPool::platformInitialize
+        https://bugs.webkit.org/show_bug.cgi?id=194370
+
+        Reviewed by Darin Adler.
+
+        It is incorrect to use g_unsetenv() here because it is MT-Unsafe. We know that it is
+        impossible and unreasonable to expect the application has not started other threads at this
+        point, and threads will be calling getenv(). WebKit itself has probably already started
+        threads of its own.
+
+        Fortunately, the remote inspector in the web process is already prepared to deal with
+        failure to connect to the inspector server, so we don't need to do anything except stop
+        messing with the environment.
+
+        Note these files are copies of each other. I'll merge them together in a follow-up patch.
+
+        * UIProcess/gtk/WebProcessPoolGtk.cpp:
+        (WebKit::initializeRemoteInspectorServer):
+        (WebKit::WebProcessPool::platformInitialize):
+        * UIProcess/wpe/WebProcessPoolWPE.cpp:
+        (WebKit::initializeRemoteInspectorServer):
+        (WebKit::WebProcessPool::platformInitialize):
+
 2019-02-08  Beth Dakin  <bdakin@apple.com>
 
         Ensure old binaries have old snapshotting behaviors
index 5127d3a..a3b0dec 100644 (file)
 namespace WebKit {
 
 #if ENABLE(REMOTE_INSPECTOR)
-static bool initializeRemoteInspectorServer(const char* address)
+static void initializeRemoteInspectorServer(const char* address)
 {
     if (Inspector::RemoteInspectorServer::singleton().isRunning())
-        return true;
+        return;
 
     if (!address[0])
-        return false;
+        return;
 
     GUniquePtr<char> inspectorAddress(g_strdup(address));
     char* portPtr = g_strrstr(inspectorAddress.get(), ":");
     if (!portPtr)
-        return false;
+        return;
 
     *portPtr = '\0';
     portPtr++;
     guint64 port = g_ascii_strtoull(portPtr, nullptr, 10);
     if (!port)
-        return false;
+        return;
 
-    return Inspector::RemoteInspectorServer::singleton().start(inspectorAddress.get(), port);
+    Inspector::RemoteInspectorServer::singleton().start(inspectorAddress.get(), port);
 }
 #endif
 
@@ -77,10 +77,8 @@ static bool memoryPressureMonitorDisabled()
 void WebProcessPool::platformInitialize()
 {
 #if ENABLE(REMOTE_INSPECTOR)
-    if (const char* address = g_getenv("WEBKIT_INSPECTOR_SERVER")) {
-        if (!initializeRemoteInspectorServer(address))
-            g_unsetenv("WEBKIT_INSPECTOR_SERVER");
-    }
+    if (const char* address = g_getenv("WEBKIT_INSPECTOR_SERVER"))
+        initializeRemoteInspectorServer(address);
 #endif
 
     if (!memoryPressureMonitorDisabled())
index f51017a..1d9e2f8 100644 (file)
 namespace WebKit {
 
 #if ENABLE(REMOTE_INSPECTOR)
-static bool initializeRemoteInspectorServer(const char* address)
+static void initializeRemoteInspectorServer(const char* address)
 {
     if (Inspector::RemoteInspectorServer::singleton().isRunning())
-        return true;
+        return;
 
     if (!address[0])
-        return false;
+        return;
 
     GUniquePtr<char> inspectorAddress(g_strdup(address));
     char* portPtr = g_strrstr(inspectorAddress.get(), ":");
     if (!portPtr)
-        return false;
+        return;
 
     *portPtr = '\0';
     portPtr++;
     guint64 port = g_ascii_strtoull(portPtr, nullptr, 10);
     if (!port)
-        return false;
+        return;
 
-    return Inspector::RemoteInspectorServer::singleton().start(inspectorAddress.get(), port);
+    Inspector::RemoteInspectorServer::singleton().start(inspectorAddress.get(), port);
 }
 #endif
 
 void WebProcessPool::platformInitialize()
 {
 #if ENABLE(REMOTE_INSPECTOR)
-    if (const char* address = g_getenv("WEBKIT_INSPECTOR_SERVER")) {
-        if (!initializeRemoteInspectorServer(address))
-            g_unsetenv("WEBKIT_INSPECTOR_SERVER");
-    }
+    if (const char* address = g_getenv("WEBKIT_INSPECTOR_SERVER"))
+        initializeRemoteInspectorServer(address);
 #endif
 }