Bug 164458

<rdar://problem/28905514>

Created attachment 294012 [details] WIP patch This patch works but I have not written a test for it yet. I think this would be testable using a file input, URL.createObjectURL() and a download anchor. However, we do not support eventSender.beginDragWithFile() in WebKit2 so I'll need another way.

Adding a way for a test to create a file object without beginDragWithFile would let us make lots of tests work with WebKit2. That would be a huge improvement.

(In reply to comment #3) > Adding a way for a test to create a file object without beginDragWithFile > would let us make lots of tests work with WebKit2. That would be a huge > improvement. It looks like there is a internals.createFile() operation. I'll try with this.

Interesting, I didn't know about that one. It is implemented fully in WebContent, so it doesn't get a sandbox extension of its own, but most things would work anyway because WebContent already has access to the whole file system.

(In reply to comment #5) > Interesting, I didn't know about that one. It is implemented fully in > WebContent, so it doesn't get a sandbox extension of its own, but most > things would work anyway because WebContent already has access to the whole > file system. I managed to write a test using internals.createFile().

Created attachment 294014 [details] Patch

It's a bit weird that NetworkDataTaskBlob is in charge of the file references only for downloads started with DownloadStart, but not for normal loads or loads converted to download. So, I wonder if we could unify that, by moving the handling to NetworkLoad and always transferring the file references to the NetworkDataTaskBlob. Then NetworkResourceLoader would pass its file references to the NetworkLoad constructor and PendingDownload too, we could add file references to NetworkLoadParameters to avoid adding more parameters to the NetworkLoad constructor. The NetworkLoad constructor could create the NetworkDataTask passing the file references. If we add file references to NetworkLoadParameters we could change the NetworkDataTask::create() to receive the NetworkLoadParameters instead and only pass the file references to the NetworkDataTaskBlob constructor. Note that NetworkResourceLoader can revoke the file references too early because in case of load converted to donwload, NetworkResourceLoader::cleanup() is called before the download has started the IO. See bug #164220 or more details about this.

(In reply to comment #8) > It's a bit weird that NetworkDataTaskBlob is in charge of the file > references only for downloads started with DownloadStart, but not for normal > loads or loads converted to download. So, I wonder if we could unify that, > by moving the handling to NetworkLoad and always transferring the file > references to the NetworkDataTaskBlob. Then NetworkResourceLoader would pass > its file references to the NetworkLoad constructor and PendingDownload too, > we could add file references to NetworkLoadParameters to avoid adding more > parameters to the NetworkLoad constructor. The NetworkLoad constructor could > create the NetworkDataTask passing the file references. If we add file > references to NetworkLoadParameters we could change the > NetworkDataTask::create() to receive the NetworkLoadParameters instead and > only pass the file references to the NetworkDataTaskBlob constructor. Note > that NetworkResourceLoader can revoke the file references too early because > in case of load converted to donwload, NetworkResourceLoader::cleanup() is > called before the download has started the IO. See bug #164220 or more > details about this. Converting a blob load into a download is currently broken for other reasons than sandboxing. This is why I did not touch that code path yet. Otherwise, I think it would be as easy as calling transferBlobFileReferences() in NetworkReaourceLoader on the NetworkLoad object after constructing it. I could do it in this patch but I could not test it because it still does not download the blob for some reason. So I figured I will deal with pure downloads first.

(In reply to comment #9) > (In reply to comment #8) > > It's a bit weird that NetworkDataTaskBlob is in charge of the file > > references only for downloads started with DownloadStart, but not for normal > > loads or loads converted to download. So, I wonder if we could unify that, > > by moving the handling to NetworkLoad and always transferring the file > > references to the NetworkDataTaskBlob. Then NetworkResourceLoader would pass > > its file references to the NetworkLoad constructor and PendingDownload too, > > we could add file references to NetworkLoadParameters to avoid adding more > > parameters to the NetworkLoad constructor. The NetworkLoad constructor could > > create the NetworkDataTask passing the file references. If we add file > > references to NetworkLoadParameters we could change the > > NetworkDataTask::create() to receive the NetworkLoadParameters instead and > > only pass the file references to the NetworkDataTaskBlob constructor. Note > > that NetworkResourceLoader can revoke the file references too early because > > in case of load converted to donwload, NetworkResourceLoader::cleanup() is > > called before the download has started the IO. See bug #164220 or more > > details about this. > > Converting a blob load into a download is currently broken for other reasons > than sandboxing. This is why I did not touch that code path yet. Otherwise, > I think it would be as easy as calling transferBlobFileReferences() in > NetworkReaourceLoader on the NetworkLoad object after constructing it. I > could do it in this patch but I could not test it because it still does not > download the blob for some reason. So I figured I will deal with pure > downloads first. I will try to figure out why converting blob loads into downloads is broken. If it fixing them is not too much work, I will fix them in the same patch.

(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > It's a bit weird that NetworkDataTaskBlob is in charge of the file > > > references only for downloads started with DownloadStart, but not for normal > > > loads or loads converted to download. So, I wonder if we could unify that, > > > by moving the handling to NetworkLoad and always transferring the file > > > references to the NetworkDataTaskBlob. Then NetworkResourceLoader would pass > > > its file references to the NetworkLoad constructor and PendingDownload too, > > > we could add file references to NetworkLoadParameters to avoid adding more > > > parameters to the NetworkLoad constructor. The NetworkLoad constructor could > > > create the NetworkDataTask passing the file references. If we add file > > > references to NetworkLoadParameters we could change the > > > NetworkDataTask::create() to receive the NetworkLoadParameters instead and > > > only pass the file references to the NetworkDataTaskBlob constructor. Note > > > that NetworkResourceLoader can revoke the file references too early because > > > in case of load converted to donwload, NetworkResourceLoader::cleanup() is > > > called before the download has started the IO. See bug #164220 or more > > > details about this. > > > > Converting a blob load into a download is currently broken for other reasons > > than sandboxing. This is why I did not touch that code path yet. Otherwise, > > I think it would be as easy as calling transferBlobFileReferences() in > > NetworkReaourceLoader on the NetworkLoad object after constructing it. I > > could do it in this patch but I could not test it because it still does not > > download the blob for some reason. So I figured I will deal with pure > > downloads first. > > I will try to figure out why converting blob loads into downloads is broken. > If it fixing them is not too much work, I will fix them in the same patch. FYI, the reason converting blob loads into downloads fails is because it crashes the network process: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebKit 0x00000001043768b0 WebKit::NetworkDataTaskBlob::download() + 528 (NetworkDataTaskBlob.cpp:477) 1 com.apple.WebKit 0x000000010437767f WebKit::NetworkDataTaskBlob::didReceiveResponse(WebKit::NetworkDataTaskBlob::Error)::$_1::operator()(WebCore::PolicyAction) const + 237 (NetworkDataTaskBlob.cpp:318) 2 com.apple.WebKit 0x00000001042cccfd WebKit::DownloadManager::continueDecidePendingDownloadDestination(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool) + 175 (DownloadManager.cpp:125) 3 com.apple.WebKit 0x000000010432b17b WebKit::NetworkProcess::continueDecidePendingDownloadDestination(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool) + 161 (utility:753) 4 com.apple.WebKit 0x00000001043331a0 void IPC::callMemberFunctionImpl<WebKit::NetworkProcess, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool), std::__1::tuple<WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle, bool>, 0ul, 1ul, 2ul, 3ul>(WebKit::NetworkProcess*, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool), std::__1::tuple<WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle, bool>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>) + 60 (utility:753) 5 com.apple.WebKit 0x0000000104332b85 void IPC::handleMessage<Messages::NetworkProcess::ContinueDecidePendingDownloadDestination, WebKit::NetworkProcess, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool)>(IPC::Decoder&, WebKit::NetworkProcess*, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool)) + 86 (tuple:178) 6 com.apple.WebKit 0x00000001042abbd3 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 119 (memory:2702) 7 com.apple.WebKit 0x00000001042ae805 IPC::Connection::dispatchOneMessage() + 175 (memory:2722) 8 com.apple.JavaScriptCore 0x0000000105cfe5f9 WTF::RunLoop::performWork() + 169 (memory:2525) 9 com.apple.JavaScriptCore 0x0000000105cfe8c2 WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39) 10 com.apple.CoreFoundation 0x00007fff84e0a991 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17 11 com.apple.CoreFoundation 0x00007fff84debafd __CFRunLoopDoSources0 + 557 12 com.apple.CoreFoundation 0x00007fff84deaff6 __CFRunLoopRun + 934 13 com.apple.CoreFoundation 0x00007fff84dea9f4 CFRunLoopRunSpecific + 420 14 com.apple.Foundation 0x00007fff8684bcd2 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 277 15 com.apple.Foundation 0x00007fff8684bbaa -[NSRunLoop(NSRunLoop) run] + 76 16 libxpc.dylib 0x00007fff9a5b289b _xpc_objc_main + 731 17 libxpc.dylib 0x00007fff9a5b12e4 xpc_main + 494 18 com.apple.WebKit.Networking 0x00000001042687a2 main + 380 (XPCServiceMain.mm:130) 19 libdyld.dylib 0x00007fff9a34e255 start + 1 m_client is apparently null.

Created attachment 294028 [details] Patch

Created attachment 294033 [details] Patch

Created attachment 294034 [details] Patch

Comment on attachment 294034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294034&action=review > Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp:60 > + parameters.blobFileReferences = NetworkBlobRegistry::singleton().filesInBlob(*connection, parameters.request.url()); Does filesInBlob really need to return a Vector<RefPtr> instead of a Vector<Ref>? > Source/WebKit2/NetworkProcess/NetworkDataTask.h:77 > + static Ref<NetworkDataTask> create(NetworkSession&, NetworkDataTaskClient&, const NetworkLoadParameters&); Can this be NetworkLoadParameters&& so we can take ownership and possibly cut down on reference count churn? I guess maybe not, but at least the vector of file references seems like it could be moved in rather than copied. Maybe this can be NetworkLoadParameters& and we can specifically document that it will take ownership of the file references? > Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.h:51 > + static Ref<NetworkDataTask> create(NetworkSession& session, NetworkDataTaskClient& client, const WebCore::ResourceRequest& request, WebCore::ContentSniffingPolicy shouldContentSniff, const Vector<RefPtr<WebCore::BlobDataFileReference>>& fileReferences) Can this be Vector&& instead of const Vector& so we can take ownership and avoid the reference count churn?

Comment on attachment 294034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294034&action=review >> Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp:60 >> + parameters.blobFileReferences = NetworkBlobRegistry::singleton().filesInBlob(*connection, parameters.request.url()); > > Does filesInBlob really need to return a Vector<RefPtr> instead of a Vector<Ref>? No, this can be updated. I'll do it in this patch if it is a small refactoring. >> Source/WebKit2/NetworkProcess/NetworkDataTask.h:77 >> + static Ref<NetworkDataTask> create(NetworkSession&, NetworkDataTaskClient&, const NetworkLoadParameters&); > > Can this be NetworkLoadParameters&& so we can take ownership and possibly cut down on reference count churn? I guess maybe not, but at least the vector of file references seems like it could be moved in rather than copied. Maybe this can be NetworkLoadParameters& and we can specifically document that it will take ownership of the file references? NetworkLoad owns the NetworkLoadParameters and keeps it as a data member so this cannot be a NetworkLoadParameters&&. This could in theory me a NetworkLoadParameters& if we removed the const from the NetworkLoad::m_parameters data member. However, I feel it would be a bit confusing for NetworkLoad to change NetworkLoad::m_parameters. Also, this is not really hot code so I would prioritize readability / simplicity over performance. ref counting churn is unlikely to be an issue here.

Comment on attachment 294034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294034&action=review >>> Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp:60 >>> + parameters.blobFileReferences = NetworkBlobRegistry::singleton().filesInBlob(*connection, parameters.request.url()); >> >> Does filesInBlob really need to return a Vector<RefPtr> instead of a Vector<Ref>? > > No, this can be updated. I'll do it in this patch if it is a small refactoring. This is actually inconvenient for 2 reasons in this case: 1. I copy the vector at the moment and the vector is no longer copyable. So to copy I would have to manually iterate over the vector and use copyRef(). 2. Vector::appendVector(const Vector&) is used in several places with this vector and this does not work for the same reason. I think we would need a Vector::appendVector(Vector&&) overload which calls WTFMove() on each vector item.

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > (In reply to comment #8) > > > > It's a bit weird that NetworkDataTaskBlob is in charge of the file > > > > references only for downloads started with DownloadStart, but not for normal > > > > loads or loads converted to download. So, I wonder if we could unify that, > > > > by moving the handling to NetworkLoad and always transferring the file > > > > references to the NetworkDataTaskBlob. Then NetworkResourceLoader would pass > > > > its file references to the NetworkLoad constructor and PendingDownload too, > > > > we could add file references to NetworkLoadParameters to avoid adding more > > > > parameters to the NetworkLoad constructor. The NetworkLoad constructor could > > > > create the NetworkDataTask passing the file references. If we add file > > > > references to NetworkLoadParameters we could change the > > > > NetworkDataTask::create() to receive the NetworkLoadParameters instead and > > > > only pass the file references to the NetworkDataTaskBlob constructor. Note > > > > that NetworkResourceLoader can revoke the file references too early because > > > > in case of load converted to donwload, NetworkResourceLoader::cleanup() is > > > > called before the download has started the IO. See bug #164220 or more > > > > details about this. > > > > > > Converting a blob load into a download is currently broken for other reasons > > > than sandboxing. This is why I did not touch that code path yet. Otherwise, > > > I think it would be as easy as calling transferBlobFileReferences() in > > > NetworkReaourceLoader on the NetworkLoad object after constructing it. I > > > could do it in this patch but I could not test it because it still does not > > > download the blob for some reason. So I figured I will deal with pure > > > downloads first. > > > > I will try to figure out why converting blob loads into downloads is broken. > > If it fixing them is not too much work, I will fix them in the same patch. > > FYI, the reason converting blob loads into downloads fails is because it > crashes the network process: > Thread 0 Crashed:: Dispatch queue: com.apple.main-thread > 0 com.apple.WebKit 0x00000001043768b0 > WebKit::NetworkDataTaskBlob::download() + 528 (NetworkDataTaskBlob.cpp:477) > 1 com.apple.WebKit 0x000000010437767f > WebKit::NetworkDataTaskBlob::didReceiveResponse(WebKit::NetworkDataTaskBlob:: > Error)::$_1::operator()(WebCore::PolicyAction) const + 237 > (NetworkDataTaskBlob.cpp:318) > 2 com.apple.WebKit 0x00000001042cccfd > WebKit::DownloadManager::continueDecidePendingDownloadDestination(WebKit:: > DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool) + > 175 (DownloadManager.cpp:125) > 3 com.apple.WebKit 0x000000010432b17b > WebKit::NetworkProcess::continueDecidePendingDownloadDestination(WebKit:: > DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool) + > 161 (utility:753) > 4 com.apple.WebKit 0x00000001043331a0 void > IPC::callMemberFunctionImpl<WebKit::NetworkProcess, void > (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, > WebKit::SandboxExtension::Handle const&, bool), > std::__1::tuple<WebKit::DownloadID, WTF::String, > WebKit::SandboxExtension::Handle, bool>, 0ul, 1ul, 2ul, > 3ul>(WebKit::NetworkProcess*, void > (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, > WebKit::SandboxExtension::Handle const&, bool), > std::__1::tuple<WebKit::DownloadID, WTF::String, > WebKit::SandboxExtension::Handle, bool>&&, > std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>) + 60 > (utility:753) > 5 com.apple.WebKit 0x0000000104332b85 void > IPC::handleMessage<Messages::NetworkProcess:: > ContinueDecidePendingDownloadDestination, WebKit::NetworkProcess, void > (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, > WebKit::SandboxExtension::Handle const&, bool)>(IPC::Decoder&, > WebKit::NetworkProcess*, void > (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, > WebKit::SandboxExtension::Handle const&, bool)) + 86 (tuple:178) > 6 com.apple.WebKit 0x00000001042abbd3 > IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, > std::__1::default_delete<IPC::Decoder> >) + 119 (memory:2702) > 7 com.apple.WebKit 0x00000001042ae805 > IPC::Connection::dispatchOneMessage() + 175 (memory:2722) > 8 com.apple.JavaScriptCore 0x0000000105cfe5f9 > WTF::RunLoop::performWork() + 169 (memory:2525) > 9 com.apple.JavaScriptCore 0x0000000105cfe8c2 > WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39) > 10 com.apple.CoreFoundation 0x00007fff84e0a991 > __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17 > 11 com.apple.CoreFoundation 0x00007fff84debafd __CFRunLoopDoSources0 > + 557 > 12 com.apple.CoreFoundation 0x00007fff84deaff6 __CFRunLoopRun + 934 > 13 com.apple.CoreFoundation 0x00007fff84dea9f4 CFRunLoopRunSpecific + > 420 > 14 com.apple.Foundation 0x00007fff8684bcd2 -[NSRunLoop(NSRunLoop) > runMode:beforeDate:] + 277 > 15 com.apple.Foundation 0x00007fff8684bbaa -[NSRunLoop(NSRunLoop) > run] + 76 > 16 libxpc.dylib 0x00007fff9a5b289b _xpc_objc_main + 731 > 17 libxpc.dylib 0x00007fff9a5b12e4 xpc_main + 494 > 18 com.apple.WebKit.Networking 0x00000001042687a2 main + 380 > (XPCServiceMain.mm:130) > 19 libdyld.dylib 0x00007fff9a34e255 start + 1 > > m_client is apparently null. Yes, this is not specific to blobs, it's bug #164220

(In reply to comment #15) > Comment on attachment 294034 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=294034&action=review > > > Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp:60 > > + parameters.blobFileReferences = NetworkBlobRegistry::singleton().filesInBlob(*connection, parameters.request.url()); > > Does filesInBlob really need to return a Vector<RefPtr> instead of a > Vector<Ref>? > > > Source/WebKit2/NetworkProcess/NetworkDataTask.h:77 > > + static Ref<NetworkDataTask> create(NetworkSession&, NetworkDataTaskClient&, const NetworkLoadParameters&); > > Can this be NetworkLoadParameters&& so we can take ownership and possibly > cut down on reference count churn? I guess maybe not, but at least the > vector of file references seems like it could be moved in rather than > copied. Maybe this can be NetworkLoadParameters& and we can specifically > document that it will take ownership of the file references? This was my idea, but I didn't notice that m_parameters is const member of NetworkLoad. > > Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.h:51 > > + static Ref<NetworkDataTask> create(NetworkSession& session, NetworkDataTaskClient& client, const WebCore::ResourceRequest& request, WebCore::ContentSniffingPolicy shouldContentSniff, const Vector<RefPtr<WebCore::BlobDataFileReference>>& fileReferences) > > Can this be Vector&& instead of const Vector& so we can take ownership and > avoid the reference count churn?

Comment on attachment 294034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294034&action=review > Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp:48 > +void DownloadManager::startDownload(NetworkConnectionToWebProcess* connection, SessionID sessionID, DownloadID downloadID, const ResourceRequest& request, const String& suggestedName) Can we pass a reference instead of a pointer?

(In reply to comment #20) > Comment on attachment 294034 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=294034&action=review > > > Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp:48 > > +void DownloadManager::startDownload(NetworkConnectionToWebProcess* connection, SessionID sessionID, DownloadID downloadID, const ResourceRequest& request, const String& suggestedName) > > Can we pass a reference instead of a pointer? One of the call sites does not have it, this is why it is a pointer. There is a RequestDownload IPC coming from the UI process, which ends up calling startDownload which therefore does not have a corresponding connection to a WebProcess.

Comment on attachment 294034 [details] Patch Attachment 294034 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2469276 New failing tests: fast/dom/HTMLAnchorElement/anchor-file-blob-convert-to-download.html

Created attachment 294085 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Created attachment 294087 [details] Patch

Comment on attachment 294087 [details] Patch Attachment 294087 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2470244 New failing tests: fast/dom/HTMLAnchorElement/anchor-file-blob-download.html fast/dom/HTMLAnchorElement/anchor-file-blob-convert-to-download.html

Created attachment 294103 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

(In reply to comment #26) > Created attachment 294103 [details] > Archive of layout-test-results from ews107 for mac-yosemite-wk2 > > The attached test failures were seen while running run-webkit-tests on the > mac-wk2-ews. > Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5 Hmm, the bot is using Yosemite, which likely means it is not using NETWORK_SESSION but the old code path instead. This is why my new tests are failing. I think I'll skip the new tests on non-Sierra and file a bug for myself to fix the !NETWORK_SESSION code path as well.

Created attachment 294173 [details] Patch

Comment on attachment 294173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294173&action=review > Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.cpp:484 > - ASSERT(m_client); > - m_client->didBecomeDownload(); > + if (m_client) > + m_client->didBecomeDownload(); Note that this is not correct, we should have a client always here. But it's properly fixed in bug #164220, this will need to be rebased if that one land first, or the other if this lands first, so it's not a problem.

Comment on attachment 294173 [details] Patch Clearing flags on attachment: 294173 Committed r208438: <http://trac.webkit.org/changeset/208438>

All reviewed patches have been landed. Closing bug.

These two tests crash on Sierra. 0 com.apple.JavaScriptCore 0x000000010bf32d44 WTFCrash + 36 (Assertions.cpp:323) 1 com.apple.WebKit 0x000000010723b721 WebKit::NetworkDataTaskBlob::readFile(WebCore::BlobDataItem const&) + 81 (NetworkDataTaskBlob.cpp:363) 2 com.apple.WebKit 0x000000010723b47a WebKit::NetworkDataTaskBlob::read() + 282 (NetworkDataTaskBlob.cpp:345) 3 com.apple.WebKit 0x000000010723c509 WebKit::NetworkDataTaskBlob::download() + 1145 (NetworkDataTaskBlob.cpp:487) 4 com.apple.WebKit 0x0000000107240cff WebKit::NetworkDataTaskBlob::didReceiveResponse(WebKit::NetworkDataTaskBlob::Error)::$_1::operator()(WebCore::PolicyAction) const + 255 (NetworkDataTaskBlob.cpp:328) 5 com.apple.WebKit 0x0000000107240b5a WTF::Function<void (WebCore::PolicyAction)>::CallableWrapper<WebKit::NetworkDataTaskBlob::didReceiveResponse(WebKit::NetworkDataTaskBlob::Error)::$_1>::call(WebCore::PolicyAction) + 42 (Function.h:89) 6 com.apple.WebKit 0x0000000106f4ec94 WTF::Function<void (WebCore::PolicyAction)>::operator()(WebCore::PolicyAction) const + 132 (Function.h:50) 7 com.apple.WebKit 0x0000000106f4e29f WebKit::DownloadManager::continueDecidePendingDownloadDestination(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool) + 639 (DownloadManager.cpp:129) 8 com.apple.WebKit 0x00000001070e8b22 WebKit::NetworkProcess::continueDecidePendingDownloadDestination(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool) + 162 (NetworkProcess.cpp:530) 9 com.apple.WebKit 0x0000000107116edc void IPC::callMemberFunctionImpl<WebKit::NetworkProcess, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool), std::__1::tuple<WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle, bool>, 0ul, 1ul, 2ul, 3ul>(WebKit::NetworkProcess*, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool), std::__1::tuple<WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle, bool>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>) + 364 (HandleMessage.h:15) 10 com.apple.WebKit 0x0000000107116b28 void IPC::callMemberFunction<WebKit::NetworkProcess, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool), std::__1::tuple<WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle, bool>, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul> >(std::__1::tuple<WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle, bool>&&, WebKit::NetworkProcess*, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool)) + 88 (HandleMessage.h:22) 11 com.apple.WebKit 0x0000000107113602 void IPC::handleMessage<Messages::NetworkProcess::ContinueDecidePendingDownloadDestination, WebKit::NetworkProcess, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool)>(IPC::Decoder&, WebKit::NetworkProcess*, void (WebKit::NetworkProcess::*)(WebKit::DownloadID, WTF::String, WebKit::SandboxExtension::Handle const&, bool)) + 338 (HandleMessage.h:102) 12 com.apple.WebKit 0x0000000107111a78 WebKit::NetworkProcess::didReceiveNetworkProcessMessage(IPC::Connection&, IPC::Decoder&) + 1976 (NetworkProcessMessageReceiver.cpp:129) 13 com.apple.WebKit 0x00000001070e409b WebKit::NetworkProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 171 (NetworkProcess.cpp:153) 14 com.apple.WebKit 0x0000000106ec3f53 IPC::Connection::dispatchMessage(IPC::Decoder&) + 51 (Connection.cpp:894) 15 com.apple.WebKit 0x0000000106eb97f8 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 712 (Connection.cpp:922) 16 com.apple.WebKit 0x0000000106ec4550 IPC::Connection::dispatchOneMessage() + 1520 (Connection.cpp:951) 17 com.apple.WebKit 0x0000000106edd15d IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()() + 29 (Connection.cpp:888) 18 com.apple.WebKit 0x0000000106edd0b9 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call() + 25 (Function.h:89) 19 com.apple.JavaScriptCore 0x000000010bf5b42e WTF::Function<void ()>::operator()() const + 94 (Function.h:50) 20 com.apple.JavaScriptCore 0x000000010bf76a53 WTF::RunLoop::performWork() + 211 (RunLoop.cpp:106) 21 com.apple.JavaScriptCore 0x000000010bf77244 WTF::RunLoop::performWork(void*) + 36 (RunLoopCF.cpp:38) 22 com.apple.CoreFoundation 0x00007fff93602581 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17 23 com.apple.CoreFoundation 0x00007fff935e398c __CFRunLoopDoSources0 + 556 24 com.apple.CoreFoundation 0x00007fff935e2e76 __CFRunLoopRun + 934 25 com.apple.CoreFoundation 0x00007fff935e2874 CFRunLoopRunSpecific + 420 26 com.apple.Foundation 0x00007fff94ffecb2 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 277 27 com.apple.Foundation 0x00007fff94ffeb8a -[NSRunLoop(NSRunLoop) run] + 76 28 libxpc.dylib 0x00007fffa8917887 _xpc_objc_main + 731 29 libxpc.dylib 0x00007fffa89162d0 xpc_main + 494 30 com.apple.WebKit.Networking 0x0000000106df618d main + 797 31 libdyld.dylib 0x00007fffa86b4255 start + 1

rolled out in http://trac.webkit.org/changeset/208513

(In reply to comment #33) > rolled out in http://trac.webkit.org/changeset/208513 ASSERT(m_client); is hit in NetworkDataTaskBlob::readFile(const BlobDataItem& item). Why is this assertion correct? My understanding is that the NetworkDataTaskBlob no longer has a client after the load gets converted into a download. Will Carlos' patch at https://bugs.webkit.org/show_bug.cgi?id=164220 help? My understand is that it keeps a client alive a bit longer but I do not know if that's long enough to actually read the file.

Created attachment 294330 [details] Patch

Created attachment 294334 [details] Patch

Comment on attachment 294334 [details] Patch Clearing flags on attachment: 294334 Committed r208522: <http://trac.webkit.org/changeset/208522>

All reviewed patches have been landed. Closing bug.