Bug 88094 – Web Inspector: Add a WebInspectorServer on Linux using the GSocket API for the GTK port

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Anton Obzhirov]

There is a platform dependant implementation missing for WebInspectorServer on Linux platform.
I implemented it using GSocket API. We want to contribute it to WebKit.
Please assign this bug to me and advise me how to proceed further with the contribution.

[Joseph Pecoraro]

Hey Anton. There is a short page detailing best practices for contributing code to WebKit:
<http://www.webkit.org/coding/contributing.html>

If you already have a patch I suppose the next step would be to attach it to this bug, mark
it as a patch, and set the review flag to "?". Some tips before you upload the patch: make
sure you have a ChangeLog and make sure code follows the style guidelines. If the patch
is large consider breaking it up into smaller pieces and upload each patch separately.
When the patches are attached, someone will eventually get around to reviewing it!

It sounds like you're adding this for the GTK port? If that is the case you might want the
patch to get reviewed by both an Inspector reviewer and a GTK reviewer. The GTK port
maintainers would probably want to be aware of this feature getting added and how to
use it.

[Anton Obzhirov]

Yes, it is GTK port. I'll attach the patch on Monday.

[Anton Obzhirov]

Hm, tried to build test dependencies as it's required to run regression tests. Got *** module webkitgtk-testing-dependencies not built due to non-buildable gdk-pixbuf *** [18/18]
*** module webkitgtk-testing-dependencies not built due to non-buildable gtk+ *** [18/18]
*** module webkitgtk-testing-dependencies not built due to non-buildable gnome-icon-theme *** [18/18]
*** the following modules were not built *** [18/18]. Does anybody know what's the problem here? I followed instruction in http://trac.webkit.org/wiki/BuildingGtk and build for Ubuntu 12.4.

[Anton Obzhirov]

Created attachment 148350 [details]
Implementation of remote Web Inspector server platform depended bits for GTK platform

[WebKit Review Bot]

Attachment 148350 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anton Obzhirov]

Created attachment 152986 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 152986 [details]
Patch

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

> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:109
> +    if (!m_readBuffer)
> +        return;

This check seems unnecessary.

> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:121
> +    if (m_client) {
> +        // The client can close the handle, potentially removing the last reference.
> +        RefPtr<SocketStreamHandle> protect(this); 
> +        m_client->didOpenSocketStream(this);
> +    }

Protecting this should not be necessary here, since we are not doing anything else after didOpenSocketStream's called.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:375
> +#if PLATFORM(GTK)
> +    // will read host address from environment variables
> +    if (m_pageGroup->preferences()->developerExtrasEnabled())
> +        WebInspectorServer::shared().listen("", 2999);
> +    else
> +        WebInspectorServer::shared().close();
> +#endif

This doesn't look right... WebSocketServer::platform{Listen,Close} should already be called when appropriate, shouldn't they?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1747
> +#if PLATFORM(GTK)
> +    // will read host address from environment variables
> +    if (m_pageGroup->preferences()->developerExtrasEnabled())
> +        WebInspectorServer::shared().listen("", 2999);
> +    else
> +        WebInspectorServer::shared().close();
> +#endif

Ditto.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:38
> +    if (path == "/") {

So, as I understand it, if the remote side asks for / we want to give it the default page, which is the inspector.html file, but we want any other resource requested to be delivered as is. I don't understand what the meta refresh is about; also, is there a reason why we are not delivering pagelist.json in a special-case like qt?

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:50
> +        if (!url)
> +            url = "http://trac.webkit.org/export/head/trunk/Source/WebCore/inspector/front-end/inspector.html";

Hmm, no, we should use the locally installed file, not the one hosted in webkit.org.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:87
> +        portValue = 2999;

Why 2999? Is it the same one used by the other ports?

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:88
> +        const char* envCStr = getenv("GTKWEBKIT_INSPECTOR_SERVER");

We use WEBKIT_ as our environment variables prefix.

[Anton Obzhirov]

Comment on attachment 152986 [details]
Patch

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

>> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:109
>> +        return;
> 
> This check seems unnecessary.

I'll remove it. By the way what is standard strategy in webkit to deal with failed memory allocations?

>> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:121
>> +    }
> 
> Protecting this should not be necessary here, since we are not doing anything else after didOpenSocketStream's called.

ok

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:375
>> +#endif
> 
> This doesn't look right... WebSocketServer::platform{Listen,Close} should already be called when appropriate, shouldn't they?

I don't think they are called, I need to check. Another reason was to start/stop the server if developer extras enabled/disabled via the menu.

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:38
>> +    if (path == "/") {
> 
> So, as I understand it, if the remote side asks for / we want to give it the default page, which is the inspector.html file, but we want any other resource requested to be delivered as is. I don't understand what the meta refresh is about; also, is there a reason why we are not delivering pagelist.json in a special-case like qt?

Meta refresh is to redirect page to http://trac.webkit.org/export/head/trunk/Source/WebCore/inspector/front-end/inspector.html?ws=ddd.ddd.ddd.ddd:2999/devtools/page/1 if there is only one page. Otherwise it will show the page with the list of links to 
http://trac.webkit.org/export/head/trunk/Source/WebCore/inspector/front-end/inspector.html?ws=ddd.ddd.ddd.ddd:2999/devtools/page/1
...
http://trac.webkit.org/export/head/trunk/Source/WebCore/inspector/front-end/inspector.html?ws=ddd.ddd.ddd.ddd:2999/devtools/page/[n]
.
The idea is to reuse latest web inspector resources from http://trac.webkit.org/export/head/trunk/Source/WebCore/inspector/front-end/. But from your comment below probably it is not a good idea.

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:50
>> +            url = "http://trac.webkit.org/export/head/trunk/Source/WebCore/inspector/front-end/inspector.html";
> 
> Hmm, no, we should use the locally installed file, not the one hosted in webkit.org.

The hosted one should have the latest updates/fixes, shouldn't it?

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:87
>> +        portValue = 2999;
> 
> Why 2999? Is it the same one used by the other ports?

well it was mentioned here as an example : http://www.webkit.org/blog/1620/webkit-remote-debugging/.
Can be any value I suppose.

[Gustavo Noronha (kov)]

(In reply to comment #8)
> > This check seems unnecessary.
> 
> I'll remove it. By the way what is standard strategy in webkit to deal with failed memory allocations?

The only sane way to deal with it is to abort I think.

> > This doesn't look right... WebSocketServer::platform{Listen,Close} should already be called when appropriate, shouldn't they?
> 
> I don't think they are called, I need to check. Another reason was to start/stop the server if developer extras enabled/disabled via the menu.

If that's the case, we should probably be calling the WebSocketServer functions rather than doing work here, and there should be no reason to do it only for a port, I think.
 
> >> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:50
> >> +            url = "http://trac.webkit.org/export/head/trunk/Source/WebCore/inspector/front-end/inspector.html";
> > 
> > Hmm, no, we should use the locally installed file, not the one hosted in webkit.org.
> 
> The hosted one should have the latest updates/fixes, shouldn't it?

Yep. And it may be incompatible with the Inspector that is built in the library. Also, we don't want all users of WebKitGTK+ hitting a WebKit development server. And we don't want to require that the developer has an internet connection available for using the inspector. I can only see disadvantages for both the WebKit project and Inspector users.
 
> >> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:87
> >> +        portValue = 2999;
> > 
> > Why 2999? Is it the same one used by the other ports?
> 
> well it was mentioned here as an example : http://www.webkit.org/blog/1620/webkit-remote-debugging/.
> Can be any value I suppose.

That sounds OK then. I hope we'll also add some API for starting the server, so a browser can do that even if it wasn't started with the env variable =).

[Anton Obzhirov]

Created attachment 157749 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 157749 [details]
Patch

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

Needs work still. I think I now understand better how starting and closing the server is supposed to work after reading more of the Qt code, so could make more useful suggestions.

> Source/WebCore/inspector/front-end/inspectorPageIndex.html:1
> +<!DOCTYPE html>

This is the same page qt uses as far as I understand, we should share it with qt somehow, either by moving it here (and thus removing qt's own copy) or by placing it somewhere under Source/WebKit2/UIProcess/InpsectorServer, let's not have a duplicate though.

> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:116
> +    if (m_client) {

Make this an early return :

if (!m_client)
    return;

> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:118
> +        // The client can close the handle, potentially removing the last reference.
> +        RefPtr<SocketStreamHandle> protect(this); 

My comment about this not being needed still stands =)

> Source/WebKit2/ChangeLog:14
> +        (for example using 192.168.124.130:2999 link).

I'd remove this parens, they are already implied by the rest of the description.

> Source/WebKit2/ChangeLog:16
> +        It is not possible to test the change automatically, so all testing has been done manually.

Qt has API tests, we can probably add a couple to our own tests, here: http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/gtk/tests

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:87
> +    start();    

See bellow.

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:98
> +    if (m_clientMap.isEmpty()) // stop the server if no pages left, the server will be restarted if new page is registered.
> +        close();

See bellow (regarding start).

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.h:4
> + * Copyright (C) 2012 Samsung Electronics Ltd. All Rights Reserved. 

Doesn't look like a substation enough change to warrant this.

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.cpp:4
> + * Copyright (C) 2012 Samsung Electronics Ltd. All Rights Reserved. 

Ditto, but see bellow.

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.cpp:94
> +void WebSocketServer::start()
> +{
> +    if (m_state == Listening)
> +        return;
> +
> +    platformGetIPAddress(m_bindAddress, m_port);
> +    listen(m_bindAddress, m_port);
> +}
> +

I don't think we should have something like this here. This kind of logic should go on the platform-specific implementation or be performed by the user agent. We should have public API in the future to start and close down the server, for now I think we should use Qt's approach of initializing the server as part of the WebContext initialization, using the variable, like you did. See: http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/qt/QtWebContext.cpp#L48

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.h:4
> + * Copyright (C) 2012 Samsung Electronics Ltd. All Rights Reserved. 

Ditto.

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.h:43
> +#include <glib.h>

Should not need glib.h here from what I've seen.

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:4
> + * Copyright (C) 2012 Samsung Electronics Ltd. All Rights Reserved.

Ditto.

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:129
> +    // To prevent double deletion in WebSocketServer::didCloseWebSocketServerConnection()
> +    if (!m_socket)
> +        return;

Is this triggered by something this patch does currently, like closing the socket when the last page is unregistered?

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:38
> +#include "FileSystem.h"
> +#include "WebInspectorProxy.h"
> +#include "WebPageProxy.h"
> +#include <WebCore/MIMETypeRegistry.h>
> +
> +#include <gio/gio.h>
> +#include <glib.h>
> +
> +#include <wtf/gobject/GOwnPtr.h>
> +#include <wtf/text/CString.h>
> +#include <wtf/text/StringBuilder.h>

There should be no empty lines here, and it looks like the sorting is not right (<g...> should go before <WebCore...>)

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:120
> +void getIPAddress(String& ipAddress)

I think we should always default to 127.0.0.1 if no explicit IP is given, opening up the server to a public IP would be quite bad, so I'd remove this function. If you need for your particular use-case you can add this kind of logic to the browser afterwards, or to a wrapper that starts the browser with the environment variable filled.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:147
> +void WebSocketServer::platformGetIPAddress(String& bindAddress, unsigned short& port)

Ditto.

> Source/WebKit2/UIProcess/InspectorServer/qt/WebSocketServerQt.cpp:52
> +void WebSocketServer::platformGetIPAddress(String&, unsigned short&)
> +{
> +}
> +

Ditto, thus.

[Anton Obzhirov]

Comment on attachment 157749 [details]
Patch

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

>> Source/WebCore/inspector/front-end/inspectorPageIndex.html:1
>> +<!DOCTYPE html>
> 
> This is the same page qt uses as far as I understand, we should share it with qt somehow, either by moving it here (and thus removing qt's own copy) or by placing it somewhere under Source/WebKit2/UIProcess/InpsectorServer, let's not have a duplicate though.

I think it should be in Source/WebCore/Inspector/front-end/inspector as it's part of front-end (kind of). I will remove qt's copy.

>> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:118
>> +        RefPtr<SocketStreamHandle> protect(this); 
> 
> My comment about this not being needed still stands =)

Forgot to remove :)

>> Source/WebKit2/ChangeLog:16
>> +        It is not possible to test the change automatically, so all testing has been done manually.
> 
> Qt has API tests, we can probably add a couple to our own tests, here: http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/gtk/tests

OK, will check how it is done in QT port. Not entirely sure yet how to test it.

>> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.h:4
>> + * Copyright (C) 2012 Samsung Electronics Ltd. All Rights Reserved. 
> 
> Doesn't look like a substation enough change to warrant this.

Is it about if change is less than 10 lines?

>> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.cpp:94
>> +
> 
> I don't think we should have something like this here. This kind of logic should go on the platform-specific implementation or be performed by the user agent. We should have public API in the future to start and close down the server, for now I think we should use Qt's approach of initializing the server as part of the WebContext initialization, using the variable, like you did. See: http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/qt/QtWebContext.cpp#L48

Will undo my change here, hopefully there will be more generic approach in the future.

>> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:129
>> +        return;
> 
> Is this triggered by something this patch does currently, like closing the socket when the last page is unregistered?

No as far as I remember, will double check anyway.

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:120
>> +void getIPAddress(String& ipAddress)
> 
> I think we should always default to 127.0.0.1 if no explicit IP is given, opening up the server to a public IP would be quite bad, so I'd remove this function. If you need for your particular use-case you can add this kind of logic to the browser afterwards, or to a wrapper that starts the browser with the environment variable filled.

That was one of your comments that it would be good to get IP address somehow :). Don't really need it, so will remove it anyway.

[Anton Obzhirov]

Comment on attachment 157749 [details]
Patch

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

>>> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.cpp:94
>>> +
>> 
>> I don't think we should have something like this here. This kind of logic should go on the platform-specific implementation or be performed by the user agent. We should have public API in the future to start and close down the server, for now I think we should use Qt's approach of initializing the server as part of the WebContext initialization, using the variable, like you did. See: http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/qt/QtWebContext.cpp#L48
> 
> Will undo my change here, hopefully there will be more generic approach in the future.

I found one place which should be good for the server initialization: void WebContext::platformInitializeWebProcess(WebProcessCreationParameters&) in WebContextGtk.cpp.
However with this approach the server will start up regardless if developer extras is enabled or not. (The same for QT port). That doesn't seem to be correct. That's why
in one of my previous patches I started the server in WebPageProxy::initializeWebPage() after checking developer extras. What do you think?

[Gustavo Noronha (kov)]

(In reply to comment #12)
> > I think we should always default to 127.0.0.1 if no explicit IP is given, opening up the server to a public IP would be quite bad, so I'd remove this function. If you need for your particular use-case you can add this kind of logic to the browser afterwards, or to a wrapper that starts the browser with the environment variable filled.
> 
> That was one of your comments that it would be good to get IP address somehow :). Don't really need it, so will remove it anyway.

I couldn't find where I said this, maybe I said something misleading, but I didn't mean that.

(In reply to comment #13)
> > Will undo my change here, hopefully there will be more generic approach in the future.
> 
> I found one place which should be good for the server initialization: void WebContext::platformInitializeWebProcess(WebProcessCreationParameters&) in WebContextGtk.cpp.
> However with this approach the server will start up regardless if developer extras is enabled or not. (The same for QT port). That doesn't seem to be correct. That's why
> in one of my previous patches I started the server in WebPageProxy::initializeWebPage() after checking developer extras. What do you think?

I think we should just do what Qt does, for now; if the developer has used the environment variable, then it's safe to assume they want it, even without checking the developer extras setting. When we add API to open up the server in the future we can think about how it will interact with developer extras.

[Anton Obzhirov]

Created attachment 163316 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 163316 [details]
Patch

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

I don't know much about the inspector nor the inspector server, setting r- because of the memory leaks, styles issues, etc.

> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:103
> +    m_socketConnection = adoptGRef(socketConnection);

You should not adopt the ref, but take a reference.

> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:116
> +    m_outputStream = G_POLLABLE_OUTPUT_STREAM(g_io_stream_get_output_stream(G_IO_STREAM(m_socketConnection.get())));
> +    m_inputStream = g_io_stream_get_input_stream(G_IO_STREAM(m_socketConnection.get()));
> +    m_readBuffer = new char[READ_BUFFER_SIZE];
> +
> +    g_input_stream_read_async(m_inputStream.get(), m_readBuffer, READ_BUFFER_SIZE, G_PRIORITY_DEFAULT, 0,
> +        reinterpret_cast<GAsyncReadyCallback>(readReadyCallback), m_id);
> +
> +    m_state = Open;
> +
> +    if (!m_client)
> +        return;
> +
> +    m_client->didOpenSocketStream(this);

Instead of duplicating all this code, since the socket is already connected this could be just:

m_id = activateHandle(this);
connected(socketConnection, 0);

For this to work, we would also need to fix connectedCallback() to use GRefPtr and adopGRef for the socket connection (note that it's leaked in case of error), and take a ref instead of adopting it in connected().

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:51
> +    GRefPtr<GFile> file(g_file_new_for_path(localPath.latin1().data()));

Use adoptGRef() to avoid leaking the GFile. GFile expects UTF-8 so you should use localPath.utf8().data() instead of latin1

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:54
> +    if (file) {

g_file_new always returns a valid pointer, so you don't need this check

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:56
> +        GRefPtr<GFileInfo> fileInfo(g_file_query_info(file.get(), G_FILE_ATTRIBUTE_STANDARD_SIZE, G_FILE_QUERY_INFO_NONE, NULL, &error.outPtr()));

Use adoptGRef here too. Use 0 instead of NULL. Also since you are not handling the error, you could pass 0, and check the return value, returning if fileInfo is NULL.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:61
> +        GRefPtr<GFileInputStream> inputStream(g_file_read(file.get(), NULL, &error.outPtr()));

You should use adopGRef here too, use 0 instad of NULL, and pass 0 for the error too since you are not handling the error.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:66
> +        size = g_file_info_get_size(fileInfo.get());
> +        data.grow(size);

This could be a single line:

data.grow(g_file_info_get_size(fileInfo.get()));

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:70
> +        gsize bytesRead = 0;
> +        if (!g_input_stream_read_all(G_INPUT_STREAM(inputStream.get()), data.data(), data.size(), &bytesRead, 0, 0))
> +            return false;

bytesRead is unused, you could simply pass NULL to g_input_stream_read_all

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:74
> +        size_t extStart = localPath.reverseFind('.');
> +        String ext = localPath.substring(extStart != notFound ? extStart + 1 : 0);
> +        contentType = WebCore::MIMETypeRegistry::getMIMETypeForExtension(ext);

Instead of this, you could pass G_FILE_ATTRIBUTE_STANDARD_FAST_CONTENT_TYPE to g_file_query_info and get the mime type from the GFileInfo with g_file_info_get_content_type().

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:55
> +    WebSocketServer* server = (WebSocketServer*)self;

Use C++ style casts. In this case we can simply avoid the cast, since the callback is already "casted", you can pass WebSocketServer* server as parameter instead of gpointer self

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:57
> +    // reference connection before accepting it
> +    g_object_ref_sink(connection);

Don't do this, pass the connection to SocketStreamHandle::create() and the constructor will tak a reference.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:69
> +    m_socketService = adoptGRef(g_socket_service_new());
> +    if (m_socketService.get()) {

g_socket_service_new() always returns a valid pointer.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:72
> +        if (!g_socket_service_is_active(m_socketService.get()))
> +            g_socket_service_start(m_socketService.get());

g_socket_service_start already checks whether the service is active or not, so you can call g_socket_service_start(m_socketService.get()); directly.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:86
> +    GSocketService* socketService = m_socketService.get();
> +    if (!socketService) {
> +        LOG_ERROR("Failed to get socket service.");
> +        return false;
> +    }

I think this shouldn't happen, if you want to be extra sure, add an assert ASSERT(socketService);

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:89
> +    GInetAddress* address = g_inet_address_new_from_string(bindAddress.latin1().data());
> +    GSocketAddress* socketAddress = g_inet_socket_address_new(address, port);

Use GRefPtr and adoptGRef in both cases. Use bindAddress.utf8().data() instead of latin1 too.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:105
> +    gboolean result = g_socket_listener_add_address(G_SOCKET_LISTENER(socketService),
> +                                  socketAddress,
> +                                  G_SOCKET_TYPE_STREAM,
> +                                  G_SOCKET_PROTOCOL_TCP, 0, 0, 0);
> +
> +    if (!result)
> +         LOG_ERROR("Failed to add inet address to web inspector server listener.");
> +
> +    g_object_unref(socketAddress);
> +    g_object_unref(address);
> +
> +    if (!result)
> +        return false;
> +
> +    return true;

All this could be just 

return g_socket_listener_add_address(G_SOCKET_LISTENER(m_socketService.get()), socketAddress.get(), G_SOCKET_TYPE_STREAM, G_SOCKET_PROTOCOL_TCP, 0, 0, 0);

> Source/WebKit2/UIProcess/InspectorServer/qt/WebInspectorServerQt.cpp:50
> -    String localPath = (path == "/") ? "/webkit/resources/inspectorPageIndex.html" : path;
> +    String localPath = (path == "/") ? "/webkit/inspector/inspectorPageIndex.html" : path;

Is this a bug in Qt implementation or something that has changed in this patch?

> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:66
> +    initialized = WebInspectorServer::shared().listen(bindAddress, port);

Do we want this even if the env variable is not present?

> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:78
> +    initInspectorServer();

How is the inspector server run? does it run in the UI process?

[Anton Obzhirov]

Comment on attachment 163316 [details]
Patch

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

>> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:116
>> +    m_client->didOpenSocketStream(this);
> 
> Instead of duplicating all this code, since the socket is already connected this could be just:
> 
> m_id = activateHandle(this);
> connected(socketConnection, 0);
> 
> For this to work, we would also need to fix connectedCallback() to use GRefPtr and adopGRef for the socket connection (note that it's leaked in case of error), and take a ref instead of adopting it in connected().

Yep, I realized I should have call connected() here. Original version of this function before few patches was a bit different. :)

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:57
>> +    g_object_ref_sink(connection);
> 
> Don't do this, pass the connection to SocketStreamHandle::create() and the constructor will tak a reference.

OK, makes sense which changes above,

>> Source/WebKit2/UIProcess/InspectorServer/qt/WebInspectorServerQt.cpp:50
>> +    String localPath = (path == "/") ? "/webkit/inspector/inspectorPageIndex.html" : path;
> 
> Is this a bug in Qt implementation or something that has changed in this patch?

GTK and QT ports will share inspectorPageIndex.html. I moved the file in generic location (see previous reviews)

>> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:66
>> +    initialized = WebInspectorServer::shared().listen(bindAddress, port);
> 
> Do we want this even if the env variable is not present?

I think so. It will be bound to 127.0.0.1 by default. It basically copies QT port behavior here.

.

>> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:78
>> +    initInspectorServer();
> 
> How is the inspector server run? does it run in the UI process?

Yes, as a part of GTK main loop in UI process.

[Martin Robinson]

Comment on attachment 163316 [details]
Patch

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

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:51
>> +    GRefPtr<GFile> file(g_file_new_for_path(localPath.latin1().data()));
> 
> Use adoptGRef() to avoid leaking the GFile. GFile expects UTF-8 so you should use localPath.utf8().data() instead of latin1

This is an awesome review, but I just thought it'd be good to point out here that GFile accepts the "glib filename encoding" which isn't necessarily UTF-8. Instead you should use fileSystemRepresentation from FileSystemGtk.cpp to convert from a String to a CString.

[Carlos Garcia Campos]

(In reply to comment #18)
> >> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:66
> >> +    initialized = WebInspectorServer::shared().listen(bindAddress, port);
> > 
> > Do we want this even if the env variable is not present?
> 
> I think so. It will be bound to 127.0.0.1 by default. It basically copies QT port behavior here.

hmm, in Source/WebKit2/UIProcess/qt/QtWebContext.cpp, function initInspectorServer() there's

QString inspectorEnv = QString::fromUtf8(qgetenv("QTWEBKIT_INSPECTOR_SERVER"));
    if (!inspectorEnv.isEmpty()) {
    }

So if the env var si not present it does nothing.

[Carlos Garcia Campos]

(In reply to comment #19)
> (From update of attachment 163316 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163316&action=review
> 
> >> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:51
> >> +    GRefPtr<GFile> file(g_file_new_for_path(localPath.latin1().data()));
> > 
> > Use adoptGRef() to avoid leaking the GFile. GFile expects UTF-8 so you should use localPath.utf8().data() instead of latin1
> 
> This is an awesome review, but I just thought it'd be good to point out here that GFile accepts the "glib filename encoding" which isn't necessarily UTF-8. Instead you should use fileSystemRepresentation from FileSystemGtk.cpp to convert from a String to a CString.

You are right.

[Anton Obzhirov]

(In reply to comment #20)
> (In reply to comment #18)
> > >> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:66
> > >> +    initialized = WebInspectorServer::shared().listen(bindAddress, port);
> > > 
> > > Do we want this even if the env variable is not present?
> > 
> > I think so. It will be bound to 127.0.0.1 by default. It basically copies QT port behavior here.
> 
> hmm, in Source/WebKit2/UIProcess/qt/QtWebContext.cpp, function initInspectorServer() there's
> 
> QString inspectorEnv = QString::fromUtf8(qgetenv("QTWEBKIT_INSPECTOR_SERVER"));
>     if (!inspectorEnv.isEmpty()) {
>     }
> 
> So if the env var si not present it does nothing.
Yes, you are right. Just checked myself. Will change it.

[Anton Obzhirov]

Created attachment 163880 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 163880 [details]
Patch

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

OK, we're getting there. I'm worried about the changes to WebSocketServerConnection, though.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:86
> +#if (!GLIB_CHECK_VERSION(2, 31, 0))
> +    g_thread_create(testServerMonitorThreadFunc, 0, FALSE, 0);
> +#else
> +    g_thread_new("TestServerMonitor", testServerMonitorThreadFunc, 0);

Do we really need to use a thread here? Wouldn't a glib timeout do?

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:96
> +    // Save current WEBKIT_INSPECTOR_SERVER variable to restore it after the test is finished.
> +    s_inspectorServerEnv = g_getenv("WEBKIT_INSPECTOR_SERVER");

You shouldn't need to do this, setting the variable for your own process will not affect the parents.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:98
> +    // Overwrite WEBKIT_INSPECTOR_SERVER variable with default value.

No need to add "what" comments when the line of code is already pretty explicit, I think you can cut back most of the comments.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:116
> +    if (!g_spawn_async_with_pipes(0, testServerArgv, 0, static_cast<GSpawnFlags>(0), 0, 0,
> +                                  &s_childProcessPid, 0, &childStdout, 0, 0)) {
> +        close(childStdout);
> +        return;
> +    }

I believe this should be an assert instead of a simple check; if the spawn fails all bets are off.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:119
> +    // Start monitoring the test server (in a separate thread) to
> +    // ensure we don't block on the child process more than a timeout.

I don't understand why you'd block on the child process using  glib timeout on the main thread.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:128
> +    GIOChannel* ioChannel = g_io_channel_unix_new(childStdout);
> +    if (g_io_channel_read_chars(ioChannel, msg, 2, 0, 0) == G_IO_STATUS_NORMAL) {
> +        // Check whether the server sent a message saying it's ready
> +        // and store the result globally, so the monitor can see it.
> +        s_childIsReady = msg[0] == 'O' && msg[1] == 'K';
> +    }

Guess because you're blocking here? You should just need to g_io_add_watch here instead.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:239
> +    // debugging session was established correctly through web socket. It should be something like "Web Inspector - <Page URL>".

How about we assert g_str_has_prefix instead?

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:84
>  {
> -    // If this ASSERT happens on any platform then their SocketStreamHandle::send
> -    // followed by a SocketStreamHandle::close is not guarenteed to have sent all
> -    // data. If this happens, we need to slightly change the design to include a
> -    // SocketStreamHandleClient::didSend, handle it here, and add an m_shutdownAfterSend
> -    // state on this WebSocketServerConnection.
> +    if (m_socket->bufferedAmount()) {
> +        m_shutdownAfterSend = true;
> +        return;
> +    }
> +
>      ASSERT(!m_socket->bufferedAmount());

This doesn't look right. The assert is now useless... it may make sense to drop it, but then we should drop it and implement what's descried in the comment, not just make it useless =). Might be useful to get the developer who wrote it (git blame should tell you) what they think about this.

[Carlos Garcia Campos]

Comment on attachment 163880 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:86
>> +    g_thread_new("TestServerMonitor", testServerMonitorThreadFunc, 0);
> 
> Do we really need to use a thread here? Wouldn't a glib timeout do?

I think the reasoning is the same as in bug #72589, see the discussion there.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:40
> +bool WebInspectorServer::platformResourceForPath(const String& path, Vector<char>& data, String& contentType)

How often is this method called? I don't like all these synchronous API blocking the UI process, but the method is synchronous so I don't see any other solution.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:58
> +    GRefPtr<GFileInputStream> inputStream = adoptGRef(g_file_read(file.get(), NULL, 0));

Use 0 instead of NULL.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:66
> +    contentType = GOwnPtr<gchar>(g_file_info_get_attribute_as_string(fileInfo.get(), G_FILE_ATTRIBUTE_STANDARD_FAST_CONTENT_TYPE)).get();

You can use g_file_info_get_content_type() which returns a const char *, if what contentType expects is actually the mime type, you might need to call g_content_type_get_mime_type() to get the mime type from the content type.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:44
> +static gboolean connectionCallback(GSocketService* service,
> +                                   GSocketConnection* connection,
> +                                   GObject* sourceObject,
> +                                   WebSocketServer* server)

This should be a single line.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:47
> +    GSocketAddress* sockaddr = g_socket_connection_get_remote_address(connection, NULL);

Use GRefPtr and adoptGRef. Use also 0 instead of NULL.

> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:61
> +        initialized = WebInspectorServer::shared().listen(bindAddress, port);

initialized is to ensure initInspectorServer() is executed only once, so it should set to true always, even if the env var is not present or the server fails to run.

[Jocelyn Turcotte]

Comment on attachment 163880 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        * inspector/front-end/inspectorPageIndex.html: Added.

I don't think it belongs in WebCore, the inspector server is contained in WebKit2, so the resource should go there as well.
Source/WebKit2/UIProcess/InspectorServer/front-end would feel like a better place to me, but that means that ports might have to include it in a different resource bundle.

> Source/WebKit2/config.h:122
>  #if PLATFORM(QT)
>  #define ENABLE_INSPECTOR_SERVER 1
> +#elif PLATFORM(GTK)
> +#define ENABLE_INSPECTOR_SERVER 1

#if PLATFORM(QT) || PLATFORM(GTK)

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:130
> +    // To prevent double deletion in WebSocketServer::didCloseWebSocketServerConnection()
> +    if (!m_socket)
> +        return;

Can you elaborate on this? If in any case didCloseWebSocketServerConnection isn't run, this would prevent the server from accepting a new connection if the previous connection wasn't cleaned-up properly.

[Kenneth Rohde Christiansen]

Comment on attachment 163880 [details]
Patch

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

Is the intent to make this work for EFL later?

> Source/WebCore/ChangeLog:11
> +        inspectorPageIndex.html has been moved to new location to share the file between GTK and QT ports. 

Qt*

> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:45
> +    String envStr(getenv("WEBKIT_INSPECTOR_SERVER"));

g_getenv?

[Anton Obzhirov]

Comment on attachment 163880 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        * inspector/front-end/inspectorPageIndex.html: Added.
> 
> I don't think it belongs in WebCore, the inspector server is contained in WebKit2, so the resource should go there as well.
> Source/WebKit2/UIProcess/InspectorServer/front-end would feel like a better place to me, but that means that ports might have to include it in a different resource bundle.

OK, I will move it to Source/WebKit2/UIProcess/InspectorServer/front-end and update resource path if needed.

>> Source/WebKit2/config.h:122
>> +#define ENABLE_INSPECTOR_SERVER 1
> 
> #if PLATFORM(QT) || PLATFORM(GTK)

ok

>>> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:86
>>> +    g_thread_new("TestServerMonitor", testServerMonitorThreadFunc, 0);
>> 
>> Do we really need to use a thread here? Wouldn't a glib timeout do?
> 
> I think the reasoning is the same as in bug #72589, see the discussion there.

Check as well implementation of TestWebkitAccessability.cpp /WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp

>> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:96
>> +    s_inspectorServerEnv = g_getenv("WEBKIT_INSPECTOR_SERVER");
> 
> You shouldn't need to do this, setting the variable for your own process will not affect the parents.

yep, forget about this.

>> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:98
>> +    // Overwrite WEBKIT_INSPECTOR_SERVER variable with default value.
> 
> No need to add "what" comments when the line of code is already pretty explicit, I think you can cut back most of the comments.

there is no harm in additional comments I suppose plus I copied this code anyway from TestWebkitAccessability.cpp with all these comments :)

>> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:116
>> +    }
> 
> I believe this should be an assert instead of a simple check; if the spawn fails all bets are off.

ok

>> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:128
>> +    }
> 
> Guess because you're blocking here? You should just need to g_io_add_watch here instead.

ok will check

>> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:239
>> +    // debugging session was established correctly through web socket. It should be something like "Web Inspector - <Page URL>".
> 
> How about we assert g_str_has_prefix instead?

hm, probably you are right. What if some changes title format in the future?

>> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:84
>>      ASSERT(!m_socket->bufferedAmount());
> 
> This doesn't look right. The assert is now useless... it may make sense to drop it, but then we should drop it and implement what's descried in the comment, not just make it useless =). Might be useful to get the developer who wrote it (git blame should tell you) what they think about this.

yes, assert shouldn't be here any more. I implemented what was described in the comment but instead of adding new callback in SocketStreamHandleClient
I used existing didUpdateBufferedAmount which basically means didSend when bufferedAmount is 0.

>> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:130
>> +        return;
> 
> Can you elaborate on this? If in any case didCloseWebSocketServerConnection isn't run, this would prevent the server from accepting a new connection if the previous connection wasn't cleaned-up properly.

I think if m_socket is 0 didCloseWebSocketServerConnection has been already called, I will attach call stack to show why double deletion happens.

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:40
>> +bool WebInspectorServer::platformResourceForPath(const String& path, Vector<char>& data, String& contentType)
> 
> How often is this method called? I don't like all these synchronous API blocking the UI process, but the method is synchronous so I don't see any other solution.

Whenever inspector page on remote host sends resource request. Shouldn't happen often, didn't notice slow down because of that.

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:58
>> +    GRefPtr<GFileInputStream> inputStream = adoptGRef(g_file_read(file.get(), NULL, 0));
> 
> Use 0 instead of NULL.

ok

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:66
>> +    contentType = GOwnPtr<gchar>(g_file_info_get_attribute_as_string(fileInfo.get(), G_FILE_ATTRIBUTE_STANDARD_FAST_CONTENT_TYPE)).get();
> 
> You can use g_file_info_get_content_type() which returns a const char *, if what contentType expects is actually the mime type, you might need to call g_content_type_get_mime_type() to get the mime type from the content type.

g_file_info_get_content_type() returns NULL because G_FILE_ATTRIBUTE_STANDARD_CONTENT_TYPE attribute is not set. I can use G_FILE_ATTRIBUTE_STANDARD_CONTENT_TYPE
in attributes string but it will probably slow down getting file info a bit.

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:44
>> +                                   WebSocketServer* server)
> 
> This should be a single line.

ok

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:47
>> +    GSocketAddress* sockaddr = g_socket_connection_get_remote_address(connection, NULL);
> 
> Use GRefPtr and adoptGRef. Use also 0 instead of NULL.

ok

>> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:45
>> +    String envStr(getenv("WEBKIT_INSPECTOR_SERVER"));
> 
> g_getenv?

ok

>> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:61
>> +        initialized = WebInspectorServer::shared().listen(bindAddress, port);
> 
> initialized is to ensure initInspectorServer() is executed only once, so it should set to true always, even if the env var is not present or the server fails to run.

ok

[Anton Obzhirov]

Comment on attachment 163880 [details]
Patch

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

>>> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:130
>>> +        return;
>> 
>> Can you elaborate on this? If in any case didCloseWebSocketServerConnection isn't run, this would prevent the server from accepting a new connection if the previous connection wasn't cleaned-up properly.
> 
> I think if m_socket is 0 didCloseWebSocketServerConnection has been already called, I will attach call stack to show why double deletion happens.

The call stack:
0	__GI_raise	raise.c	64	0x7ffff283f445	
1	__GI_abort	abort.c	91	0x7ffff2842bab	
2	__libc_message	libc_fatal.c	201	0x7ffff287ce2e	
3	malloc_printerr	malloc.c	5007	0x7ffff2887626	
4	WebKit::WebSocketServerConnection::~WebSocketServerConnection	WebSocketServerConnection.cpp	60	0x7ffff43e590e	
5	WTF::deleteOwnedPtr<WebKit::WebSocketServerConnection>	OwnPtrCommon.h	56	0x7ffff43e57ec	
6	WTF::OwnPtr<WebKit::WebSocketServerConnection>::~OwnPtr	OwnPtr.h	55	0x7ffff43e5783	
7	WTF::VectorDestructor<true, WTF::OwnPtr<WebKit::WebSocketServerConnection> >::destruct	Vector.h	59	0x7ffff43e563f	
8	WTF::VectorTypeOperations<WTF::OwnPtr<WebKit::WebSocketServerConnection> >::destruct	Vector.h	221	0x7ffff43e5181	
9	WTF::Deque<WTF::OwnPtr<WebKit::WebSocketServerConnection>, 0ul>::remove	Deque.h	516	0x7ffff43e4fbd	
10	WTF::Deque<WTF::OwnPtr<WebKit::WebSocketServerConnection>, 0ul>::remove	Deque.h	496	0x7ffff43e45d1	
11	WebKit::WebSocketServer::didCloseWebSocketServerConnection	WebSocketServer.cpp	95	0x7ffff43e4267	
12	WebKit::WebSocketServerConnection::didCloseSocketStream	WebSocketServerConnection.cpp	137	0x7ffff43e5ef1	
13	WebCore::SocketStreamHandle::platformClose	SocketStreamHandleSoup.cpp	205	0x7ffff4e5477c	
14	WebCore::SocketStreamHandleBase::disconnect	SocketStreamHandleBase.cpp	98	0x7ffff4e48c82	
15	WebCore::SocketStreamHandleBase::close	SocketStreamHandleBase.cpp	91	0x7ffff4e48c44	
16	WebCore::SocketStreamHandle::readBytes	SocketStreamHandleSoup.cpp	140	0x7ffff4e54327	
17	WebCore::readReadyCallback	SocketStreamHandleSoup.cpp	274	0x7ffff4e54bf3	
18	async_ready_callback_wrapper	ginputstream.c	529	0x7ffff33b1775	
19	g_simple_async_result_complete	gsimpleasyncresult.c	767	0x7ffff33c68dd	
20	complete_in_idle_cb	gsimpleasyncresult.c	779	0x7ffff33c6a0c	
...	<More>

SocketStreamHandle instance gets deleted as a part of WebSocketServerConnection instance. if I protect SocketStreamHandle before close() call the issue seems to be fixed,
however there another crush might occur. Will investigate it further and fix after my 2 weeks trip.


void SocketStreamHandle::readBytes(signed long bytesRead, GError* error)
{
    if (error) {
        m_client->didFailSocketStream(this, SocketStreamError(error->code, error->message));
        return;
    }

+   RefPtr<SocketStreamHandle> protect(this);
    if (!bytesRead) {
        close();
        return;
    }

    // The client can close the handle, potentially removing the last reference.
-   RefPtr<SocketStreamHandle> protect(this);

[Jocelyn Turcotte]

Comment on attachment 163880 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:130
>>>> +        return;
>>> 
>>> Can you elaborate on this? If in any case didCloseWebSocketServerConnection isn't run, this would prevent the server from accepting a new connection if the previous connection wasn't cleaned-up properly.
>> 
>> I think if m_socket is 0 didCloseWebSocketServerConnection has been already called, I will attach call stack to show why double deletion happens.
> 
> The call stack:
> 0    __GI_raise    raise.c    64    0x7ffff283f445    
> 1    __GI_abort    abort.c    91    0x7ffff2842bab    
> 2    __libc_message    libc_fatal.c    201    0x7ffff287ce2e    
> 3    malloc_printerr    malloc.c    5007    0x7ffff2887626    
> 4    WebKit::WebSocketServerConnection::~WebSocketServerConnection    WebSocketServerConnection.cpp    60    0x7ffff43e590e    
> 5    WTF::deleteOwnedPtr<WebKit::WebSocketServerConnection>    OwnPtrCommon.h    56    0x7ffff43e57ec    
> 6    WTF::OwnPtr<WebKit::WebSocketServerConnection>::~OwnPtr    OwnPtr.h    55    0x7ffff43e5783    
> 7    WTF::VectorDestructor<true, WTF::OwnPtr<WebKit::WebSocketServerConnection> >::destruct    Vector.h    59    0x7ffff43e563f    
> 8    WTF::VectorTypeOperations<WTF::OwnPtr<WebKit::WebSocketServerConnection> >::destruct    Vector.h    221    0x7ffff43e5181    
> 9    WTF::Deque<WTF::OwnPtr<WebKit::WebSocketServerConnection>, 0ul>::remove    Deque.h    516    0x7ffff43e4fbd    
> 10    WTF::Deque<WTF::OwnPtr<WebKit::WebSocketServerConnection>, 0ul>::remove    Deque.h    496    0x7ffff43e45d1    
> 11    WebKit::WebSocketServer::didCloseWebSocketServerConnection    WebSocketServer.cpp    95    0x7ffff43e4267    
> 12    WebKit::WebSocketServerConnection::didCloseSocketStream    WebSocketServerConnection.cpp    137    0x7ffff43e5ef1    
> 13    WebCore::SocketStreamHandle::platformClose    SocketStreamHandleSoup.cpp    205    0x7ffff4e5477c    
> 14    WebCore::SocketStreamHandleBase::disconnect    SocketStreamHandleBase.cpp    98    0x7ffff4e48c82    
> 15    WebCore::SocketStreamHandleBase::close    SocketStreamHandleBase.cpp    91    0x7ffff4e48c44    
> 16    WebCore::SocketStreamHandle::readBytes    SocketStreamHandleSoup.cpp    140    0x7ffff4e54327    
> 17    WebCore::readReadyCallback    SocketStreamHandleSoup.cpp    274    0x7ffff4e54bf3    
> 18    async_ready_callback_wrapper    ginputstream.c    529    0x7ffff33b1775    
> 19    g_simple_async_result_complete    gsimpleasyncresult.c    767    0x7ffff33c68dd    
> 20    complete_in_idle_cb    gsimpleasyncresult.c    779    0x7ffff33c6a0c    
> ...    <More>
> 
> SocketStreamHandle instance gets deleted as a part of WebSocketServerConnection instance. if I protect SocketStreamHandle before close() call the issue seems to be fixed,
> however there another crush might occur. Will investigate it further and fix after my 2 weeks trip.
> 
> 
> void SocketStreamHandle::readBytes(signed long bytesRead, GError* error)
> {
>     if (error) {
>         m_client->didFailSocketStream(this, SocketStreamError(error->code, error->message));
>         return;
>     }
> 
> +   RefPtr<SocketStreamHandle> protect(this);
>     if (!bytesRead) {
>         close();
>         return;
>     }
> 
>     // The client can close the handle, potentially removing the last reference.
> -   RefPtr<SocketStreamHandle> protect(this);

To me this confirms that WebSocketServer::didCloseWebSocketServerConnection has something to clean up, thus that preventing this call would make it impossible to re-inspect a page after closing a previous browser window.
I remember that the order of destruction was pretty tight around there for Qt as well, and things might have to be adjusted a bit for GTK. Feel free to ping me on IRC if you see the need.

[Jesus Sanchez-Palencia]

Comment on attachment 163880 [details]
Patch

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

> Source/WebKit2/UIProcess/InspectorServer/qt/WebInspectorServerQt.cpp:50
> +    String localPath = (path == "/") ? "/webkit/inspector/inspectorPageIndex.html" : path;

Why are you changing this?

[Anton Obzhirov]

(In reply to comment #31)
> (From update of attachment 163880 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163880&action=review
> 
> > Source/WebKit2/UIProcess/InspectorServer/qt/WebInspectorServerQt.cpp:50
> > +    String localPath = (path == "/") ? "/webkit/inspector/inspectorPageIndex.html" : path;
> 
> Why are you changing this?

I found out that I don't need to do it actually. So this code will be reverted. I want to reuse inspectorPageIndex.html for GTK port.

[Anton Obzhirov]

Created attachment 167539 [details]
Patch

[Yuta Kitamura]

Comment on attachment 167539 [details]
Patch

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

Just added style comments. SocketStream part looks good to me. I'd like to let GTK folks to do the final review.

> Source/WebKit2/ChangeLog:9
> +        The server listens on port 2999 by default. Ip address of the server can be set

Ip -> IP

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.h:64
> +#if (PLATFORM(QT) || PLATFORM(GTK))

Outermost parentheses are not necessary.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:44
> +#ifndef LOG_DISABLED

I think this line should be "#if !LOG_DISABLED".

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:68
> +#ifndef LOG_DISABLED
> +    LOG(Network, "Listen to address=%s, port=%d.", bindAddress.utf8().data(), port);
> +#endif

This #ifndef block is not necessary because LOG() becomes a no-op when LOG_DISABLED is true. See wtf/Assertions.h.

> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:46
> +    String envStr(g_getenv("WEBKIT_INSPECTOR_SERVER"));

We tend to avoid abbreviations like "env" or "Str" and spell the words fully.

Or, as an alternative, "serverAddress" or "serverSpecification" may sound better.

[Jocelyn Turcotte]

Comment on attachment 167539 [details]
Patch

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

> Source/WebKit2/WebKit2.qrc:3
> -    <file alias="inspectorPageIndex.html">qt/Resources/inspectorPageIndex.html</file>
> +    <file alias="inspectorPageIndex.html">UIProcess/InspectorServer/front-end/inspectorPageIndex.html</file>

This patch should also remove the now unused file at Source/WebKit2/qt/Resources/inspectorPageIndex.html

> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:55
> +        Vector<String> result;
> +        envStr.split(":", result);
> +
> +        if (result.size() == 2) {

I don't know how you intend to use WEBKIT_INSPECTOR_SERVER, but if users are going to fill it directly it might be good to give back some error message in cases where you couldn't parse the address/port or if you couldn't open the server.

[Jesus Sanchez-Palencia]

Comment on attachment 167539 [details]
Patch

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

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.h:40
> +#elif PLATFORM(GTK)

Shouldn't this be #elif USE(SOUP), instead? Then both GTK and EFL could benefit from it.
Also, I would move this entire block up, before the #if PLATFORM(QT), since it is related to includes and not namespaces/forward declarations.

[Jesus Sanchez-Palencia]

Comment on attachment 167539 [details]
Patch

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

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:47
> +    LOG(Network, "New Connection from %s:%d.", addressStr.get(), g_inet_socket_address_get_port(G_INET_SOCKET_ADDRESS(socketAddress.get())));

Shouldn't the #ifndef LOG_DISABLED be protecting only this line instead of all the 3 ones above?

[Yuta Kitamura]

Comment on attachment 167539 [details]
Patch

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

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:46
> +    GOwnPtr<gchar> addressStr(g_inet_address_to_string(g_inet_socket_address_get_address(G_INET_SOCKET_ADDRESS(socketAddress.get()))));

Abbreviation "Str" here, too. "addressString" would be fine.

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:47
>> +    LOG(Network, "New Connection from %s:%d.", addressStr.get(), g_inet_socket_address_get_port(G_INET_SOCKET_ADDRESS(socketAddress.get())));
> 
> Shouldn't the #ifndef LOG_DISABLED be protecting only this line instead of all the 3 ones above?

I guess the original code is fine in this case because socketAddress and addressStr are only used within LOG().

[Seokju Kwon]

(In reply to comment #36)
> (From update of attachment 167539 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167539&action=review
> 
> > Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.h:40
> > +#elif PLATFORM(GTK)
> 
> Shouldn't this be #elif USE(SOUP), instead? Then both GTK and EFL could benefit from it.
> Also, I would move this entire block up, before the #if PLATFORM(QT), since it is related to includes and not namespaces/forward declarations.

I want to use 'WebSocketServer' using g_socket_service on efl port.
Should I copy WebSocketServerGtk.cpp to WebSocketServerEfl.cpp? or make a shareable directory for gtk/efl?

[Jesus Sanchez-Palencia]

(In reply to comment #39)
> I want to use 'WebSocketServer' using g_socket_service on efl port.
> Should I copy WebSocketServerGtk.cpp to WebSocketServerEfl.cpp? or make a shareable directory for gtk/efl?

I would go for a shared implementation inside WebKit2/UIProcess/InspectorServer/soup/ (ok, I know it will be GSocket but why not following the convention of WebCore/platform/network/soup), if EFL is gonna use a GSocket based implementation as well.

And, as I've commented on the patch, you will be able to avoid stuff like:
"#elif PLATFORM(GTK) || PLATFORM(EFL)" by using "USE(SOUP)" instead.

In fact, from another glance at the patch, the only thing that might change in between the ports is the implementation of WebInspectorServer::platformResourceForPath .

IMO, all of these could still be fixed on this patch since it is still waiting for review.

[Anton Obzhirov]

Comment on attachment 167539 [details]
Patch

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

>> Source/WebKit2/ChangeLog:9
>> +        The server listens on port 2999 by default. Ip address of the server can be set
> 
> Ip -> IP

ok

>> Source/WebKit2/WebKit2.qrc:3
>> +    <file alias="inspectorPageIndex.html">UIProcess/InspectorServer/front-end/inspectorPageIndex.html</file>
> 
> This patch should also remove the now unused file at Source/WebKit2/qt/Resources/inspectorPageIndex.html

Yep, forgot to remove.

>> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.h:64
>> +#if (PLATFORM(QT) || PLATFORM(GTK))
> 
> Outermost parentheses are not necessary.

ok

>>> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.h:40
>>> +#elif PLATFORM(GTK)
>> 
>> Shouldn't this be #elif USE(SOUP), instead? Then both GTK and EFL could benefit from it.
>> Also, I would move this entire block up, before the #if PLATFORM(QT), since it is related to includes and not namespaces/forward declarations.
> 
> I want to use 'WebSocketServer' using g_socket_service on efl port.
> Should I copy WebSocketServerGtk.cpp to WebSocketServerEfl.cpp? or make a shareable directory for gtk/efl?

Shareable folder should be better. Probably it should be WebSocketServerSoup.cpp in soup folder. I can change it in the next patch.

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:44
>> +#ifndef LOG_DISABLED
> 
> I think this line should be "#if !LOG_DISABLED".

ok

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:46
>> +    GOwnPtr<gchar> addressStr(g_inet_address_to_string(g_inet_socket_address_get_address(G_INET_SOCKET_ADDRESS(socketAddress.get()))));
> 
> Abbreviation "Str" here, too. "addressString" would be fine.

ok

>>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:47
>>> +    LOG(Network, "New Connection from %s:%d.", addressStr.get(), g_inet_socket_address_get_port(G_INET_SOCKET_ADDRESS(socketAddress.get())));
>> 
>> Shouldn't the #ifndef LOG_DISABLED be protecting only this line instead of all the 3 ones above?
> 
> I guess the original code is fine in this case because socketAddress and addressStr are only used within LOG().

Yes, it doesn't make sense to move them out of the block.

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:68
>> +#endif
> 
> This #ifndef block is not necessary because LOG() becomes a no-op when LOG_DISABLED is true. See wtf/Assertions.h.

ok, will check

>> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:46
>> +    String envStr(g_getenv("WEBKIT_INSPECTOR_SERVER"));
> 
> We tend to avoid abbreviations like "env" or "Str" and spell the words fully.
> 
> Or, as an alternative, "serverAddress" or "serverSpecification" may sound better.

ok

>> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:55
>> +        if (result.size() == 2) {
> 
> I don't know how you intend to use WEBKIT_INSPECTOR_SERVER, but if users are going to fill it directly it might be good to give back some error message in cases where you couldn't parse the address/port or if you couldn't open the server.

ok, I will add LOG_ERROR message here.

[Anton Obzhirov]

Hi all, I am going to make new patch in few days, there is one question I want to clarify about refactoring the code to use SOUP compilation flag. I guess I can just follow Comment #40 From Jesus Sanchez-Palencia for that. Does anybody have any additional comments about it?

[Jocelyn Turcotte]

(In reply to comment #42)
> Hi all, I am going to make new patch in few days, there is one question I want to clarify about refactoring the code to use SOUP compilation flag. I guess I can just follow Comment #40 From Jesus Sanchez-Palencia for that. Does anybody have any additional comments about it?

This is just my opinion and not everybody might agree with this, but I think that it would be less wasted reviewing efforts if you apply the modifications needed to land this patch first and then do further tuning in different patches (e.g. for EFL support).
If you refactor this code, it might be difficult to do a review on only what changed.

This patch is already larger than the average and splitting in smaller patches is usually encouraged.

[Anton Obzhirov]

(In reply to comment #43)
> (In reply to comment #42)
> > Hi all, I am going to make new patch in few days, there is one question I want to clarify about refactoring the code to use SOUP compilation flag. I guess I can just follow Comment #40 From Jesus Sanchez-Palencia for that. Does anybody have any additional comments about it?
> 
> This is just my opinion and not everybody might agree with this, but I think that it would be less wasted reviewing efforts if you apply the modifications needed to land this patch first and then do further tuning in different patches (e.g. for EFL support).
> If you refactor this code, it might be difficult to do a review on only what changed.
> 
> This patch is already larger than the average and splitting in smaller patches is usually encouraged.

Makes sense for me. I will fix now all remaining bits except EFL support which will be addressed later probably with another bug.

[Anton Obzhirov]

Created attachment 169656 [details]
Patch

[Zan Dobersek]

Is there anything specific that needs to be done to move this work forward? The patch already went through detailed reviews and is quite matured, it seems as if only the polishing is left.

At the very least I'd recommend splitting up the patch, into at least two parts. For instance, the SocketStreamHandle work seems to be ready to land, so that could go into a separate patch.

[Jocelyn Turcotte]

Comment on attachment 169656 [details]
Patch

The inspector server, Qt and SocketStreamHandle parts look fine to me. If someone could review the GTK part it would be nice.

[Carlos Garcia Campos]

Comment on attachment 169656 [details]
Patch

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

Looks good to me, I have only a couple of minor comments. Gustavo, are your concerns about the changes to WebSocketServerConnection fixed now?

> Source/WebKit2/GNUmakefile.am:560
> +dist_remoteinspector_DATA = \

do we really need the dist prefix? I guess you can use remoteinspector_DATA and add the inspectorPageIndex.html to EXTRA_DIST.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:32
> +static const char* s_testServerAppName = "InspectorTestServer";

We don't use that prefix for global variables, we use k or g without the underscore.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:35
> +static const int s_maxWaitForChild = 5;

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:38
> +static GPid s_childProcessPid = 0;

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:41
> +static bool s_childIsReady = false;

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:193
> +    String validInspectorURL = String("/webinspector/inspector.html?page=") + String::number(pageId);
> +    g_assert_cmpstr(valueString.get(), ==, validInspectorURL.latin1().data());

utf8() instead of latin1()

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:241
> +    String resolvedURL = String("http://127.0.0.1:2999/") + WebViewTest::javascriptResultToCString(javascriptResult);

I think this should be 

String("http://127.0.0.1:2999/") + String::fromUTF8(WebViewTest::javascriptResultToCString(javascriptResult).data())

because js result is a CString utf8 encoded.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:242
> +    test->loadURI(resolvedURL.latin1().data());

utf8() instead of latin1()

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:28
> +#include "FileSystem.h"

Use <WebCore/FileSystem.h>

[Anton Obzhirov]

Hi all,

I guess all found issues so far are minor and located in the test code. Can we still proceed with landing the patch? Another option would be splitting the patch into soup socket handle changes, implementation and test.

[Anton Obzhirov]

Created attachment 173334 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 173334 [details]
Patch

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

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:82
> -    // If this ASSERT happens on any platform then their SocketStreamHandle::send
> -    // followed by a SocketStreamHandle::close is not guarenteed to have sent all
> -    // data. If this happens, we need to slightly change the design to include a
> -    // SocketStreamHandleClient::didSend, handle it here, and add an m_shutdownAfterSend
> -    // state on this WebSocketServerConnection.
> -    ASSERT(!m_socket->bufferedAmount());
> +    if (m_socket->bufferedAmount()) {
> +        m_shutdownAfterSend = true;
> +        return;
> +    }

The only concern I still have is this, since the comment has not been fully applied, I'm not 100% sure this is acting as expected. If you could double-check with the person who wrote the comment that would be great, otherwise looks fine to me.

[Anton Obzhirov]

(In reply to comment #51)
> (From update of attachment 173334 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173334&action=review
> 
> > Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:82
> > -    // If this ASSERT happens on any platform then their SocketStreamHandle::send
> > -    // followed by a SocketStreamHandle::close is not guarenteed to have sent all
> > -    // data. If this happens, we need to slightly change the design to include a
> > -    // SocketStreamHandleClient::didSend, handle it here, and add an m_shutdownAfterSend
> > -    // state on this WebSocketServerConnection.
> > -    ASSERT(!m_socket->bufferedAmount());
> > +    if (m_socket->bufferedAmount()) {
> > +        m_shutdownAfterSend = true;
> > +        return;
> > +    }
> 
> The only concern I still have is this, since the comment has not been fully applied, I'm not 100% sure this is acting as expected. If you could double-check with the person who wrote the comment that would be great, otherwise looks fine to me.

Hi, from functionality point of view it follows the comment. I will check with the person who left the comment anyway, and if he is unhappy for the reason I can always submit another patch to fix it. Current solution seems to work flawlessly for me.

[Danilo de Paula]

(In reply to comment #52)
> (In reply to comment #51)
> > (From update of attachment 173334 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=173334&action=review
> > 
> > > Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:82
> > > -    // If this ASSERT happens on any platform then their SocketStreamHandle::send
> > > -    // followed by a SocketStreamHandle::close is not guarenteed to have sent all
> > > -    // data. If this happens, we need to slightly change the design to include a
> > > -    // SocketStreamHandleClient::didSend, handle it here, and add an m_shutdownAfterSend
> > > -    // state on this WebSocketServerConnection.
> > > -    ASSERT(!m_socket->bufferedAmount());
> > > +    if (m_socket->bufferedAmount()) {
> > > +        m_shutdownAfterSend = true;
> > > +        return;
> > > +    }
> > 
> > The only concern I still have is this, since the comment has not been fully applied, I'm not 100% sure this is acting as expected. If you could double-check with the person who wrote the comment that would be great, otherwise looks fine to me.
> 
> Hi, from functionality point of view it follows the comment. I will check with the person who left the comment anyway, and if he is unhappy for the reason I can always submit another patch to fix it. Current solution seems to work flawlessly for me.

Can you check if this patch compiles on a debug build, please?
Looks like WebContextGtk.cpp uses Log but it doesn't include the header.

It compiles fine on a release build, but debug is failing here.

[Anton Obzhirov]

Seems to build fine for me here. What is your IRC?

[Danilo de Paula]

(In reply to comment #54)
> Seems to build fine for me here. What is your IRC?

danilocesar

[Danilo de Paula]

(In reply to comment #55)
> (In reply to comment #54)
> > Seems to build fine for me here. What is your IRC?
> 
> danilocesar

A(In reply to comment #55)
> (In reply to comment #54)
> > Seems to build fine for me here. What is your IRC?
> 
> danilocesar

A question: Why should we specify the host address? Shouldn't we always set the server address as localhost?
On chromium, we only need to specify the port to be used. Can't we use the same solution?

[Jocelyn Turcotte]

(In reply to comment #56)
> A question: Why should we specify the host address? Shouldn't we always set the server address as localhost?
> On chromium, we only need to specify the port to be used. Can't we use the same solution?

The reason was to allow remote debugging on phones/tablets. The Qt port opens the port on localhost by default (if only the port is given) but also allows to specify the interface.

[Anton Obzhirov]

Created attachment 173859 [details]
Patch

[Anton Obzhirov]

Had to resubmit to fix compilation problem due to merging with the latest baseline, The only change is adding "Logging.h" in WebContextGtk.cpp.

[Danilo de Paula]

From a IRC talk:
[10:06] <danilocesar> is there any webkit call that can be use to use a file that can be installed or not installed on the system? Right now #88094 uses WebCore::sharedResourcesPath().data() which translates to /usr/local/share. So remote debugging won't work if it's not installed. Any idea?
[10:19] <philn> danilocesar: in AudioBusGtk for instance, we handle the uninstalled case with an env var
[10:20] <philn> the inspector does something similar iirc
[10:21] <danilocesar> I remember something about the inspector.
[10:21] <danilocesar> philn: do you remember who fixed it?
[10:22] <philn> i don't, but i did the AudioBus stuff :)
[10:23] <philn> another approach is to use a macro, i think that's what the inspector uses actually
[10:24] <philn> resources management is not great in glib... but i think some recent work was done to improve it

Is it OK to push it this way and fix it later?

[Anton Obzhirov]

I would (In reply to comment #60)
> From a IRC talk:
> [10:06] <danilocesar> is there any webkit call that can be use to use a file that can be installed or not installed on the system? Right now #88094 uses WebCore::sharedResourcesPath().data() which translates to /usr/local/share. So remote debugging won't work if it's not installed. Any idea?
> [10:19] <philn> danilocesar: in AudioBusGtk for instance, we handle the uninstalled case with an env var
> [10:20] <philn> the inspector does something similar iirc
> [10:21] <danilocesar> I remember something about the inspector.
> [10:21] <danilocesar> philn: do you remember who fixed it?
> [10:22] <philn> i don't, but i did the AudioBus stuff :)
> [10:23] <philn> another approach is to use a macro, i think that's what the inspector uses actually
> [10:24] <philn> resources management is not great in glib... but i think some recent work was done to improve it
> 
> Is it OK to push it this way and fix it later?

I would still suggest to use sharedResourcePath() for now (I need to double check but that how QT port used to do it) and submit the change. If we need to change it I can always do it with another patch later.

[Danilo de Paula]

(In reply to comment #61)
> I would (In reply to comment #60)
> > From a IRC talk:
> > [10:06] <danilocesar> is there any webkit call that can be use to use a file that can be installed or not installed on the system? Right now #88094 uses WebCore::sharedResourcesPath().data() which translates to /usr/local/share. So remote debugging won't work if it's not installed. Any idea?
> > [10:19] <philn> danilocesar: in AudioBusGtk for instance, we handle the uninstalled case with an env var
> > [10:20] <philn> the inspector does something similar iirc
> > [10:21] <danilocesar> I remember something about the inspector.
> > [10:21] <danilocesar> philn: do you remember who fixed it?
> > [10:22] <philn> i don't, but i did the AudioBus stuff :)
> > [10:23] <philn> another approach is to use a macro, i think that's what the inspector uses actually
> > [10:24] <philn> resources management is not great in glib... but i think some recent work was done to improve it
> > 
> > Is it OK to push it this way and fix it later?
> 
> I would still suggest to use sharedResourcePath() for now (I need to double check but that how QT port used to do it) and submit the change. If we need to change it I can always do it with another patch later.

Looks good enough to me.

[Jocelyn Turcotte]

(In reply to comment #51)
> The only concern I still have is this, since the comment has not been fully applied, I'm not 100% sure this is acting as expected. If you could double-check with the person who wrote the comment that would be great, otherwise looks fine to me.

Joseph Pecoraro added that comment initially for Mac and I didn't need to fix it for Qt either but I think that the way that soup is behaving is the kind of case that this assert was trying to protect against. I believe that this patch properly handles it and the assert can be removed.

[WebKit Review Bot]

Comment on attachment 173859 [details]
Patch

Clearing flags on attachment: 173859

Committed r134600: <http://trac.webkit.org/changeset/134600>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.