Bug 152480 - ASSERT(m_downloads.isEmpty()) fails in DownloadProxyMap::~DownloadProxyMap()
Summary: ASSERT(m_downloads.isEmpty()) fails in DownloadProxyMap::~DownloadProxyMap()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Mac OS X 10.11
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-12-21 10:01 PST by Daniel Bates
Modified: 2019-03-10 17:28 PDT (History)
11 users (show)

See Also:


Attachments
Unit test (1.93 KB, patch)
2015-12-21 10:08 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (6.78 KB, patch)
2019-03-08 15:00 PST, David Quesada
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.60 MB, application/zip)
2019-03-08 17:58 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2015-12-21 10:01:57 PST
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()
Comment 1 Daniel Bates 2015-12-21 10:08:26 PST
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.
Comment 2 Daniel Bates 2015-12-21 10:24:04 PST
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.
Comment 3 David Quesada 2019-03-08 15:00:04 PST
Created attachment 364075 [details]
Patch
Comment 4 EWS Watchlist 2019-03-08 17:58:33 PST
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
Comment 5 EWS Watchlist 2019-03-08 17:58:35 PST
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
Comment 6 Chris Dumez 2019-03-09 18:35:03 PST
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);
Comment 7 David Quesada 2019-03-10 15:56:53 PDT
(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)
Comment 8 Chris Dumez 2019-03-10 17:01:04 PDT
Comment on attachment 364075 [details]
Patch

Oh, I forgot that the NetworkProcessProxy is not Refcounted anymore. r=me then.
Comment 9 WebKit Commit Bot 2019-03-10 17:27:28 PDT
Comment on attachment 364075 [details]
Patch

Clearing flags on attachment: 364075

Committed r242693: <https://trac.webkit.org/changeset/242693>
Comment 10 WebKit Commit Bot 2019-03-10 17:27:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-03-10 17:28:21 PDT
<rdar://problem/48754439>