<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#c9">Comment # 9</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:mario@webkit.org" title="Mario Sanchez Prada <mario@webkit.org>"> <span class="fn">Mario Sanchez Prada</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>
Thanks a lot for the review, Carlos. I will address those issues soon enough, but in the meanwhile see my answers to your comments below...
<span class="quote">>> Source/WebKit2/ChangeLog:9
>> + 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.</span >
I see. I reported it this way because this is what I've seen myself, but will change the description in a follow-up patch
<span class="quote">>> Source/WebKit2/Shared/Network/CustomProtocols/soup/CustomProtocolManagerImpl.cpp:123
>> + 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....</span >
That was my initial version, but I was unsure about doing it and in the end preferred to do it always and assert task or !task depending on the case.
Anyway, I will change it in the next patch
<span class="quote">>> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:154
>> + }
>
> We already have a way to detect unexpected web process crashes.</span >
I see, sorry about that. Will remove it.
<span class="quote">>> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:172
>> + // happen, so we use a GFileInputStream instead of a GMemoryInputStream here.
>
> This is going to be flaky</span >
Well, yeah... thing is that it does not happen always but doing this in my new laptop (with an SSD) I get the crash 8/10 times. If I use a GMemoryInputStream, I can't get it a single time, so I thought it could be acceptable.
<span class="quote">>> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:213
>> + }
>
> We don'ty need this, WebViewTest does this automatically.</span >
Ok
<span class="quote">>> 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.</span >
Same thing here :). In my case, I reproduce it 8/10 times consistently with this 1sec delay, which I chose because I saw it as the max value used in other tests. I'd rather do something like waitUntilFinishLoading, but can't do it because I risk canceling the wrong thing... but perhaps I was doing it wrong, based on your comment above.
Anyway, about those two cases:
- didFailWithError before didReceiveResponse: Ok, will follow your suggestion.
- didFailWithError after the first didLoadData: this is the one that I'm trying to test I'm checking here and which, as I said, I manage to reproduce 8/10 times with the current patch. I'll give a try to your suggestion, though
About the names, sure I can pick better ones, no problem :)
<span class="quote">>> Tools/TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp:30
>> + test->m_loadFailed = false;
>
> I guess this assumes we always use waitUntilLoadFinishes</span >
Not sure I understand your comment. Mind to ellaborate?
<span class="quote">>> Tools/TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp:187
>> + }
>
> Ditto.</span >
Same thing :)</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>