[Webkit-unassigned] [Bug 144262] [GTK] Crash in WebProcess when loading large content with custom URI schemes

[bugzilla-daemon at webkit.org]

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

--- Comment #8 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 251857
  --> https://bugs.webkit.org/attachment.cgi?id=251857
Patch proposal (unit test included)

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

> Source/WebKit2/ChangeLog:9
> +        Consider already initiated loads of content coming out of a custom
> +        URI scheme handler when a failure has been reported from the UI process.

This is not the only case failing, it can also happen that the didFailWithError message is received before the first data chunk is read, and then we would crash in didReceiveResponse() because data is null (was removed from the map in didFailWithError(). We need an early return there when data is NULL, similar to the on in didLoadData.

> Source/WebKit2/Shared/Network/CustomProtocols/soup/CustomProtocolManagerImpl.cpp:123
>      GRefPtr<GTask> task = data->releaseTask();
> -    ASSERT(task.get());
> -    g_task_return_new_error(task.get(), g_quark_from_string(error.domain().utf8().data()),
> -        error.errorCode(), "%s", error.localizedDescription().utf8().data());
> +    if (data->stream) {
> +        // There won't be any task to complete if the loader has already started
> +        // to read data from the input stream's data sent from the UI process.
> +        ASSERT(!task);

I think we shouldn't even call data->releaseTask() when we have a stream. I would use an early return instead, something like:

if (!data->stream) {
    // Failed while reading the stream, the task was already completed by didLoadData().
    return;
}

GRefPtr<GTask> task = data->releaseTask();
ASSERT(task.get());
g_task_return_new_error....

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:154
> +    static gboolean webProcessCrashedCallback(WebKitWebView*, gpointer userData)
> +    {
> +        URISchemeTest* test = static_cast<URISchemeTest*>(userData);
> +        test->m_webProcessCrashed = true;
> +        test->quitMainLoop();
> +        return FALSE;
> +    }

We already have a way to detect unexpected web process crashes.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:172
> +            // The slower read access to data is, the easier it gets for the crash to,
> +            // happen, so we use a GFileInputStream instead of a GMemoryInputStream here.

This is going to be flaky

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:213
> +    URISchemeTest()
> +    {
> +        g_signal_connect(m_webView, "web-process-crashed", G_CALLBACK(webProcessCrashedCallback), this);
> +    }
> +
> +    ~URISchemeTest()
> +    {
> +        g_signal_handlers_disconnect_matched(m_webView, G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, this);
> +    }

We don'ty need this, WebViewTest does this automatically.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:279
> +    test->wait(1);

This is going to be super flaky :-)

There are actually at least two cases more we want to test:

 - didFailWithError before didReceiveResponse: This is easy. Just call webkit_uri_scheme_request_finish_error() right after webkit_uri_scheme_request_finish(), and wait until load finishes, it will crash.
 - didFailWithError after the first didLoadData: This is more tricky, because we need to ensure, there's at least one chunk read, and we don't finish reading the stream. Maybe we could use a pipe, and create a GUnixInputStream. Then we write something in the pipe, and then call webkit_uri_scheme_request_finish_error(). 

I would use better names than crashy, something like error:before-response and error:after-first-chunk. Since it's a load tracking test, it would also be interesting to wait until load finishes (instead of one second) and check that the loader client callbacks were correctly called.

> Tools/TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp:30
> +    // We need to reset the value of m_loadFailed or any use of LoadTrackingTest's
> +    // API after a load had failed will be wrongly considered to have failed as well.
> +    if (loadEvent != WEBKIT_LOAD_FINISHED)
> +        test->m_loadFailed = false;

I guess this assumes we always use waitUntilLoadFinishes

> Tools/TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp:187
> +    if (!m_loadFailed) {
> +        // This assertion only makes sense if the current load has not failed,
> +        // otherwise we might be comparing values relative to separate loads.
> +        g_assert_cmpfloat(m_estimatedProgress, <, progress);
> +    }

Ditto.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150429/79b39380/attachment.html>