This is unsafe as it owns strings and other types that are only safe to delete in the main thread.
rdar://problem/24492104
Created attachment 274293 [details] patch
Comment on attachment 274293 [details] patch r=me
https://trac.webkit.org/r198335
I just noticed a crash that I haven't seen before, is it a result of this change? https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r198358%20(3811)/plugins/return-negative-one-from-write-crash-log.txt Thread 22 Crashed:: Dispatch queue: org.webkit.DataURLDecoder 0 com.apple.JavaScriptCore 0x0000000106edd1e7 WTFCrash + 39 (Assertions.cpp:322) 1 com.apple.JavaScriptCore 0x0000000106f2d0ea WTF::HashTableConstIterator<WTF::RefPtr<WTF::SchedulePair>, WTF::RefPtr<WTF::SchedulePair>, WTF::IdentityExtractor, WTF::SchedulePairHash, WTF::HashTraits<WTF::RefPtr<WTF::SchedulePair> >, WTF::HashTraits<WTF::RefPtr<WTF::SchedulePair> > >::checkValidity() const + 74 (HashTable.h:213) 2 com.apple.JavaScriptCore 0x0000000106f2d039 WTF::HashTableConstIterator<WTF::RefPtr<WTF::SchedulePair>, WTF::RefPtr<WTF::SchedulePair>, WTF::IdentityExtractor, WTF::SchedulePairHash, WTF::HashTraits<WTF::RefPtr<WTF::SchedulePair> >, WTF::HashTraits<WTF::RefPtr<WTF::SchedulePair> > >::operator++() + 25 (HashTable.h:181) 3 com.apple.JavaScriptCore 0x0000000106f2c445 WTF::HashTableConstIteratorAdapter<WTF::HashTable<WTF::RefPtr<WTF::SchedulePair>, WTF::RefPtr<WTF::SchedulePair>, WTF::IdentityExtractor, WTF::SchedulePairHash, WTF::HashTraits<WTF::RefPtr<WTF::SchedulePair> >, WTF::HashTraits<WTF::RefPtr<WTF::SchedulePair> > >, WTF::RefPtr<WTF::SchedulePair> >::operator++() + 37 (HashTable.h:1437) 4 com.apple.JavaScriptCore 0x0000000106f2c1dc WTF::RunLoopTimerBase::schedule(WTF::HashSet<WTF::RefPtr<WTF::SchedulePair>, WTF::SchedulePairHash, WTF::HashTraits<WTF::RefPtr<WTF::SchedulePair> > > const&) + 156 (RunLoopTimerCF.cpp:73) 5 com.apple.WebCore 0x0000000109641703 WebCore::DataURLDecoder::DecodingResultDispatcher::startTimer() + 99 (DataURLDecoder.cpp:79) 6 com.apple.WebCore 0x0000000109640687 WebCore::DataURLDecoder::DecodingResultDispatcher::dispatch(std::__1::unique_ptr<WebCore::DataURLDecoder::DecodeTask, std::__1::default_delete<WebCore::DataURLDecoder::DecodeTask> >) + 631 (DataURLDecoder.cpp:66) 7 com.apple.WebCore 0x000000010963dc13 WebCore::DataURLDecoder::decode(WebCore::URL const&, WebCore::DataURLDecoder::ScheduleContext const&, std::__1::function<void (WTF::Optional<WebCore::DataURLDecoder::Result>)>)::$_0::operator()() const + 291 (DataURLDecoder.cpp:178) 8 com.apple.WebCore 0x000000010963dadd void std::__1::__invoke_void_return_wrapper<void>::__call<WebCore::DataURLDecoder::decode(WebCore::URL const&, WebCore::DataURLDecoder::ScheduleContext const&, std::__1::function<void (WTF::Optional<WebCore::DataURLDecoder::Result>)>)::$_0&>(WebCore::DataURLDecoder::decode(WebCore::URL const&, WebCore::DataURLDecoder::ScheduleContext const&, std::__1::function<void (WTF::Optional<WebCore::DataURLDecoder::Result>)>)::$_0&&&) + 45 (__functional_base:441) 9 com.apple.WebCore 0x000000010963da7c std::__1::__function::__func<WebCore::DataURLDecoder::decode(WebCore::URL const&, WebCore::DataURLDecoder::ScheduleContext const&, std::__1::function<void (WTF::Optional<WebCore::DataURLDecoder::Result>)>)::$_0, std::__1::allocator<WebCore::DataURLDecoder::decode(WebCore::URL const&, WebCore::DataURLDecoder::ScheduleContext const&, std::__1::function<void (WTF::Optional<WebCore::DataURLDecoder::Result>)>)::$_0>, void ()>::operator()() + 44 (functional:1407) 10 com.apple.JavaScriptCore 0x00000001067e4f2a std::__1::function<void ()>::operator()() const + 26 (functional:1793) 11 com.apple.JavaScriptCore 0x0000000106f50309 ___ZN3WTF9WorkQueue8dispatchENSt3__18functionIFvvEEE_block_invoke + 41 (WorkQueueCocoa.cpp:36) 12 libdispatch.dylib 0x00007fff93318871 _dispatch_call_block_and_release + 12
I cannot find instances of this crash before r198348, so this is most likely the cause.
Re-opened since this is blocked by bug 155617
Comment on attachment 274293 [details] patch Your patch basically reverted http://trac.webkit.org/changeset/191921 which was a crash fix :)
Hah. Lets try again.
Created attachment 274383 [details] patch
Comment on attachment 274383 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=274383&action=review > Source/WebCore/platform/network/DataURLDecoder.cpp:89 > + m_decodeTask = nullptr; I think this needs a comment, explaining that we do this so the destruction of m_decodeTask is done on the main thread.
Comment on attachment 274383 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=274383&action=review Looks like mac-wk2 EWS failed. > Source/WebCore/ChangeLog:12 > + exists the implicit deref will trigger deletion of DecodingResultDispatcher in the dispatching thread. "exits", not "exists"
ttps://trac.webkit.org/r198387
https://trac.webkit.org/r198387
Looks like the crash is still occurring with LayoutTest media/video-trackmenu-selection.html <https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r198446%20(3845)/results.html> <http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=media%2Fvideo-trackmenu-selection.html>
(In reply to comment #15) > Looks like the crash is still occurring with LayoutTest > media/video-trackmenu-selection.html > > <https://build.webkit.org/results/ > Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r198446%20(3845)/results.html> > <http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=media%2Fvideo-trackmenu-selection.html> We should file a new bug for this crash, especially if this test crashed before this fix (which it appears that it did).
Created attachment 274513 [details] followup There is now a chance of null pointer crash here.
Comment on attachment 274513 [details] followup r=me
Reopened until Followup patch is landed.
http://trac.webkit.org/changeset/198471