Bug 155584

Summary:

Data URL DecodeTask may get deleted outside main thread

Product:

WebKit

Reporter:

Antti Koivisto <koivisto>

Component:

Page Loading

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, beidson, cdumez, commit-queue, ddkilzer, ryanhaddad, webkit-bug-importer

Priority:

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 15 Ryan Haddad 2016-03-18 17:22:12 PDT

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>

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