When working on a patch for bug #121121 I found an issue with the InspectorServer where it would try to parse an HTTP message before receiving the full message and thus fail connecting with the chromedevtools plugin. What happens is that the WebSocketServerConnection receives buffers on WebSocketServerConnection::didReceiveSocketStreamData and calls WebSocketServerConnection::readHTTPMessage which then checks if we have a valid request by calling HTTPRequest::parseHTTPRequestFromBuffer. If the request is valid it tries to parse the message and clears the buffer, otherwise it continues adding data to the internal buffer until we have a valid request. The problem is that currently HTTPRequest::parseHTTPRequestFromBuffer considers the request as valid before receiving the full message. To solve this we should make the method check if the request headers end with a blank line otherwise we consider the request as invalid (see also http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html). Patch to follow.
<rdar://problem/14958134>
Created attachment 211243 [details] Patch
Created attachment 211453 [details] Patch with GTK tests This patch is the same as attachment #211243 [details] but it also includes a patch for the GTK TestInspectorServer. I did not write a generic test as I am not sure how (or if even possible) to do a similar test using JS+HTML. If there is another option please let me know and I can update the test to not be gtk specific. Feel free to apply the other patch without the test if you prefer.
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 211453 [details] Patch with GTK tests Clearing flags on attachment: 211453 Committed r155642: <http://trac.webkit.org/changeset/155642>
Comment on attachment 211453 [details] Patch with GTK tests View in context: https://bugs.webkit.org/attachment.cgi?id=211453&action=review I'm late again, sorry, I have some comments, though, please consider fixing them in a follow up commit. > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:248 > + GSocketClient* client = g_socket_client_new(); You are leaking this, please use GRefPtr > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:249 > + GSocketConnection* connection = g_socket_client_connect_to_host(client, "127.0.0.1", 2999, NULL, &error.outPtr()); Use 0 instead of NULL for the cancellable. You are leaking the connection too, please use GRefPtr. > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:250 > + g_assert(!error.get()); Use g_assert_no_error instead, that way if there's an error you will see a message with the error description. > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:258 > + g_output_stream_write(ostream, incompleteRequest, strlen(incompleteRequest), NULL, &error.outPtr()); Use 0 instead of NULL. > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:259 > + g_assert(!error.get()); Use g_assert_no_error instead, that way if there's an error you will see a message with the error description. > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:264 > + g_input_stream_read_async(istream, response, sizeof(response) - 1, G_PRIORITY_DEFAULT, NULL, NULL, NULL); Use 0 instead of NULL. > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:268 > + // Give a chance for the server to reply. > + test->wait(2); > + // If we got any answer it means the server replied to an incomplete request, lets fail. > + g_assert(String(response).isEmpty()); I don't think this is the proper way to test this. You should use a GAsyncReadyCallback and spin a run loop to get the bytes read with g_input_stream_read_finish(). You can also start a timeout idle to assert when reached. With this code we are always waiting for 2 seconds, making running the tests slower even with it runs correctly. Also you don't need to initialize the buffer, and then convert it to a String to check if it's empty, just check the bytes actually read.
Created attachment 211555 [details] Patch addressing Carlos review (In reply to comment #6) > (From update of attachment 211453 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211453&action=review > > I'm late again, sorry, I have some comments, though, please consider fixing them in a follow up commit. > np, attach patch should address the issues, see comments below. > > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:248 > > + GSocketClient* client = g_socket_client_new(); > > You are leaking this, please use GRefPtr > Done > > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:249 > > + GSocketConnection* connection = g_socket_client_connect_to_host(client, "127.0.0.1", 2999, NULL, &error.outPtr()); > > Use 0 instead of NULL for the cancellable. You are leaking the connection too, please use GRefPtr. > Done > > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:250 > > + g_assert(!error.get()); > > Use g_assert_no_error instead, that way if there's an error you will see a message with the error description. > Done > > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:258 > > + g_output_stream_write(ostream, incompleteRequest, strlen(incompleteRequest), NULL, &error.outPtr()); > > Use 0 instead of NULL. > Done > > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:259 > > + g_assert(!error.get()); > > Use g_assert_no_error instead, that way if there's an error you will see a message with the error description. > Done > > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:264 > > + g_input_stream_read_async(istream, response, sizeof(response) - 1, G_PRIORITY_DEFAULT, NULL, NULL, NULL); > > Use 0 instead of NULL. > Done > > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:268 > > + // Give a chance for the server to reply. > > + test->wait(2); > > + // If we got any answer it means the server replied to an incomplete request, lets fail. > > + g_assert(String(response).isEmpty()); > > I don't think this is the proper way to test this. You should use a GAsyncReadyCallback and spin a run loop to get the bytes read with g_input_stream_read_finish(). You can also start a timeout idle to assert when reached. With this code we are always waiting for 2 seconds, making running the tests slower even with it runs correctly. Also you don't need to initialize the buffer, and then convert it to a String to check if it's empty, just check the bytes actually read. Quoting from IRC: <andrunko> KaL, hi there, so for bug 121123, I will do the changes you suggested but I am not sure about the last suggestion (do not wait 2 seconds) <andrunko> the thing is that with the patch (where the test pass) we should wait for a timeout as expected result <andrunko> that is why we wait 2 seconds to give a chance to the inspector server to reply, if it does /not/ reply we have a successful test <andrunko> if it does reply the test should fail <KaL> ah, I see, it's the opposite to what I thought I changed the timeout to 1 second though. The test still fails without the fix and works with the fix accordingly. Thanks both for the reviews.
Comment on attachment 211555 [details] Patch addressing Carlos review View in context: https://bugs.webkit.org/attachment.cgi?id=211555&action=review > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:249 > + GRefPtr<GSocketClient> client = g_socket_client_new(); > + GRefPtr<GSocketConnection> connection = g_socket_client_connect_to_host(client.get(), "127.0.0.1", 2999, 0, &error.outPtr()); This is still wrong, you have to adopt the reference with adoptGRef(). > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:264 > + g_input_stream_read_async(istream, response, sizeof(response) - 1, G_PRIORITY_DEFAULT, 0, 0, 0); Since the operation might never finish, I think it would be better to pass a cancellable, and cancel the operation before the test finish so that the thread used to implement the async operation exits. > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:268 > g_assert(String(response).isEmpty()); You don't need to convert the buffer to a String just to check if it's empty, you can simply check response[0] == '\0'
Created attachment 211778 [details] Patch addressing Carlos review (new round) (In reply to comment #8) > (From update of attachment 211555 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211555&action=review > > > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:249 > > + GRefPtr<GSocketClient> client = g_socket_client_new(); > > + GRefPtr<GSocketConnection> connection = g_socket_client_connect_to_host(client.get(), "127.0.0.1", 2999, 0, &error.outPtr()); > > This is still wrong, you have to adopt the reference with adoptGRef(). > Done. > > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:264 > > + g_input_stream_read_async(istream, response, sizeof(response) - 1, G_PRIORITY_DEFAULT, 0, 0, 0); > > Since the operation might never finish, I think it would be better to pass a cancellable, and cancel the operation before the test finish so that the thread used to implement the async operation exits. > Done > > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:268 > > g_assert(String(response).isEmpty()); > > You don't need to convert the buffer to a String just to check if it's empty, you can simply check response[0] == '\0' Of course, don't ask me why I did that :D. Done
Btw, TestInspectorServer is hanging on master after 86b826a. The test gets stuck on "/webkit2/WebKitWebInspectorServer/test-open-debugging-session:". I had to comment this failing testcase to test the updated patch on attachment #211778 [details].
The timeout is my bad, see https://bugs.webkit.org/show_bug.cgi?id=121383
Comment on attachment 211778 [details] Patch addressing Carlos review (new round) Thanks!
Comment on attachment 211778 [details] Patch addressing Carlos review (new round) Clearing flags on attachment: 211778 Committed r155861: <http://trac.webkit.org/changeset/155861>
All reviewed patches have been landed. Closing bug.