Looks like some clients manage to tear down WebProcessPool from processDidClose callback. Add assertion to find out how. Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x0000000000000000) [ 0] 0x000000023482c8fc WebKit`WebKit::WebProcessPool::networkProcessFailedToLaunch(WebKit::NetworkProcessProxy&) + 76 at WebProcessPool.cpp:548:21 544 ASSERT(&networkProcessProxy == m_networkProcess.get()); 545 m_didNetworkProcessCrash = true; 546 547 for (auto& supplement : m_supplements.values()) -> 548 supplement->processDidClose(&networkProcessProxy);
<rdar://problem/41157398>
Created attachment 342803 [details] patch
Comment on attachment 342803 [details] patch Attachment 342803 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/8196219 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html
Created attachment 342808 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
How did you determine that the WebProcessPool was dead? Brady and I were actually looking at the same crash trace a couple of days ago and I assumed the issue was that m_supplements was getting modified while we were iterating over it?
(In reply to Chris Dumez from comment #5) > How did you determine that the WebProcessPool was dead? Brady and I were > actually looking at the same crash trace a couple of days ago and I assumed > the issue was that m_supplements was getting modified while we were > iterating over it? Hmm, looking at it more, it looks like we only ever add supplements in the WebProcessPool constructor. Also, we do not seem to have a way to remove supplements. Therefore, I guess you must be right that the WebProcessPool is getting destroyed.
Comment on attachment 342803 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=342803&action=review > Source/WebKit/ChangeLog:9 > + Looks like some clients manage to tear down WebProcessPool from processDidClose callback. Add assertion to find out how. Why don't we protect |this| in networkProcessFailedToLaunch() ?
I'm working on a bug right now where it is known and *expected* that the WebProcessPool is getting destroyed, and therefore a protector is the right call. Ping me for details or wait until I finish my API test which reproduces it :)
(In reply to Brady Eidson from comment #8) > I'm working on a bug right now where it is known and *expected* that the > WebProcessPool is getting destroyed, and therefore a protector is the right > call. > > Ping me for details or wait until I finish my API test which reproduces it :) Actually mine is in StorageProcess related code, not NetworkProcess, but the general point still stands. You can create a WKWebsiteDataStore that points to non-default locations on disk. You can then use it for WebsiteData fetching or clearing. You can do this without ever creating a process pool or a web view. When you do so a process pool is created for you internally, and it's completely okay that said process pool is then destroyed. You can look for my storage process example later today, or try to construct a network process example if you'd like (We *can* API test this)
> Why don't we protect |this| in networkProcessFailedToLaunch() ? That's not going to be sufficient. The protection needs to be lower on the stack and other things (NetworkProcessProxy) will need to be protected too.
(In reply to Antti Koivisto from comment #10) > > Why don't we protect |this| in networkProcessFailedToLaunch() ? > > That's not going to be sufficient. The protection needs to be lower on the > stack and other things (NetworkProcessProxy) will need to be protected too. In the storage process case that is absolutely not true. Please try to reproduce this with an API test now that I've described the behavior of how these process pools can come into existence and then be destroyed.
Basically the best explanation I have for this crash is that some cookie related callback (cookies appear to be the only client for this supplement) is destroying the process pool synchronously. That sounds to me like something the client shouldn't be doing. The idea here was to find out exactly how. Of course we can do handwavy papering over too if that is preferred.
(In reply to Antti Koivisto from comment #12) > Basically the best explanation I have for this crash is that some cookie > related callback (cookies appear to be the only client for this supplement) > is destroying the process pool synchronously. That sounds to me like > something the client shouldn't be doing. I personally think it would be very fragile to expect the client not to do so. I'd rather we make our code more robust.
> I personally think it would be very fragile to expect the client not to do > so. I'd rather we make our code more robust. This a very specific crash occurring in SafariViewService. Basically I wanted to find out if there is bug in SafariViewService that we should fix.
Note that the assertion added here is correct no matter which fix is done and makes these sort of crashes much more debuggable. I don't quite get the r-.