Bug 186653 – Add release assertion against destroying WebProcessPool from networkProcessFailedToLaunch callback

• Component: WebKit2
• Status: NEW

[Antti Koivisto]

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);

[Antti Koivisto]

<rdar://problem/41157398>

[Antti Koivisto]

Created attachment 342803 [details]
patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

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?

[Chris Dumez]

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

[Chris Dumez]

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() ?

[Brady Eidson]

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 :)

[Brady Eidson]

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

[Antti Koivisto]

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

[Brady Eidson]

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

[Antti Koivisto]

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.

[Chris Dumez]

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

[Antti Koivisto]

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

[Antti Koivisto]

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