Bug 168963

Summary: NetworkProcess aborts in WebKit::NetworkLoad::didCompleteWithError at Source/WebKit2/NetworkProcess/NetworkLoad.cpp:423
Product: WebKit Reporter: Andres Gomez Garcia <agomez>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cdumez, cgarcia, kling, koivisto
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
BT from gdb for the NetworkProcess
none
Patch
koivisto: review-
Updated patch koivisto: review+

Description Andres Gomez Garcia 2017-02-28 01:56:28 PST
Created attachment 302925 [details]
BT from gdb for the NetworkProcess

I'm using WebKitGtk+ with my own JHBuild setting:
https://github.com/tanty/jhbuild-epiphany/tree/master

Epiphany 3.22.5 and WebKit 2.15.90.

However, the rest of the dependencies are all provided from Debian Testing.

The compilation was done with CMake args:

'-DPORT=GTK -DCMAKE_BUILD_TYPE=Release -DENABLE_MINIBROWSER=ON -DCMAKE_C_FLAGS_RELEASE="-O0 -g -DNDEBUG  -DG_DISABLE_CAST_CHECKS" -DCMAKE_CXX_FLAGS_RELEASE="-O0 -g -DNDEBUG -DG_DISABLE_CAST_CHECKS"'

After visiting several pages, eventually, the NetworkProcess aborts.

This bug is not reproducible in a predictable way.
Comment 1 Andres Gomez Garcia 2017-02-28 01:57:03 PST
(In reply to comment #0)
> Epiphany 3.22.5

Epiphany 3.22.6
Comment 2 Carlos Garcia Campos 2017-02-28 02:55:13 PST
I don't think this is specific to GTK+
Comment 3 Carlos Garcia Campos 2017-02-28 02:56:47 PST
Created attachment 302929 [details]
Patch

I haven't been able to reproduce the crash, but looking at the bt, I think this patch should fix it.
Comment 4 Andres Gomez Garcia 2017-02-28 04:13:24 PST
This is *very* easy to reproduce ... :(
Comment 5 Carlos Garcia Campos 2017-02-28 08:16:50 PST
(In reply to comment #4)
> This is *very* easy to reproduce ... :(

Could you try the patch then? It has never happened to me.
Comment 6 Chris Dumez 2017-02-28 11:02:12 PST
Antti, can you please take a look, you added this code recently.
Comment 7 Antti Koivisto 2017-02-28 23:16:43 PST
Comment on attachment 302929 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp:86
> -    didComplete();
> -
> -    // This causes call to didFailLoading().
> -    if (load)
> +    if (load) {
> +        // This causes call to didFailLoading() that calls didComplete().
>          load->continueWillSendRequest({ });
> +    } else
> +        didComplete();

This would break the functionality since didFailLoading will null out the m_response before didComplete. I think the right fix is just to remove the continueWillSendRequest and let the load be deleted.
Comment 8 Carlos Garcia Campos 2017-02-28 23:27:17 PST
Created attachment 303045 [details]
Updated patch

New patch after talking to Antti on IRC. Previous patch was not correct, because I didn't take into account that didFailLoading deletes the cache entry.
Comment 9 Carlos Garcia Campos 2017-02-28 23:36:17 PST
Committed r213206: <http://trac.webkit.org/changeset/213206>