<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Crash in WebProcess when loading large content with custom URI schemes"
href="https://bugs.webkit.org/show_bug.cgi?id=144262#c8">Comment # 8</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Crash in WebProcess when loading large content with custom URI schemes"
href="https://bugs.webkit.org/show_bug.cgi?id=144262">bug 144262</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=251857&action=diff" name="attach_251857" title="Patch proposal (unit test included)">attachment 251857</a> <a href="attachment.cgi?id=251857&action=edit" title="Patch proposal (unit test included)">[details]</a></span>
Patch proposal (unit test included)
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=251857&action=review">https://bugs.webkit.org/attachment.cgi?id=251857&action=review</a>
<span class="quote">> 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.</span >
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.
<span class="quote">> 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);</span >
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....
<span class="quote">> 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;
> + }</span >
We already have a way to detect unexpected web process crashes.
<span class="quote">> 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.</span >
This is going to be flaky
<span class="quote">> 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);
> + }</span >
We don'ty need this, WebViewTest does this automatically.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:279
> + test->wait(1);</span >
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.
<span class="quote">> 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;</span >
I guess this assumes we always use waitUntilLoadFinishes
<span class="quote">> 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);
> + }</span >
Ditto.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>