[Webkit-unassigned] [Bug 121123] Web Inspector: Do not try to parse incomplete HTTP requests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=121123


Andre Moreira Magalhaes <andrunko at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #211555|                            |review?, commit-queue?
               Flag|                            |




--- Comment #7 from Andre Moreira Magalhaes <andrunko at gmail.com>  2013-09-13 08:23:55 PST ---
Created an attachment (id=211555)
 --> (https://bugs.webkit.org/attachment.cgi?id=211555&action=review)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.