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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Apr 29 06:18:34 PDT 2015


--- Comment #9 from Mario Sanchez Prada <mario at webkit.org> ---
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

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...

>> 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.

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

>> 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....

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

>> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:154
>> +    }
> We already have a way to detect unexpected web process crashes.

I see, sorry about that. Will remove it.

>> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:172
>> +            // happen, so we use a GFileInputStream instead of a GMemoryInputStream here.
> This is going to be flaky

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.

>> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:213
>> +    }
> 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.

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 :)

>> Tools/TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp:30
>> +        test->m_loadFailed = false;
> I guess this assumes we always use waitUntilLoadFinishes

Not sure I understand your comment. Mind to ellaborate?

>> Tools/TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp:187
>> +    }
> Ditto.

Same thing :)

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/3aa71a86/attachment-0001.html>

More information about the webkit-unassigned mailing list