| Summary: | [SOUP] Network process crash in WebKit::CustomProtocolManagerImpl::didFailWithError | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||
| Component: | WebKitGTK | Assignee: | Alex Christensen <achristensen> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Major | CC: | achristensen, aestes, bugs-noreply, cgarcia, commit-queue, loic.yhuel, mcatanzaro, mkwst | ||||||||
| Priority: | P2 | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| See Also: |
https://bugzilla.redhat.com/show_bug.cgi?id=1398951 https://bugzilla.redhat.com/show_bug.cgi?id=1402191 https://bugzilla.redhat.com/show_bug.cgi?id=1419362 https://bugzilla.redhat.com/show_bug.cgi?id=1426449 |
||||||||||
| Attachments: |
|
||||||||||
|
Description
Michael Catanzaro
2016-11-27 07:26:34 PST
116 reports of this. Oh and the user report: "Simply opened an email within Evolution and the crash occured. I think the email contained HTML elements." (In reply to comment #1) > 116 reports of this. We're now up to 285 reports of this crash. We're now up to 850 reports of this crash (in Fedora). Bug #165655 is potentially-related. 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. *** Bug 165655 has been marked as a duplicate of this bug. *** (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. (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. No, thread 1 in gdb is the thread that crashed. (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. 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 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 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. (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. (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. Carlos, if you want to remove the WorkQueue and related synchronization on all platforms, I'd review that patch. Comment on attachment 297998 [details]
Patch
I agree. Let's keep this the same for all platforms.
(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. (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 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.
Michael, in the stable branch I'll land the other patch The new cpp file is not buili in mac, so the Xcode changes are definitely not correct, *sigh*. Created attachment 298119 [details]
Patch
Comment on attachment 298119 [details] Patch Clearing flags on attachment: 298119 Committed r210374: <http://trac.webkit.org/changeset/210374> All reviewed patches have been landed. Closing bug. Thank you very much, Alex. (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! (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. This commit reverted https://bugs.webkit.org/show_bug.cgi?id=146014 so reintroduced the warning. |