Bug 165082 – [SOUP] Network process crash in WebKit::CustomProtocolManagerImpl::didFailWithError

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

[Michael Catanzaro]

This might be code that was recently removed from master, but anyway:

Truncated backtrace:
Thread no. 1 (10 frames)
 #0 WTF::GRefPtr<_GInputStream>::operator! at /usr/src/debug/webkitgtk-2.14.1/Source/WTF/wtf/glib/GRefPtr.h:109
 #1 WebKit::CustomProtocolManagerImpl::didFailWithError at /usr/src/debug/webkitgtk-2.14.1/Source/WebKit2/NetworkProcess/CustomProtocols/soup/CustomProtocolManagerImpl.cpp:123
 #2 IPC::callMemberFunctionImpl<WebKit::CustomProtocolManager, void (WebKit::CustomProtocolManager::*)(unsigned long, WebCore::ResourceError const&), std::tuple<unsigned long, WebCore::ResourceError>, 0ul, 1ul>(WebKit::CustomProtocolManager*, void (WebKit::CustomProtocolManager::*)(unsigned long, WebCore::ResourceError const&), std::tuple<unsigned long, WebCore::ResourceError>&&, std::integer_sequence<unsigned long, 0ul, 1ul>) at /usr/src/debug/webkitgtk-2.14.1/Source/WebKit2/Platform/IPC/HandleMessage.h:13
 #3 IPC::callMemberFunction<WebKit::CustomProtocolManager, void (WebKit::CustomProtocolManager::*)(unsigned long, WebCore::ResourceError const&), std::tuple<unsigned long, WebCore::ResourceError>, std::integer_sequence<unsigned long, 0ul, 1ul> >(std::tuple<unsigned long, WebCore::ResourceError>&&, WebKit::CustomProtocolManager*, void (WebKit::CustomProtocolManager::*)(unsigned long, WebCore::ResourceError const&)) at /usr/src/debug/webkitgtk-2.14.1/Source/WebKit2/Platform/IPC/HandleMessage.h:19
 #4 IPC::handleMessage<Messages::CustomProtocolManager::DidFailWithError, WebKit::CustomProtocolManager, void (WebKit::CustomProtocolManager::*)(unsigned long, WebCore::ResourceError const&)> at /usr/src/debug/webkitgtk-2.14.1/Source/WebKit2/Platform/IPC/HandleMessage.h:99
 #5 WebKit::CustomProtocolManager::didReceiveMessage at /usr/src/debug/webkitgtk-2.14.1/x86_64-redhat-linux-gnu/DerivedSources/WebKit2/CustomProtocolManagerMessageReceiver.cpp:45
 #6 IPC::Connection::dispatchWorkQueueMessageReceiverMessage at /usr/src/debug/webkitgtk-2.14.1/Source/WebKit2/Platform/IPC/Connection.cpp:285
 #7 WTF::Function<void ()>::operator()() const at /usr/src/debug/webkitgtk-2.14.1/Source/WTF/wtf/Function.h:50
 #8 WTF::RunLoop::performWork at /usr/src/debug/webkitgtk-2.14.1/Source/WTF/wtf/RunLoop.cpp:105
 #9 WTF::RunLoop::<lambda(gpointer)>::operator() at /usr/src/debug/webkitgtk-2.14.1/Source/WTF/wtf/glib/RunLoopGLib.cpp:66

Full backtrace downstream.

[Michael Catanzaro]

116 reports of this.

[Michael Catanzaro]

Oh and the user report: "Simply opened an email within Evolution and the crash occured.  I think the email contained HTML elements."

[Michael Catanzaro]

(In reply to comment #1)
> 116 reports of this.

We're now up to 285 reports of this crash.

[Michael Catanzaro]

We're now up to 850 reports of this crash (in Fedora). Bug #165655 is potentially-related.

[Carlos Garcia Campos]

There is one thing I don't understand from the backtrace, the main thread is a WorkQueue thread. How can this happen? 

So what happen seems to be that CustomProtocolManager messages are handled in the work queue thread instead of the real main thread. Other IPC messages are handled in the real main thread (Thread 11) and we have two threads using CustomProtocolManager at the same time. Another thing is why we ping custom uri schemes, though. I need a way to reproduce this.

[Michael Catanzaro]

*** Bug 165655 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

(In reply to comment #5)
> There is one thing I don't understand from the backtrace, the main thread is
> a WorkQueue thread. How can this happen? 

It can't happen. Thread 1 is the crashing thread, not the main thread.

In bug 1398951 the main thread is thread 11 (LWP 8497, lower number than any of the other threads) which is using CustomProtocolManagerImpl at the time of the crash, same as the crashing thread.

In bug 1402191, which I just realized was not public (fixed), the main thread is thread 5 (LWP 5528) which is calling WebKit::NetworkLoad::NetworkLoad. In this backtrace, the only the crashing thread's backtrace contains CustomProtocolManagerImpl.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #5)
> > There is one thing I don't understand from the backtrace, the main thread is
> > a WorkQueue thread. How can this happen? 
> 
> It can't happen. Thread 1 is the crashing thread, not the main thread.

Isn't Thread 1 always the main thread?

> In bug 1398951 the main thread is thread 11 (LWP 8497, lower number than any
> of the other threads) which is using CustomProtocolManagerImpl at the time
> of the crash, same as the crashing thread.
> 
> In bug 1402191, which I just realized was not public (fixed), the main
> thread is thread 5 (LWP 5528) which is calling
> WebKit::NetworkLoad::NetworkLoad. In this backtrace, the only the crashing
> thread's backtrace contains CustomProtocolManagerImpl.

[Michael Catanzaro]

No, thread 1 in gdb is the thread that crashed.

[Michael Catanzaro]

(In reply to comment #9)
> No, thread 1 in gdb is the thread that crashed.

At least in backtraces taken from core dumps it is. Maybe that's different if running a program via gdb.

[Carlos Garcia Campos]

Created attachment 297998 [details]
Patch

Ok, I finally understood the problem, see the changelog for the details. Alex, if this can be done in Mac too, I can do it in a follow up patch, or maybe even as part of the refactoring proposed in bug #165028

[Alex Christensen]

I just verified that the API test added in r149194 passes on Mac with CustomProtocolManager being a MessageReceiver instead of a WorkQueueMessageReceiver.  I did this with and without NetworkSession, but with NetworkSession also requires changing globalCustomProtocolManager() to be a std::unique_ptr and setCustomProtocolManager to call the std::unique_ptr constructor explicitly.
Let's give it a shot and see what Andy says.  We don't have many clients using WebKit2's CustomProtocolManager because it's not public API, so it's possible we have a similar thread safety issue on Mac and just don't get a lot of stack traces.

[Carlos Garcia Campos]

(In reply to comment #12)
> I just verified that the API test added in r149194 passes on Mac with
> CustomProtocolManager being a MessageReceiver instead of a
> WorkQueueMessageReceiver.  I did this with and without NetworkSession, but
> with NetworkSession also requires changing globalCustomProtocolManager() to
> be a std::unique_ptr and setCustomProtocolManager to call the
> std::unique_ptr constructor explicitly.
> Let's give it a shot and see what Andy says.  We don't have many clients
> using WebKit2's CustomProtocolManager because it's not public API, so it's
> possible we have a similar thread safety issue on Mac and just don't get a
> lot of stack traces.

In mac the manager is thread safe, the registered schemes were protected in r149198, in r149520 methods were dispatched to the main thread and in r149121 mutex were added. I think we should revert all those things if we change to use message receiver again. There are a lot of changes in mac code that I can't test myself, but I can try.

[Alex Christensen]

(In reply to comment #12)
> ...and setCustomProtocolManager to call the
> std::unique_ptr constructor explicitly.
Woah, definitely don't do this.  Use unique_ptr properly if we make this change.

Why not just make the soup custom protocol handler thread safe?  I think it would be strange to have one port be a work queue message receiver and one port not.

[Andy Estes]

(In reply to comment #12)
> I just verified that the API test added in r149194 passes on Mac with
> CustomProtocolManager being a MessageReceiver instead of a
> WorkQueueMessageReceiver.  I did this with and without NetworkSession, but
> with NetworkSession also requires changing globalCustomProtocolManager() to
> be a std::unique_ptr and setCustomProtocolManager to call the
> std::unique_ptr constructor explicitly.
> Let's give it a shot and see what Andy says.  We don't have many clients
> using WebKit2's CustomProtocolManager because it's not public API, so it's
> possible we have a similar thread safety issue on Mac and just don't get a
> lot of stack traces.

It looks like synchronous XHR is now handled by sending a delayed synchronous CoreIPC message from the Web process to the Networking process. The networking process handles the load asynchronously and replies to the Web process when finished.

Since there is no longer a nested, non-common-mode run loop, there's no longer a need for a WorkQueue in CustomProtocolManager.

At least on Mac, there is still a need for some synchronization between the main thread and the thread that NSURLProtocol methods are called on, but that shouldn't affect the Soup implementation.

[Andy Estes]

Carlos, if you want to remove the WorkQueue and related synchronization on all platforms, I'd review that patch.

[Alex Christensen]

Comment on attachment 297998 [details]
Patch

I agree.  Let's keep this the same for all platforms.

[Carlos Garcia Campos]

(In reply to comment #14)
> (In reply to comment #12)
> > ...and setCustomProtocolManager to call the
> > std::unique_ptr constructor explicitly.
> Woah, definitely don't do this.  Use unique_ptr properly if we make this
> change.
> 
> Why not just make the soup custom protocol handler thread safe?  I think it
> would be strange to have one port be a work queue message receiver and one
> port not.

Because we don't want jumps from one thread to the other and mutexes if we can just use the main thread.

[Carlos Garcia Campos]

(In reply to comment #16)
> Carlos, if you want to remove the WorkQueue and related synchronization on
> all platforms, I'd review that patch.

Okidoki

[Carlos Garcia Campos]

Created attachment 298083 [details]
New patch

Made it common to cocoa and soup. I left the mutexes and initiating run loop thing because I don't know if they are still needed in mac. I'm not sure the Xcode changes are correct.

[Carlos Garcia Campos]

Michael, in the stable branch I'll land the other patch

[Carlos Garcia Campos]

The new cpp file is not buili in mac, so the Xcode changes are definitely not correct, *sigh*.

[Alex Christensen]

Created attachment 298119 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 298119 [details]
Patch

Clearing flags on attachment: 298119

Committed r210374: <http://trac.webkit.org/changeset/210374>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

Thank you very much, Alex.

[Michael Catanzaro]

(In reply to comment #21)
> Michael, in the stable branch I'll land the other patch

It looks like you forgot? We have 190 reports of this crash in Fedora from users who have upgraded to 2.14.3 alone!

[Carlos Garcia Campos]

(In reply to comment #27)
> (In reply to comment #21)
> > Michael, in the stable branch I'll land the other patch
> 
> It looks like you forgot? We have 190 reports of this crash in Fedora from
> users who have upgraded to 2.14.3 alone!

I guess I did. I'll add it to the wiki to ensure I won't forget again, it's the best way to make sure.

[Loïc Yhuel]

This commit reverted https://bugs.webkit.org/show_bug.cgi?id=146014 so reintroduced the warning.