Bug 182352 - [SOUP] http/tests/misc/bubble-drag-events.html crashes
Summary: [SOUP] http/tests/misc/bubble-drag-events.html crashes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-31 14:15 PST by Alicia Boya García
Modified: 2018-07-15 22:22 PDT (History)
9 users (show)

See Also:


Attachments
gdb log (6.56 KB, text/plain)
2018-06-19 22:36 PDT, Fujii Hironori
no flags Details
debugging patch (1.44 KB, patch)
2018-06-19 23:20 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (4.20 KB, patch)
2018-06-20 01:36 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.82 MB, application/zip)
2018-06-20 02:47 PDT, EWS Watchlist
no flags Details
Patch (4.44 KB, patch)
2018-06-20 20:07 PDT, Fujii Hironori
cgarcia: review+
Details | Formatted Diff | Diff
Patch (8.55 KB, patch)
2018-07-06 01:00 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (8.29 KB, patch)
2018-07-06 01:13 PDT, Carlos Garcia Campos
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.38 MB, application/zip)
2018-07-06 02:20 PDT, EWS Watchlist
no flags Details
Patch (8.18 KB, patch)
2018-07-06 02:49 PDT, Carlos Garcia Campos
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews205 for win-future (12.79 MB, application/zip)
2018-07-06 04:01 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.24 MB, application/zip)
2018-07-06 04:36 PDT, EWS Watchlist
no flags Details
Updated patch (8.75 KB, patch)
2018-07-09 00:45 PDT, Carlos Garcia Campos
cdumez: review-
Details | Formatted Diff | Diff
New patch (8.33 KB, patch)
2018-07-12 23:18 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (3.86 KB, patch)
2018-07-14 01:42 PDT, Carlos Garcia Campos
youennf: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alicia Boya García 2018-01-31 14:15:28 PST
http/tests/misc/bubble-drag-events.html crashes since long ago in GTK.
Comment 1 Fujii Hironori 2018-06-19 22:21:57 PDT
It's able to reproduce the crash in my Ubuntu 18.04.

./Tools/Scripts/run-webkit-tests --gtk --release -v --child-processes=1 http/tests/misc/before-unload-load-image.html http/tests/misc/bubble-drag-events.html

> [1/2] http/tests/misc/before-unload-load-image.html passed
> [2/2] http/tests/misc/bubble-drag-events.html failed (WebKitNetworkProcess crashed [pid=50749])

Callstack. 

> Thread 1 (Thread 0x7f110e0ddf80 (LWP 50749)):
> #0  0x00007f110ac93d7b in WebKit::PingLoad::didFinish(WebCore::ResourceError const&, WebCore::ResourceResponse const&) () from /home/fujii/work/webkit/ga/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #1  0x00007f110ac979ab in WebKit::PingLoad::didReceiveChallenge(WebCore::AuthenticationChallenge const&, WTF::CompletionHandler<void (WebKit::AuthenticationChallengeDisposition, WebCore::Credential const&)>&&) () from /home/fujii/work/webkit/ga/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #2  0x00007f110b0b7cb5 in WebKit::NetworkDataTaskSoup::continueAuthenticate(WebCore::AuthenticationChallenge&&) () from /home/fujii/work/webkit/ga/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #3  0x00007f110c1f06ca in WebCore::NetworkStorageSession::getCredentialFromPersistentStorage(WebCore::ProtectionSpace const&, _GCancellable*, WTF::Function<void (WebCore::Credential&&)>&&)::{lambda(_GObject*, _GAsyncResult*, void*)#1}::_FUN(_GObject*, _GAsyncResult*, void*) () from /home/fujii/work/webkit/ga/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #4  0x00007f1106ecd6b6 in g_simple_async_result_complete () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/gio/gsimpleasyncresult.c:801
> #5  0x00007f1100879f61 in ?? () from /usr/lib/x86_64-linux-gnu/libsecret-1.so.0
> #6  0x00007f1106ecd6b6 in g_simple_async_result_complete () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/gio/gsimpleasyncresult.c:801
> #7  0x00007f1106ecd739 in complete_in_idle_cb () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/gio/gsimpleasyncresult.c:813
> #8  0x00007f1106935c55 in g_main_dispatch () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3148
> #9  g_main_context_dispatch () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3813
> #10 0x00007f1106936020 in g_main_context_iterate () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3886
> #11 0x00007f1106936332 in g_main_loop_run () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:4082
> #12 0x00007f1105b95030 in WTF::RunLoop::run() () from /home/fujii/work/webkit/ga/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
> #13 0x00007f110b0b2e2d in NetworkProcessMainUnix () from /home/fujii/work/webkit/ga/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #14 0x00007f1108e6ab97 in __libc_start_main (main=0x562f1d3ad8c0 <main>, argc=3, argv=0x7fff4d3b0258, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fff4d3b0248) at ../csu/libc-start.c:310
> #15 0x0000562f1d3ad94a in _start ()

Looks same with bug 186778.
Comment 2 Fujii Hironori 2018-06-19 22:24:26 PDT
An assertion failure happens in debug build.

> ./Tools/Scripts/run-webkit-tests --gtk --debug -v --child-processes=1 http/tests/misc/before-unload-load-image.html http/tests/misc/bubble-drag-events.html

Callstack

> Thread 1 (Thread 0x7f8cd956bf80 (LWP 50494)):
> #0  0x00007f8cc3e013aa in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:267
> #1  0x00007f8cd09ed71d in WTF::UniqueRef<WebKit::NetworkLoadChecker>::operator-> (this=0x55cf3e894620) at DerivedSources/ForwardingHeaders/wtf/UniqueRef.h:58
> #2  0x00007f8cd09ebb02 in WebKit::PingLoad::currentURL (this=0x55cf3e8943c0) at ../../Source/WebKit/NetworkProcess/PingLoad.cpp:168
> #3  0x00007f8cd09eb5d5 in WebKit::PingLoad::didReceiveChallenge(WebCore::AuthenticationChallenge const&, WTF::CompletionHandler<void (WebKit::AuthenticationChallengeDisposition, WebCore::Credential const&)>&&) (this=0x55cf3e8943c0, completionHandler=...) at ../../Source/WebKit/NetworkProcess/PingLoad.cpp:118
> #4  0x00007f8cd12d1f58 in WebKit::NetworkDataTaskSoup::continueAuthenticate (this=0x7f8cb1dbb000, challenge=...) at ../../Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:524
> #5  0x00007f8cd12d156e in WebKit::NetworkDataTaskSoup::<lambda(WebCore::Credential&&)>::operator()(WebCore::Credential &&) (__closure=0x7f8c703f8008, credential=...) at ../../Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:516
> #6  0x00007f8cd12d9d50 in WTF::Function<void(WebCore::Credential&&)>::CallableWrapper<WebKit::NetworkDataTaskSoup::authenticate(WebCore::AuthenticationChallenge&&)::<lambda(WebCore::Credential&&)> >::call(WebCore::Credential &&) (this=0x7f8c703f8000, in#0=...) at DerivedSources/ForwardingHeaders/wtf/Function.h:101
> #7  0x00007f8cd33e2966 in WTF::Function<void (WebCore::Credential&&)>::operator()(WebCore::Credential&&) const (this=0x55cf3e9d3198, in#0=...) at DerivedSources/ForwardingHeaders/wtf/Function.h:56
> #8  0x00007f8cd33dc888 in WebCore::NetworkStorageSession::<lambda(GObject*, GAsyncResult*, gpointer)>::operator()(GObject *, GAsyncResult *, gpointer) const (__closure=0x0, source=0x0, result=0x55cf3e8825f0, userData=0x55cf3e9d3190) at ../../Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:235
> #9  0x00007f8cd33dcb37 in WebCore::NetworkStorageSession::<lambda(GObject*, GAsyncResult*, gpointer)>::_FUN(GObject *, GAsyncResult *, gpointer) () at ../../Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:252
> #10 0x00007f8cc5fe56b6 in g_simple_async_result_complete () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/gio/gsimpleasyncresult.c:801
> #11 0x00007f8cbcc4ff61 in ?? () from /usr/lib/x86_64-linux-gnu/libsecret-1.so.0
> #12 0x00007f8cc5fe56b6 in g_simple_async_result_complete () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/gio/gsimpleasyncresult.c:801
> #13 0x00007f8cc5fe5739 in complete_in_idle_cb () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/gio/gsimpleasyncresult.c:813
> #14 0x00007f8cc5a4dc55 in g_main_dispatch () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3148
> #15 g_main_context_dispatch () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3813
> #16 0x00007f8cc5a4e020 in g_main_context_iterate () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3886
> #17 0x00007f8cc5a4e332 in g_main_loop_run () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:4082
> #18 0x00007f8cc3e82f11 in WTF::RunLoop::run () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:96
> #19 0x00007f8cd12db150 in WebKit::ChildProcessMain<WebKit::NetworkProcess, WebKit::NetworkProcessMain> (argc=3, argv=0x7ffdc13830a8) at ../../Source/WebKit/Shared/unix/ChildProcessMain.h:61
> #20 0x00007f8cd12d58c0 in WebKit::NetworkProcessMainUnix (argc=3, argv=0x7ffdc13830a8) at ../../Source/WebKit/NetworkProcess/soup/NetworkProcessMainSoup.cpp:46
> #21 0x000055cf3cd45a45 in main (argc=3, argv=0x7ffdc13830a8) at ../../Source/WebKit/NetworkProcess/EntryPoint/unix/NetworkProcessMain.cpp:52
Comment 3 Fujii Hironori 2018-06-19 22:36:00 PDT
Created attachment 343132 [details]
gdb log

OMG. My debugger did crash just by trying to show variables.
Anyway. this->m_networkLoadChecker.m_ref is 0 in WebKit::PingLoad::didReceiveChallenge.
Comment 4 Fujii Hironori 2018-06-19 23:17:46 PDT
PingLoad is not a ref-count object.
PingLoad::didFinish deletes this.
PingLoad is destructed twice.
The first in completionHandler, and the second in didFinish.

> void PingLoad::didReceiveChallenge(const AuthenticationChallenge&, ChallengeCompletionHandler&& completionHandler)
> {
>     RELEASE_LOG_IF_ALLOWED("didReceiveChallenge");
>     completionHandler(AuthenticationChallengeDisposition::Cancel, { });
>     didFinish(ResourceError { String(), 0, currentURL(), ASCIILiteral("Failed HTTP authentication"), ResourceError::Type::AccessControl });
> }

It's unable to use protectThis ideom in this case because PingLoad is not a ref-count object.
Comment 5 Fujii Hironori 2018-06-19 23:20:30 PDT
Created attachment 343135 [details]
debugging patch

Applying this debugging patch, I got the following backtrace.
It show PingLoad::didFinish was called in the callback in PingLoad::didReceiveChallenge.

> Thread 1 (Thread 0x7f58f96a3f80 (LWP 54564)):
> #0  0x00007f58e3f393aa in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:267
> #1  0x00007f58f0b23307 in WebKit::PingLoad::didFinish (this=0x55c68d5c1370, error=..., response=...) at ../../Source/WebKit/NetworkProcess/PingLoad.cpp:79
> #2  0x00007f58f0b2382d in WebKit::PingLoad::didCompleteWithError (this=0x55c68d5c1370, error=...) at ../../Source/WebKit/NetworkProcess/PingLoad.cpp:144
> #3  0x00007f58f1408f8a in WebKit::NetworkDataTaskSoup::dispatchDidCompleteWithError (this=0x7f58d1ebb000, error=...) at ../../Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:405
> #4  0x00007f58f140d3fb in WebKit::NetworkDataTaskSoup::didFail (this=0x7f58d1ebb000, error=...) at ../../Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1046
> #5  0x00007f58f1409c1f in WebKit::NetworkDataTaskSoup::<lambda(WebKit::AuthenticationChallengeDisposition, const WebCore::Credential&)>::operator()(WebKit::AuthenticationChallengeDisposition, const WebCore::Credential &) const (__closure=0x7f58904f8348, disposition=WebKit::AuthenticationChallengeDisposition::Cancel, credential=...) at ../../Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:532
> #6  0x00007f58f1411c90 in WTF::Function<void(WebKit::AuthenticationChallengeDisposition, const WebCore::Credential&)>::CallableWrapper<WebKit::NetworkDataTaskSoup::continueAuthenticate(WebCore::AuthenticationChallenge&&)::<lambda(WebKit::AuthenticationChallengeDisposition, const WebCore::Credential&)> >::call(WebKit::AuthenticationChallengeDisposition, const WebCore::Credential &) (this=0x7f58904f8340, in#0=WebKit::AuthenticationChallengeDisposition::Cancel, in#1=...) at DerivedSources/ForwardingHeaders/wtf/Function.h:101
> #7  0x00007f58f0a9a3ce in WTF::Function<void (WebKit::AuthenticationChallengeDisposition, WebCore::Credential const&)>::operator()(WebKit::AuthenticationChallengeDisposition, WebCore::Credential const&) const (this=0x7fff3c131560, in#0=WebKit::AuthenticationChallengeDisposition::Cancel, in#1=...) at DerivedSources/ForwardingHeaders/wtf/Function.h:56
> #8  0x00007f58f0a97c02 in WTF::CompletionHandler<void (WebKit::AuthenticationChallengeDisposition, WebCore::Credential const&)>::operator()(WebKit::AuthenticationChallengeDisposition, WebCore::Credential const&) const (this=0x7fff3c1318c8, in#0=WebKit::AuthenticationChallengeDisposition::Cancel, in#1=...) at DerivedSources/ForwardingHeaders/wtf/CompletionHandler.h:60
> #9  0x00007f58f0b235de in WebKit::PingLoad::didReceiveChallenge(WebCore::AuthenticationChallenge const&, WTF::CompletionHandler<void (WebKit::AuthenticationChallengeDisposition, WebCore::Credential const&)>&&) (this=0x55c68d5c1370, completionHandler=...) at ../../Source/WebKit/NetworkProcess/PingLoad.cpp:119
> #10 0x00007f58f1409fb6 in WebKit::NetworkDataTaskSoup::continueAuthenticate (this=0x7f58d1ebb000, challenge=...) at ../../Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:524
> #11 0x00007f58f14095cc in WebKit::NetworkDataTaskSoup::<lambda(WebCore::Credential&&)>::operator()(WebCore::Credential &&) (__closure=0x7f58904f8008, credential=...) at ../../Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:516
> #12 0x00007f58f1411dae in WTF::Function<void(WebCore::Credential&&)>::CallableWrapper<WebKit::NetworkDataTaskSoup::authenticate(WebCore::AuthenticationChallenge&&)::<lambda(WebCore::Credential&&)> >::call(WebCore::Credential &&) (this=0x7f58904f8000, in#0=...) at DerivedSources/ForwardingHeaders/wtf/Function.h:101
> #13 0x00007f58f351a9c4 in WTF::Function<void (WebCore::Credential&&)>::operator()(WebCore::Credential&&) const (this=0x55c68d519a98, in#0=...) at DerivedSources/ForwardingHeaders/wtf/Function.h:56
> #14 0x00007f58f35148e6 in WebCore::NetworkStorageSession::<lambda(GObject*, GAsyncResult*, gpointer)>::operator()(GObject *, GAsyncResult *, gpointer) const (__closure=0x0, source=0x0, result=0x55c68d519df0, userData=0x55c68d519a90) at ../../Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:235
> #15 0x00007f58f3514b95 in WebCore::NetworkStorageSession::<lambda(GObject*, GAsyncResult*, gpointer)>::_FUN(GObject *, GAsyncResult *, gpointer) () at ../../Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:252
> #16 0x00007f58e611d6b6 in g_simple_async_result_complete () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/gio/gsimpleasyncresult.c:801
> #17 0x00007f58dcd87f61 in ?? () from /usr/lib/x86_64-linux-gnu/libsecret-1.so.0
> #18 0x00007f58e611d6b6 in g_simple_async_result_complete () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/gio/gsimpleasyncresult.c:801
> #19 0x00007f58e611d739 in complete_in_idle_cb () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/gio/gsimpleasyncresult.c:813
> #20 0x00007f58e5b85c55 in g_main_dispatch () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3148
> #21 g_main_context_dispatch () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3813
> #22 0x00007f58e5b86020 in g_main_context_iterate () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3886
> #23 0x00007f58e5b86332 in g_main_loop_run () at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:4082
> #24 0x00007f58e3fbaf11 in WTF::RunLoop::run () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:96
> #25 0x00007f58f14131ae in WebKit::ChildProcessMain<WebKit::NetworkProcess, WebKit::NetworkProcessMain> (argc=3, argv=0x7fff3c132128) at ../../Source/WebKit/Shared/unix/ChildProcessMain.h:61
> #26 0x00007f58f140d91e in WebKit::NetworkProcessMainUnix (argc=3, argv=0x7fff3c132128) at ../../Source/WebKit/NetworkProcess/soup/NetworkProcessMainSoup.cpp:46
> #27 0x000055c68ba08a45 in main (argc=3, argv=0x7fff3c132128) at ../../Source/WebKit/NetworkProcess/EntryPoint/unix/NetworkProcessMain.cpp:52
Comment 6 Fujii Hironori 2018-06-20 01:36:35 PDT
Created attachment 343141 [details]
Patch
Comment 7 EWS Watchlist 2018-06-20 02:47:12 PDT
Comment on attachment 343141 [details]
Patch

Attachment 343141 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8262785

New failing tests:
http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm
Comment 8 EWS Watchlist 2018-06-20 02:47:13 PDT
Created attachment 343144 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 9 Carlos Garcia Campos 2018-06-20 04:58:49 PDT
Comment on attachment 343141 [details]
Patch

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

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:534
>              cancel();
> -            didFail(cancelledError(m_soupRequest.get()));
> +            RunLoop::main().dispatch([this, protectedThis = makeRef(*this)] {
> +                didFail(cancelledError(m_soupRequest.get()));
> +            });

We only receive Cancel when the web view is destroyed and there's a pending challenge, because in case of the auth dialog being cancelled we continue the load without credentials. so, I think we could simply call invalidateAndCancel() here and return. I thin curls backend should also be fixed, since the code since to be copy pasted from here.
Comment 10 Fujii Hironori 2018-06-20 19:52:57 PDT
Cocoa port simply convers WebKit::AuthenticationChallengeDisposition::Cancel
to NSURLSessionAuthChallengeCancelAuthenticationChallenge.

https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm?rev=232520#L89

Because I'm too lazy to check the implementation behavior, I just read the document.

https://developer.apple.com/library/archive/documentation/NetworkingInternetWeb/Conceptual/NetworkingOverview/WorkingWithHTTPAndHTTPSRequests/WorkingWithHTTPAndHTTPSRequests.html

> To cancel the authentication challenge, pass
> NSURLSessionAuthChallengeCancelAuthenticationChallenge as the
> disposition (for NSURLSession) or call
> cancelAuthenticationChallenge: (for NSURLConnection). If you
> cancel the authentication challenge, the stream delegate’s error
> method is called.

It seems something called back.
Comment 11 Fujii Hironori 2018-06-20 19:54:55 PDT
Comment on attachment 343141 [details]
Patch

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

Thank you for the feedback, KaL.

>> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:534
>> +            });
> 
> We only receive Cancel when the web view is destroyed and there's a pending challenge, because in case of the auth dialog being cancelled we continue the load without credentials. so, I think we could simply call invalidateAndCancel() here and return. I thin curls backend should also be fixed, since the code since to be copy pasted from here.

I got the idea. It seems for me that it works at least at the moment. I will do so.
Comment 12 Fujii Hironori 2018-06-20 19:59:01 PDT
I will file another bug ticket for curl port.
Comment 13 Fujii Hironori 2018-06-20 20:07:53 PDT
Created attachment 343206 [details]
Patch

* Addressed review feedbacks.
Comment 14 Carlos Garcia Campos 2018-06-21 01:39:06 PDT
Comment on attachment 343206 [details]
Patch

Thanks!
Comment 15 Fujii Hironori 2018-06-21 02:55:12 PDT
Committed r233032: <https://trac.webkit.org/changeset/233032>
Comment 16 Carlos Garcia Campos 2018-07-06 00:48:00 PDT
(In reply to Carlos Garcia Campos from comment #9)
> Comment on attachment 343141 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343141&action=review
> 
> > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:534
> >              cancel();
> > -            didFail(cancelledError(m_soupRequest.get()));
> > +            RunLoop::main().dispatch([this, protectedThis = makeRef(*this)] {
> > +                didFail(cancelledError(m_soupRequest.get()));
> > +            });
> 
> We only receive Cancel when the web view is destroyed and there's a pending
> challenge, because in case of the auth dialog being cancelled we continue
> the load without credentials. so, I think we could simply call
> invalidateAndCancel() here and return. I thin curls backend should also be
> fixed, since the code since to be copy pasted from here.

I was wrong, this also happens when explicitly cancelled by the user using webkit_authentication_request_cancel(). It's even documented in API:

Cancel the authentication challenge. This will also cancel the page loading and result in a #WebKitWebView::load-failed signal with a #WebKitNetworkError of type %WEBKIT_NETWORK_ERROR_CANCELLED being emitted.

And covered by test /webkit/Authentication/authentication-cancel that is timing out since r233032. I think it's safer to make PingLoad ref counted and caller take its ownership. Reopening.
Comment 17 Carlos Garcia Campos 2018-07-06 01:00:14 PDT
Created attachment 344406 [details]
Patch
Comment 18 Carlos Garcia Campos 2018-07-06 01:13:10 PDT
Created attachment 344408 [details]
Patch
Comment 19 EWS Watchlist 2018-07-06 02:20:12 PDT
Comment on attachment 344408 [details]
Patch

Attachment 344408 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8454128

New failing tests:
media/media-fullscreen-pause-inline.html
Comment 20 EWS Watchlist 2018-07-06 02:20:14 PDT
Created attachment 344411 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 21 Fujii Hironori 2018-07-06 02:23:38 PDT
Comment on attachment 344408 [details]
Patch

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

I'm not a reviewer. This is an informal review.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:266
> +    m_pingLoaders.add(identifier, pingLoad.ptr());

Is ".ptr()" needed?

> Source/WebKit/NetworkProcess/PingLoad.cpp:84
> +    auto completionHandler = std::exchange(m_completionHandler, nullptr);

You don't need to use "std::exchange". CompletionHandler does this when be called and "if (!m_completionHandler)" will become true after the calling.
Comment 22 Carlos Garcia Campos 2018-07-06 02:47:32 PDT
(In reply to Fujii Hironori from comment #21)
> Comment on attachment 344408 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344408&action=review
> 
> I'm not a reviewer. This is an informal review.

Thanks!

> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:266
> > +    m_pingLoaders.add(identifier, pingLoad.ptr());
> 
> Is ".ptr()" needed?

Yes, because pingLoad is Ref<>.

DerivedSources/ForwardingHeaders/wtf/HashTraits.h:66:20: error: no match for ‘operator=’ (operand types are ‘WTF::RefPtr<WebKit::PingLoad>’ and ‘WTF::Ref<WebKit::PingLoad>’)
         emptyValue = std::forward<V>(value);
         ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~

> > Source/WebKit/NetworkProcess/PingLoad.cpp:84
> > +    auto completionHandler = std::exchange(m_completionHandler, nullptr);
> 
> You don't need to use "std::exchange". CompletionHandler does this when be
> called and "if (!m_completionHandler)" will become true after the calling.

Cool magic, I'll update it. Thanks.
Comment 23 Carlos Garcia Campos 2018-07-06 02:49:18 PDT
Created attachment 344414 [details]
Patch
Comment 24 Fujii Hironori 2018-07-06 03:13:07 PDT
Comment on attachment 344408 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:266
>>> +    m_pingLoaders.add(identifier, pingLoad.ptr());
>> 
>> Is ".ptr()" needed?
> 
> Yes, because pingLoad is Ref<>.
> 
> DerivedSources/ForwardingHeaders/wtf/HashTraits.h:66:20: error: no match for ‘operator=’ (operand types are ‘WTF::RefPtr<WebKit::PingLoad>’ and ‘WTF::Ref<WebKit::PingLoad>’)
>          emptyValue = std::forward<V>(value);
>          ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~

Ah, RefPtr has only operator=(Ref<X>&&) but operator=(const Ref<X>&) even though it has both operator=(const RefPtr<X, Y>&) and operator=(RefPtr<X, Y>&&).
https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/RefPtr.h?rev=230113#L96
Comment 25 EWS Watchlist 2018-07-06 04:01:06 PDT
Comment on attachment 344414 [details]
Patch

Attachment 344414 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8454861

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
Comment 26 EWS Watchlist 2018-07-06 04:01:18 PDT
Created attachment 344416 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 27 EWS Watchlist 2018-07-06 04:36:19 PDT
Comment on attachment 344414 [details]
Patch

Attachment 344414 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8454976

New failing tests:
animations/needs-layout.html
Comment 28 EWS Watchlist 2018-07-06 04:36:21 PDT
Created attachment 344419 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 29 youenn fablet 2018-07-06 10:49:44 PDT
Comment on attachment 344414 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        and NetworkConnectionToWebProcess takes its ownership like all other loads.

The issue is that ping loads should potentially last longer than its NetworkConnectionToWebProcess.
It seems that with this change, when NetworkConnectionToWebProcess goes away, PingLoad will go away also thus cancel the load.
If we want to go down that road, NetworkProcess should own the PingLoads.

I am not sure what is the issue when NetworkDataTask finishes too early.
Can we try to improve upon r233032?
Comment 30 Carlos Garcia Campos 2018-07-08 02:29:34 PDT
(In reply to youenn fablet from comment #29)
> Comment on attachment 344414 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344414&action=review
> 
> > Source/WebKit/ChangeLog:11
> > +        and NetworkConnectionToWebProcess takes its ownership like all other loads.
> 
> The issue is that ping loads should potentially last longer than its
> NetworkConnectionToWebProcess.

How can that be? The ping load completion handler is taking a reference of NetworkConnectionToWebProcess, so it will be alive until the ping load finishes.

> It seems that with this change, when NetworkConnectionToWebProcess goes
> away, PingLoad will go away also thus cancel the load.

No, because the completion handler keeps a reference. There's no change in behavior, if the connection is finished early, the ping load completion handler will also return early on if (!m_connection->isValid()).

> If we want to go down that road, NetworkProcess should own the PingLoads.

What I can do to simplify it a bit and avoid the ping loads map, is to make the completion handler also own the ping load. This way it will be removed when the ping load finishes, but only once.

> I am not sure what is the issue when NetworkDataTask finishes too early.

1- PingLoad::didReceiveChallenge() calls the auth completion handler with AuthenticationChallengeDisposition::Cancel.
2- NetworkDataTask handles AuthenticationChallengeDisposition::Cancel by cancelling the load and finishing it, by calling m_client->didCompleteWithError.
3- PingLoad::didCompleteWithError() calls PingLoad::didFinish()
4- PingLoad::didFinish() calls the completion handler and deletes this.
5- PingLoad::didReceiveChallenge() continues, but this was already deleted and PingLoad::didFinish() is called again.

> Can we try to improve upon r233032?

This patch is also preventing other potential cases where the network data task might call didCompleteWithError from a PingLoad completion handler like didReceiveResponse, for example.
Comment 31 youenn fablet 2018-07-08 09:03:38 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=344414&action=review

>>> Source/WebKit/ChangeLog:11
>>> +        and NetworkConnectionToWebProcess takes its ownership like all other loads.
>> 
>> The issue is that ping loads should potentially last longer than its NetworkConnectionToWebProcess.
>> It seems that with this change, when NetworkConnectionToWebProcess goes away, PingLoad will go away also thus cancel the load.
>> If we want to go down that road, NetworkProcess should own the PingLoads.
>> 
>> I am not sure what is the issue when NetworkDataTask finishes too early.
>> Can we try to improve upon r233032?
> 
> How can that be? The ping load completion handler is taking a reference of NetworkConnectionToWebProcess, so it will be alive until the ping load finishes.

Oh right, I overlooked this.
That said, this seems easy to break.
I would for instance be tempted to catch in the completion handler a WeakPtr<NetworkConnectionToWebProcess>.
This would allow destroying the NetworkConnectionToWebProcess as soon as the WebProcess is gone but would have an impact on PingLoads that might get unnoticed.

It seems stronger to me to have a map of PingLoads at NetworkProcess level.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:262
> +        if (m_connection->isValid())

I am not sure m_connection->isValid is actually needed.
Also, maybe only m_connection could be refed in the lambda.

> > I am not sure what is the issue when NetworkDataTask finishes too early.
> 
> 1- PingLoad::didReceiveChallenge() calls the auth completion handler with
> AuthenticationChallengeDisposition::Cancel.
> 2- NetworkDataTask handles AuthenticationChallengeDisposition::Cancel by
> cancelling the load and finishing it, by calling
> m_client->didCompleteWithError.
> 3- PingLoad::didCompleteWithError() calls PingLoad::didFinish()
> 4- PingLoad::didFinish() calls the completion handler and deletes this.

I may be missing something.
Given the CompletionHandler will also be called inside PingLoad::didFinish, it seems PingLoad will be destroyed after this call with or without the patch.

> 5- PingLoad::didReceiveChallenge() continues, but this was already deleted
> and PingLoad::didFinish() is called again.
> 
> > Can we try to improve upon r233032?
> 
> This patch is also preventing other potential cases where the network data
> task might call didCompleteWithError from a PingLoad completion handler like
> didReceiveResponse, for example.

A similar issue is also happening in NetworkResourceLoader, hence the current 'hack' that is done in NetworkResourceLoader::didReceiveResponse (call to RunLoop::main().dispatch to asynchronously error the load).
We should probably try to find a better model here.

FWIW, I would be interested in reunifying NetworkResourceLoader and PingLoad as much as possible. That would help implementing fetch keepAlive requests.
Comment 32 Carlos Garcia Campos 2018-07-08 23:18:12 PDT
(In reply to youenn fablet from comment #31)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344414&action=review
> 
> >>> Source/WebKit/ChangeLog:11
> >>> +        and NetworkConnectionToWebProcess takes its ownership like all other loads.
> >> 
> >> The issue is that ping loads should potentially last longer than its NetworkConnectionToWebProcess.
> >> It seems that with this change, when NetworkConnectionToWebProcess goes away, PingLoad will go away also thus cancel the load.
> >> If we want to go down that road, NetworkProcess should own the PingLoads.
> >> 
> >> I am not sure what is the issue when NetworkDataTask finishes too early.
> >> Can we try to improve upon r233032?
> > 
> > How can that be? The ping load completion handler is taking a reference of NetworkConnectionToWebProcess, so it will be alive until the ping load finishes.
> 
> Oh right, I overlooked this.
> That said, this seems easy to break.
> I would for instance be tempted to catch in the completion handler a
> WeakPtr<NetworkConnectionToWebProcess>.
> This would allow destroying the NetworkConnectionToWebProcess as soon as the
> WebProcess is gone but would have an impact on PingLoads that might get
> unnoticed.

That's how current code works.

> It seems stronger to me to have a map of PingLoads at NetworkProcess level.

We can keep the map in the connection, like all other maps, but abort the loads on NetworkConnectionToWebProcess::didClose. That way it doesn't matter if load has multiple references, it will be aborted as soon as the connection is closed.

> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:262
> > +        if (m_connection->isValid())
> 
> I am not sure m_connection->isValid is actually needed.
> Also, maybe only m_connection could be refed in the lambda.

Network resource loader also keeps a reference to the NetworkConnectionToWebProcess, I think we should try to keep consistency here.

> > > I am not sure what is the issue when NetworkDataTask finishes too early.
> > 
> > 1- PingLoad::didReceiveChallenge() calls the auth completion handler with
> > AuthenticationChallengeDisposition::Cancel.
> > 2- NetworkDataTask handles AuthenticationChallengeDisposition::Cancel by
> > cancelling the load and finishing it, by calling
> > m_client->didCompleteWithError.
> > 3- PingLoad::didCompleteWithError() calls PingLoad::didFinish()
> > 4- PingLoad::didFinish() calls the completion handler and deletes this.
> 
> I may be missing something.
> Given the CompletionHandler will also be called inside PingLoad::didFinish,
> it seems PingLoad will be destroyed after this call with or without the
> patch.

There are two completion handlers here, the auth one and the ping load completion handler. The auth one is calling didFinish() so the ping load is deleted.

void PingLoad::didReceiveChallenge(const AuthenticationChallenge&, ChallengeCompletionHandler&& completionHandler)
{
    RELEASE_LOG_IF_ALLOWED("didReceiveChallenge");
    completionHandler(AuthenticationChallengeDisposition::Cancel, { });
--> this is deleted here.
    didFinish(ResourceError { String(), 0, currentURL(), "Failed HTTP authentication"_s, ResourceError::Type::AccessControl });
}

This could happen in did receive response too, for example, if the completion handler called didCompleteWithError for whatever reason. By taking a ref before calling completionHandler, didFinish() will still be called twice, but the second time it will return early because m_completionHandler is nullptr.

> > 5- PingLoad::didReceiveChallenge() continues, but this was already deleted
> > and PingLoad::didFinish() is called again.
> > 
> > > Can we try to improve upon r233032?
> > 
> > This patch is also preventing other potential cases where the network data
> > task might call didCompleteWithError from a PingLoad completion handler like
> > didReceiveResponse, for example.
> 
> A similar issue is also happening in NetworkResourceLoader, hence the
> current 'hack' that is done in NetworkResourceLoader::didReceiveResponse
> (call to RunLoop::main().dispatch to asynchronously error the load).
> We should probably try to find a better model here.

Indeed, that's why I didn't like the initial patch proposed by Fujii.

> FWIW, I would be interested in reunifying NetworkResourceLoader and PingLoad
> as much as possible. That would help implementing fetch keepAlive requests.

I thought about that too, but out of scope of this patch. Actually, if we don't agree on a simple solution for this, like the proposed patch, I'll roll out r233032 to make the tests pass again until we find a better solution.
Comment 33 Carlos Garcia Campos 2018-07-09 00:45:44 PDT
Created attachment 344568 [details]
Updated patch

Abort the ping loads on connection close.
Comment 34 Carlos Garcia Campos 2018-07-11 22:07:20 PDT
Ping youenn.
Comment 35 Chris Dumez 2018-07-12 08:35:01 PDT
Comment on attachment 344568 [details]
Updated patch

The whole point of ping loads are that they can outlive the process that scheduled them. If ping loads are owned by NetworkConnectionToWebProcess, then when the WebProcess terminates, the NetworkConnectionToWebProcess goes away and we'd abort the ping load.
Comment 36 youenn fablet 2018-07-12 09:55:34 PDT
> > Oh right, I overlooked this.
> > That said, this seems easy to break.
> > I would for instance be tempted to catch in the completion handler a
> > WeakPtr<NetworkConnectionToWebProcess>.
> > This would allow destroying the NetworkConnectionToWebProcess as soon as the
> > WebProcess is gone but would have an impact on PingLoads that might get
> > unnoticed.
> 
> That's how current code works.

Current code never cancels a PingLoad.
The PingLoad, on its own, might cancel based on its timer.

> > It seems stronger to me to have a map of PingLoads at NetworkProcess level.
> 
> We can keep the map in the connection, like all other maps, but abort the
> loads on NetworkConnectionToWebProcess::didClose. That way it doesn't matter
> if load has multiple references, it will be aborted as soon as the
> connection is closed.

We want PingLoads to continue after the connection is closed.
That is why having a Map<RefPtr<PingLoad>> gives the false sense that PingLoads are owned by NetworkConnectionToWebProcess, which is not really true.

> > > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:262
> > > +        if (m_connection->isValid())
> > 
> > I am not sure m_connection->isValid is actually needed.
> > Also, maybe only m_connection could be refed in the lambda.
> 
> Network resource loader also keeps a reference to the
> NetworkConnectionToWebProcess, I think we should try to keep consistency
> here.

NetworkResourceLoader are currently tied to their WebProcess.
If the WebProcess goes away, NetworkResourceLoaders are cancelled, contrary to PingLoads.

I do not have a precise suggestion on how to move forward on this.
It seems that it should be handled at NetworkLoad/NetworkDataTask level, with maybe some adoption in NetworkResourceLoader/PingLoad.
Comment 37 Carlos Garcia Campos 2018-07-12 23:18:22 PDT
Created attachment 344929 [details]
New patch
Comment 38 Chris Dumez 2018-07-13 14:24:54 PDT
Comment on attachment 344929 [details]
New patch

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

> Source/WebKit/NetworkProcess/PingLoad.cpp:121
> +    Ref<PingLoad> protectedThis(*this);

The fact that calling the completion handler now deletes |this| makes lifetime management really awkward. The fact that we need these protectors now is proof of that.
Comment 39 Chris Dumez 2018-07-13 14:35:24 PDT
Comment on attachment 344929 [details]
New patch

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

>> Source/WebKit/NetworkProcess/PingLoad.cpp:121
>> +    Ref<PingLoad> protectedThis(*this);
> 
> The fact that calling the completion handler now deletes |this| makes lifetime management really awkward. The fact that we need these protectors now is proof of that.

Also, per your previous investigation, this means that didFinish() will now be called twice, which is weird design. My proposal:
1. Have PingLoad subclass CanMakeWeakPtr<PingLoad>
2. Have a auto weakThis = makeWeakPtr(*this) here instead of a protector
3. After calling the completion handler, return early if weakThis is null.

> Source/WebKit/NetworkProcess/PingLoad.cpp:129
> +    Ref<PingLoad> protectedThis(*this);

Same here.
Comment 40 Carlos Garcia Campos 2018-07-14 01:27:35 PDT
(In reply to Chris Dumez from comment #38)
> Comment on attachment 344929 [details]
> New patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344929&action=review
> 
> > Source/WebKit/NetworkProcess/PingLoad.cpp:121
> > +    Ref<PingLoad> protectedThis(*this);
> 
> The fact that calling the completion handler now deletes |this| makes
> lifetime management really awkward. The fact that we need these protectors
> now is proof of that.

It's the same idea, the object is deleted as soon as the operation finishes, and the operation finishes when the completion handler is called. Using delete this with an object that involves client callbacks and completion handlers is not a good idea IMO, but still. It's not that we now need these protectors, we have always needed them, that's what this bug is about, but the object is not refcounted. Calling a completion handler provided by the caller might finish the operation and delete the object, so protection has always been needed.
Comment 41 Carlos Garcia Campos 2018-07-14 01:28:29 PDT
(In reply to Chris Dumez from comment #39)
> Comment on attachment 344929 [details]
> New patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344929&action=review
> 
> >> Source/WebKit/NetworkProcess/PingLoad.cpp:121
> >> +    Ref<PingLoad> protectedThis(*this);
> > 
> > The fact that calling the completion handler now deletes |this| makes lifetime management really awkward. The fact that we need these protectors now is proof of that.
> 
> Also, per your previous investigation, this means that didFinish() will now
> be called twice, which is weird design. My proposal:
> 1. Have PingLoad subclass CanMakeWeakPtr<PingLoad>
> 2. Have a auto weakThis = makeWeakPtr(*this) here instead of a protector
> 3. After calling the completion handler, return early if weakThis is null.
> 
> > Source/WebKit/NetworkProcess/PingLoad.cpp:129
> > +    Ref<PingLoad> protectedThis(*this);
> 
> Same here.

Ok, I still think delete this is not a good idea, but I'll try this approach based on weak pointers to unblock this.
Comment 42 Carlos Garcia Campos 2018-07-14 01:42:17 PDT
Created attachment 345033 [details]
Patch
Comment 43 Chris Dumez 2018-07-14 08:15:34 PDT
Comment on attachment 345033 [details]
Patch

LGTM too, much better.
Comment 44 Carlos Garcia Campos 2018-07-15 22:22:50 PDT
Committed r233842: <https://trac.webkit.org/changeset/233842>