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.
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.
Yes, it is GTK port. I'll attach the patch on Monday.
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.
Created attachment 148350 [details] Implementation of remote Web Inspector server platform depended bits for GTK platform
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.
Created attachment 152986 [details] Patch
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.
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.
(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 =).
Created attachment 157749 [details] Patch
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.
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.
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?
(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.
Created attachment 163316 [details] Patch
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
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?
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.
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.
(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.
(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.
(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.
Created attachment 163880 [details] Patch
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.
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.
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.
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?
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
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);
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.
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?
(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.
Created attachment 167539 [details] Patch
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.
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.
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.
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?
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().
(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?
(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.
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.
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?
(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.
(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.
Created attachment 169656 [details] Patch
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.
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.
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>
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.
Created attachment 173334 [details] Patch
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.
(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.
(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.
Seems to build fine for me here. What is your IRC?
(In reply to comment #54) > Seems to build fine for me here. What is your IRC? danilocesar
(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?
(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.
Created attachment 173859 [details] Patch
Had to resubmit to fix compilation problem due to merging with the latest baseline, The only change is adding "Logging.h" in WebContextGtk.cpp.
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 (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.
(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.
(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.
Comment on attachment 173859 [details] Patch Clearing flags on attachment: 173859 Committed r134600: <http://trac.webkit.org/changeset/134600>
All reviewed patches have been landed. Closing bug.