Changeset 241304 in webkit


Ignore:
Timestamp:
Feb 12, 2019 10:45:31 AM (5 years ago)
Author:
Michael Catanzaro
Message:

[WPE][GTK] Unsafe g_unsetenv() use in WebProcessPool::platformInitialize
​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):

Location:
trunk/Source
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r241299 r241304  
     12019-02-12  Michael Catanzaro  <mcatanzaro@igalia.com>
     2
     3        [WPE][GTK] Unsafe g_unsetenv() use in WebProcessPool::platformInitialize
     4        https://bugs.webkit.org/show_bug.cgi?id=194370
     5
     6        Reviewed by Darin Adler.
     7
     8        Change a couple WTFLogAlways to use g_warning, for good measure. Of course this isn't
     9        necessary, but it will make errors more visible.
     10
     11        * inspector/remote/glib/RemoteInspectorGlib.cpp:
     12        (Inspector::RemoteInspector::start):
     13        (Inspector::dbusConnectionCallAsyncReadyCallback):
     14        * inspector/remote/glib/RemoteInspectorServer.cpp:
     15        (Inspector::RemoteInspectorServer::start):
     16
    1172019-02-12  Andy Estes  <aestes@apple.com>
    218

  • trunk/Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp

    r240330 r241304  
    8080                inspector->setupConnection(WTFMove(connection));
    8181            else if (!g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED))
    82                 WTFLogAlways("RemoteInspector failed to connect to inspector server at: %s: %s", g_getenv("WEBKIT_INSPECTOR_SERVER"), error->message);
     82                g_warning("RemoteInspector failed to connect to inspector server at: %s: %s", g_getenv("WEBKIT_INSPECTOR_SERVER"), error->message);
    8383    }, this);
    8484}
    … …  
    179179    GRefPtr<GVariant> resultVariant = adoptGRef(g_dbus_connection_call_finish(G_DBUS_CONNECTION(source), result, &error.outPtr()));
    180180    if (!resultVariant && !g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED))
    181         WTFLogAlways("RemoteInspector failed to send DBus message: %s", error->message);
     181        g_warning("RemoteInspector failed to send DBus message: %s", error->message);
    182182}
    183183

  • trunk/Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp

    r233077 r241304  
    198198    if (!m_dbusServer) {
    199199        if (!g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED))
    200             WTFLogAlways("Failed to start remote inspector server on %s: %s\n", dbusAddress.get(), error->message);
     200            g_warning("Failed to start remote inspector server on %s: %s\n", dbusAddress.get(), error->message);
    201201        return false;
    202202    }

  • trunk/Source/WebKit/ChangeLog

    r241303 r241304  
     12019-02-12  Michael Catanzaro  <mcatanzaro@igalia.com>
     2
     3        [WPE][GTK] Unsafe g_unsetenv() use in WebProcessPool::platformInitialize
     4        https://bugs.webkit.org/show_bug.cgi?id=194370
     5
     6        Reviewed by Darin Adler.
     7
     8        It is incorrect to use g_unsetenv() here because it is MT-Unsafe. We know that it is
     9        impossible and unreasonable to expect the application has not started other threads at this
     10        point, and threads will be calling getenv(). WebKit itself has probably already started
     11        threads of its own.
     12
     13        Fortunately, the remote inspector in the web process is already prepared to deal with
     14        failure to connect to the inspector server, so we don't need to do anything except stop
     15        messing with the environment.
     16
     17        Note these files are copies of each other. I'll merge them together in a follow-up patch.
     18
     19        * UIProcess/gtk/WebProcessPoolGtk.cpp:
     20        (WebKit::initializeRemoteInspectorServer):
     21        (WebKit::WebProcessPool::platformInitialize):
     22        * UIProcess/wpe/WebProcessPoolWPE.cpp:
     23        (WebKit::initializeRemoteInspectorServer):
     24        (WebKit::WebProcessPool::platformInitialize):
     25
    1262019-02-08  Beth Dakin  <bdakin@apple.com>
    227

  • trunk/Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp

    r241224 r241304  
    4646
    4747#if ENABLE(REMOTE_INSPECTOR)
    48 static bool initializeRemoteInspectorServer(const char* address)
     48static void initializeRemoteInspectorServer(const char* address)
    4949{
    5050    if (Inspector::RemoteInspectorServer::singleton().isRunning())
    51         return true;
     51        return;
    5252
    5353    if (!address[0])
    54         return false;
     54        return;
    5555
    5656    GUniquePtr<char> inspectorAddress(g_strdup(address));
    5757    char* portPtr = g_strrstr(inspectorAddress.get(), ":");
    5858    if (!portPtr)
    59         return false;
     59        return;
    6060
    6161    *portPtr = '\0';
    … …  
    6363    guint64 port = g_ascii_strtoull(portPtr, nullptr, 10);
    6464    if (!port)
    65         return false;
     65        return;
    6666
    67     return Inspector::RemoteInspectorServer::singleton().start(inspectorAddress.get(), port);
     67    Inspector::RemoteInspectorServer::singleton().start(inspectorAddress.get(), port);
    6868}
    6969#endif
    … …  
    7878{
    7979#if ENABLE(REMOTE_INSPECTOR)
    80     if (const char* address = g_getenv("WEBKIT_INSPECTOR_SERVER")) {
    81         if (!initializeRemoteInspectorServer(address))
    82             g_unsetenv("WEBKIT_INSPECTOR_SERVER");
    83     }
     80    if (const char* address = g_getenv("WEBKIT_INSPECTOR_SERVER"))
     81        initializeRemoteInspectorServer(address);
    8482#endif
    8583

  • trunk/Source/WebKit/UIProcess/wpe/WebProcessPoolWPE.cpp

    r240437 r241304  
    5252
    5353#if ENABLE(REMOTE_INSPECTOR)
    54 static bool initializeRemoteInspectorServer(const char* address)
     54static void initializeRemoteInspectorServer(const char* address)
    5555{
    5656    if (Inspector::RemoteInspectorServer::singleton().isRunning())
    57         return true;
     57        return;
    5858
    5959    if (!address[0])
    60         return false;
     60        return;
    6161
    6262    GUniquePtr<char> inspectorAddress(g_strdup(address));
    6363    char* portPtr = g_strrstr(inspectorAddress.get(), ":");
    6464    if (!portPtr)
    65         return false;
     65        return;
    6666
    6767    *portPtr = '\0';
    … …  
    6969    guint64 port = g_ascii_strtoull(portPtr, nullptr, 10);
    7070    if (!port)
    71         return false;
     71        return;
    7272
    73     return Inspector::RemoteInspectorServer::singleton().start(inspectorAddress.get(), port);
     73    Inspector::RemoteInspectorServer::singleton().start(inspectorAddress.get(), port);
    7474}
    7575#endif
    … …  
    7878{
    7979#if ENABLE(REMOTE_INSPECTOR)
    80     if (const char* address = g_getenv("WEBKIT_INSPECTOR_SERVER")) {
    81         if (!initializeRemoteInspectorServer(address))
    82             g_unsetenv("WEBKIT_INSPECTOR_SERVER");
    83     }
     80    if (const char* address = g_getenv("WEBKIT_INSPECTOR_SERVER"))
     81        initializeRemoteInspectorServer(address);
    8482#endif
    8583}
Note: See TracChangeset for help on using the changeset viewer.