Bug 182352 – [SOUP] http/tests/misc/bubble-drag-events.html crashes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alicia Boya García]

http/tests/misc/bubble-drag-events.html crashes since long ago in GTK.

[Fujii Hironori]

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.

[Fujii Hironori]

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

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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

[Fujii Hironori]

Created attachment 343141 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Carlos Garcia Campos]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

I will file another bug ticket for curl port.

[Fujii Hironori]

Created attachment 343206 [details]
Patch

* Addressed review feedbacks.

[Carlos Garcia Campos]

Comment on attachment 343206 [details]
Patch

Thanks!

[Fujii Hironori]

Committed r233032: <https://trac.webkit.org/changeset/233032>

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 344406 [details]
Patch

[Carlos Garcia Campos]

Created attachment 344408 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Fujii Hironori]

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 344414 [details]
Patch

[Fujii Hironori]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

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?

[Carlos Garcia Campos]

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

[youenn fablet]

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 344568 [details]
Updated patch

Abort the ping loads on connection close.

[Carlos Garcia Campos]

Ping youenn.

[Chris Dumez]

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.

[youenn fablet]

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

[Carlos Garcia Campos]

Created attachment 344929 [details]
New patch

[Chris Dumez]

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.

[Chris Dumez]

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 345033 [details]
Patch

[Chris Dumez]

Comment on attachment 345033 [details]
Patch

LGTM too, much better.

[Carlos Garcia Campos]

Committed r233842: <https://trac.webkit.org/changeset/233842>