ASSERT(m_downloads.isEmpty()) fails in DownloadProxyMap::~DownloadProxyMap()

• Component: WebKit2
• Status: RESOLVED FIXED

[Daniel Bates]

When running TestWebKitAPI with a debug build of WebKit in Xcode, TestWebKitAPI crashes after running test ContentFiltering.BlockDownloadAfterWillSendRequest because the assertion ASSERT(m_downloads.isEmpty()) fails in DownloadProxyMap::~DownloadProxyMap(). For some reason, I have not had success reproducing this issue when running TestWebKitAPI tests using the command line script run-api-tests.

I am using a debug build of WebKit r194240.

You can reproduce this crash by performing the following:

1. Open WebKit.xcworkspace.
2. Ensure scheme is All Tools/My Mac (64-bit).
3. Choose Product > Scheme > Edit Scheme from the menu bar.
4. In the scheme editor, click Run in the sidebar. In the Info pane, choose executable TestWebKitAPI. Then switch to the Arguments pane and pass the following argument on launch:
--gtest_filter=ContentFiltering.AllowDownloadAfterWillSendRequest:ContentFiltering.BlockDownloadAfterWillSendRequest
5. Click the Close button to close the scheme editor.
6. Run TestWebKitAPI (if you have an existing build then choose Product > Perform Action > Run Without Building).

Then you will break into a debug session when the assertion fails with the following debugger output:

ASSERTION FAILED: m_downloads.isEmpty()
/Volumes/.../Source/WebKit2/UIProcess/Downloads/DownloadProxyMap.cpp(44) : WebKit::DownloadProxyMap::~DownloadProxyMap()
1   0x10188d9b0 WTFCrash
2   0x1037c66a1 WebKit::DownloadProxyMap::~DownloadProxyMap()
3   0x1037c66d5 WebKit::DownloadProxyMap::~DownloadProxyMap()
4   0x103987172 WebKit::NetworkProcessProxy::~NetworkProcessProxy()
5   0x103987215 WebKit::NetworkProcessProxy::~NetworkProcessProxy()
6   0x103987299 WebKit::NetworkProcessProxy::~NetworkProcessProxy()
7   0x103a67843 WTF::ThreadSafeRefCounted<WebKit::ChildProcessProxy>::deref()
8   0x104019aca void WTF::derefIfNotNull<WebKit::NetworkProcessProxy>(WebKit::NetworkProcessProxy*)
9   0x104019a89 WTF::RefPtr<WebKit::NetworkProcessProxy>::~RefPtr()
10  0x10400db75 WTF::RefPtr<WebKit::NetworkProcessProxy>::~RefPtr()
11  0x1040054fe WebKit::WebProcessPool::~WebProcessPool()
12  0x104005945 WebKit::WebProcessPool::~WebProcessPool()
13  0x10419d53e -[WKProcessPool dealloc]
14  0x10366c81d API::Object::deref()
15  0x103670134 void WTF::derefIfNotNull<WebKit::WebProcessPool>(WebKit::WebProcessPool*)
16  0x1037c5957 WTF::RefPtr<WebKit::WebProcessPool>::operator=(std::nullptr_t)
17  0x1037c4a82 WebKit::DownloadProxy::invalidate()
18  0x1037c68f2 WebKit::DownloadProxyMap::downloadFinished(WebKit::DownloadProxy*)
19  0x1037c50d4 WebKit::DownloadProxy::didFinish()
20  0x1037cb8e3 void IPC::callMemberFunctionImpl<WebKit::DownloadProxy, void (WebKit::DownloadProxy::*)(), std::__1::tuple<> >(WebKit::DownloadProxy*, void (WebKit::DownloadProxy::*)(), std::__1::tuple<>&&, std::index_sequence<>)
21  0x1037cb858 void IPC::callMemberFunction<WebKit::DownloadProxy, void (WebKit::DownloadProxy::*)(), std::__1::tuple<>, std::make_index_sequence<0ul> >(std::__1::tuple<>&&, WebKit::DownloadProxy*, void (WebKit::DownloadProxy::*)())
22  0x1037ca75a void IPC::handleMessage<Messages::DownloadProxy::DidFinish, WebKit::DownloadProxy, void (WebKit::DownloadProxy::*)()>(IPC::MessageDecoder&, WebKit::DownloadProxy*, void (WebKit::DownloadProxy::*)())
23  0x1037c9d02 WebKit::DownloadProxy::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&)
24  0x1037c9e77 non-virtual thunk to WebKit::DownloadProxy::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&)
25  0x10384bfcf IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::MessageDecoder&)
26  0x1036cdc87 WebKit::ChildProcessProxy::dispatchMessage(IPC::Connection&, IPC::MessageDecoder&)
27  0x10398827a WebKit::NetworkProcessProxy::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&)
28  0x103988307 non-virtual thunk to WebKit::NetworkProcessProxy::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&)
29  0x1036dbe13 IPC::Connection::dispatchMessage(IPC::MessageDecoder&)
30  0x1036d2d41 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >)
31  0x1036dc40f IPC::Connection::dispatchOneMessage()

[Daniel Bates]

Created attachment 267753 [details]
Unit test

Unit test that demonstrates the bug. Apply the patch and run:

Tools/Scripts/run-api-tests --debug _WKDownload.CrashAfterDownloadDidFinishWhenDownloadProxyHoldsTheLastRefOnWebProcessPool

Then TestWebKitAPI will crash due to the assertion failure.

[Daniel Bates]

As can be implied from examining _WKDownload.CrashAfterDownloadDidFinishWhenDownloadProxyHoldsTheLastRefOnWebProcessPool or downloadTest() (invoked by tests ContentFiltering.{Allow, Block}DownloadAfterWillSendRequest) the DownloadProxy object associated with the started download holds the last ref to the WebProcessPool that created it. The driver code in these tests released the reference to the WebProcessPool object they held when the WKWebView they created was destroyed and when the WKProcessPool object (returned by -[WKWebViewConfiguration processPool]) was destroyed when the autorelease pool was drained.

[David Quesada]

Created attachment 364075 [details]
Patch

[EWS Watchlist]

Comment on attachment 364075 [details]
Patch

Attachment 364075 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11433932

New failing tests:
accessibility/mac/selection-notification-focus-change.html

[EWS Watchlist]

Created attachment 364101 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Chris Dumez]

Comment on attachment 364075 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364075&action=review

> Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp:62
> +    auto protectedProcessPool = makeRefPtr(m_process->processPool());

Since the DownloadProxyMap is owned by the NetworkProcessProxy, I think it would be clearer to ref m_process here:
auto protectedThis = makeRefPtr(m_process);

[David Quesada]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 364075 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364075&action=review
> 
> > Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp:62
> > +    auto protectedProcessPool = makeRefPtr(m_process->processPool());
> 
> Since the DownloadProxyMap is owned by the NetworkProcessProxy, I think it
> would be clearer to ref m_process here:
> auto protectedThis = makeRefPtr(m_process);

I thought about doing that at first, but the WebProcessPool has a unique_ptr to the NetworkProcessProxy, so I assumed it would be wrong to hold another reference outside of the "unique" reference. If we only ref the NetworkProcessProxy here, then the WebProcessPool would still be deallocated when invalidating the DownloadProxy, and the NetworkProcessProxy will be deallocated after a short existence without a WebProcessPool. I thought protecting the process pool would be safer, and I couldn't think of any downsides to doing so that would justify changing the process pool's unique_ptr to a Ref and ensuring the NetworkProcessProxy can outlive the WebProcessPool without any issues.


(Also, the win EWS issues appear unrelated - the tests are failing with or without this patch. And the mac-wk2 failure is also unrelated - when I checked on Friday, many other unrelated patches were seeing this test fail, and it was marked as a failure in r242688)

[Chris Dumez]

Comment on attachment 364075 [details]
Patch

Oh, I forgot that the NetworkProcessProxy is not Refcounted anymore. r=me then.

[WebKit Commit Bot]

Comment on attachment 364075 [details]
Patch

Clearing flags on attachment: 364075

Committed r242693: <https://trac.webkit.org/changeset/242693>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48754439>