Bug 155584

Summary: Data URL DecodeTask may get deleted outside main thread
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, beidson, cdumez, commit-queue, ddkilzer, ryanhaddad, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 155617    
Bug Blocks:    
Attachments:
Description Flags
patch
cdumez: review+
patch
darin: review+
followup ddkilzer: review+

Description Antti Koivisto 2016-03-17 08:06:01 PDT
This is unsafe as it owns strings and other types that are only safe to delete in the main thread.
Comment 1 Antti Koivisto 2016-03-17 08:11:03 PDT
rdar://problem/24492104
Comment 2 Antti Koivisto 2016-03-17 08:18:54 PDT
Created attachment 274293 [details]
patch
Comment 3 Chris Dumez 2016-03-17 08:47:56 PDT
Comment on attachment 274293 [details]
patch

r=me
Comment 4 Antti Koivisto 2016-03-17 10:02:35 PDT
https://trac.webkit.org/r198335
Comment 5 Alexey Proskuryakov 2016-03-17 17:45:57 PDT
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
Comment 6 Ryan Haddad 2016-03-17 18:03:53 PDT
I cannot find instances of this crash before r198348, so this is most likely the cause.
Comment 7 WebKit Commit Bot 2016-03-17 18:10:46 PDT
Re-opened since this is blocked by bug 155617
Comment 8 Chris Dumez 2016-03-17 19:23:43 PDT
Comment on attachment 274293 [details]
patch

Your patch basically reverted http://trac.webkit.org/changeset/191921 which was  a crash fix :)
Comment 9 Antti Koivisto 2016-03-17 23:29:03 PDT
Hah. Lets try again.
Comment 10 Antti Koivisto 2016-03-17 23:36:39 PDT
Created attachment 274383 [details]
patch
Comment 11 Darin Adler 2016-03-17 23:46:34 PDT
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 12 Darin Adler 2016-03-17 23:47:05 PDT
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"
Comment 13 Antti Koivisto 2016-03-18 00:25:58 PDT
ttps://trac.webkit.org/r198387
Comment 14 Antti Koivisto 2016-03-18 00:26:18 PDT
https://trac.webkit.org/r198387
Comment 16 David Kilzer (:ddkilzer) 2016-03-18 19:20:42 PDT
(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).
Comment 17 Antti Koivisto 2016-03-19 04:59:28 PDT
Created attachment 274513 [details]
followup

There is now a chance of null pointer crash here.
Comment 18 David Kilzer (:ddkilzer) 2016-03-19 08:29:17 PDT
Comment on attachment 274513 [details]
followup

r=me
Comment 19 David Kilzer (:ddkilzer) 2016-03-19 08:29:57 PDT
Reopened until Followup patch is landed.
Comment 20 Antti Koivisto 2016-03-19 11:54:42 PDT
http://trac.webkit.org/changeset/198471