Bug 155584 – Data URL DecodeTask may get deleted outside main thread

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

This is unsafe as it owns strings and other types that are only safe to delete in the main thread.

[Antti Koivisto]

rdar://problem/24492104

[Antti Koivisto]

Created attachment 274293 [details]
patch

[Chris Dumez]

Comment on attachment 274293 [details]
patch

r=me

[Antti Koivisto]

https://trac.webkit.org/r198335

[Alexey Proskuryakov]

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

[Ryan Haddad]

I cannot find instances of this crash before r198348, so this is most likely the cause.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 155617

[Chris Dumez]

Comment on attachment 274293 [details]
patch

Your patch basically reverted http://trac.webkit.org/changeset/191921 which was  a crash fix :)

[Antti Koivisto]

Hah. Lets try again.

[Antti Koivisto]

Created attachment 274383 [details]
patch

[Darin Adler]

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.

[Darin Adler]

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"

[Antti Koivisto]

ttps://trac.webkit.org/r198387

[Antti Koivisto]

https://trac.webkit.org/r198387

[Ryan Haddad]

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>

[David Kilzer (:ddkilzer)]

(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).

[Antti Koivisto]

Created attachment 274513 [details]
followup

There is now a chance of null pointer crash here.

[David Kilzer (:ddkilzer)]

Comment on attachment 274513 [details]
followup

r=me

[David Kilzer (:ddkilzer)]

Reopened until Followup patch is landed.

[Antti Koivisto]

http://trac.webkit.org/changeset/198471