Bug 187247

Summary: REGRESSION(r233381): Double WebResourceLoadStatisticsStore destructor invocation
Product: WebKit Reporter: Zan Dobersek <zan>
Component: WebKit2Assignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, cgarcia, commit-queue, mcatanzaro, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 187200    
Attachments:
Description Flags
Patch
none
Patch none

Description Zan Dobersek 2018-07-02 07:24:10 PDT 
Since r233381, the WebResourceLoadStatisticsStore destructor dispatches a task on its statistics WorkQueue and waits for it completion, but in doing so increases its reference count just to drop it again after the dispatched WTF::Function<> is destroyed and that additional reference to WebResourceLoadStatisticsStore released, yet again dropping reference count to 0 and invoking the WebResourceLoadStatisticsStore destructor.
http://trac.webkit.org/changeset/233381

Here's the backtrace, though the cause of the crash isn't obvious from it:
Thread 1 (Thread 0x7fb2a7f83f80 (LWP 23168)):
#0  0x00007fb2b5727085 in g_mutex_lock () at /home/buildbot/webkitgtk/gtk-linux-64-release-wayland-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gthread-posix.c:1336
#1  0x00007fb2b56dfe00 in g_source_destroy_internal () at /home/buildbot/webkitgtk/gtk-linux-64-release-wayland-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:1236
#2  0x00005580712cfa0b in _ZN3WTF7RunLoop9TimerBaseD2Ev ()
#3  0x00007fb2b7d25953 in _ZN6WebKit30WebResourceLoadStatisticsStoreD2Ev () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#4  0x00007fb2b7d259e9 in _ZN6WebKit30WebResourceLoadStatisticsStoreD0Ev () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#5  0x000055807128ea2d in _ZN3WTF31dispatchFunctionsFromMainThreadEv ()
#6  0x00005580712cfb63 in _ZZN3WTF7RunLoop9TimerBaseC4ERS0_ENUlPvE_4_FUNES3_ ()
#7  0x00007fb2b56e281a in g_main_dispatch () at /home/buildbot/webkitgtk/gtk-linux-64-release-wayland-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3148
#8  g_main_context_dispatch () at /home/buildbot/webkitgtk/gtk-linux-64-release-wayland-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3813
#9  0x00007fb2b56e2ba8 in g_main_context_iterate () at /home/buildbot/webkitgtk/gtk-linux-64-release-wayland-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3886
#10 0x00007fb2b56e2c4c in g_main_context_iteration () at /home/buildbot/webkitgtk/gtk-linux-64-release-wayland-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3947
#11 0x00007fb2b5cb1035 in gtk_main_iteration () at /home/buildbot/webkitgtk/gtk-linux-64-release-wayland-tests/build/WebKitBuild/DependenciesGTK/Source/gtk+-3.22.11/gtk/gtkmain.c:1413
#12 0x000055807127f2d5 in _ZN3WTR15PlatformWebViewC2EPK25OpaqueWKPageConfigurationRKNS_11TestOptionsE ()
#13 0x0000558071263654 in _ZN3WTR14TestController24createWebViewWithOptionsERKNS_11TestOptionsE ()
#14 0x000055807126557b in _ZN3WTR14TestController32ensureViewSupportsOptionsForTestERKNS_14TestInvocationE ()
#15 0x0000558071265851 in _ZN3WTR14TestController20configureViewForTestERKNS_14TestInvocationE ()
#16 0x000055807126ad1b in _ZN3WTR14TestInvocation6invokeEv ()
#17 0x000055807125e41e in _ZN3WTR14TestController7runTestEPKc ()
#18 0x000055807125e62b in _ZN3WTR14TestController20runTestingServerLoopEv ()
#19 0x0000558071262408 in _ZN3WTR14TestControllerC2EiPPKc ()
#20 0x0000558071257596 in main ()
Comment 1 Zan Dobersek 2018-07-02 07:49:46 PDT 
Created attachment 344101 [details]
Patch
Comment 2 Chris Dumez 2018-07-02 07:54:14 PDT 
Comment on attachment 344101 [details]
Patch

Please do not duplicate this code, just update the existing function to stop using postTask() and dispatch to the work queue instead.
Comment 3 Zan Dobersek 2018-07-02 08:03:18 PDT 
(In reply to Chris Dumez from comment #2)
> Comment on attachment 344101 [details]
> Patch
> 
> Please do not duplicate this code, just update the existing function to stop
> using postTask() and dispatch to the work queue instead.

applicationWillTerminate() still calls flushAndDestroyPersistentStore(). Is it fine to assume that the WebResourceLoadStatisticsStore reference through which applicationWillTerminate() will survive for the duration of this dispatch?
Comment 4 Zan Dobersek 2018-07-02 08:14:19 PDT 
Created attachment 344104 [details]
Patch
Comment 5 Zan Dobersek 2018-07-02 08:22:38 PDT 
(In reply to Zan Dobersek from comment #3)
> (In reply to Chris Dumez from comment #2)
> > Comment on attachment 344101 [details]
> > Patch
> > 
> > Please do not duplicate this code, just update the existing function to stop
> > using postTask() and dispatch to the work queue instead.
> 
> applicationWillTerminate() still calls flushAndDestroyPersistentStore(). Is
> it fine to assume that the WebResourceLoadStatisticsStore reference through
> which applicationWillTerminate() will survive for the duration of this
> dispatch?

I guess it should be the caller's responsibility to ensure object's validity for the duration of the method call they invoked.
Comment 6 Chris Dumez 2018-07-02 08:26:05 PDT 
Comment on attachment 344104 [details]
Patch

r=me, thanks for fixing.
Comment 7 Radar WebKit Bug Importer 2018-07-02 08:26:40 PDT 
<rdar://problem/41723801>
Comment 8 Zan Dobersek 2018-07-02 08:27:31 PDT 
Comment on attachment 344104 [details]
Patch

Clearing flags on attachment: 344104

Committed r233423: <https://trac.webkit.org/changeset/233423>
Comment 9 Zan Dobersek 2018-07-02 08:27:36 PDT 
All reviewed patches have been landed.  Closing bug.