Bug 207342 – REGRESSION (r255953): [ iOS Mac ] imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events.html is crashing

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Truitt Savell]

imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events.html

This test began crashing after the changes in r255953

History:
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fweb-animations%2Ftiming-model%2Ftimelines%2Fupdate-and-send-events.html

Log:
https://build.webkit.org/results/Apple-Catalina-Debug-WK2-Tests/r255953%20(2161)/imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-crash-log.txt

[Radar WebKit Bug Importer]

<rdar://problem/59227960>

[Truitt Savell]

Found this test that also looks like it started at the same time:
imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html

https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fweb-animations%2Ftiming-model%2Ftimelines%2Fupdate-and-send-events-replacement.html

[Ryosuke Niwa]

e.g.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000121d964e1 WTF::HashTable<WTF::ListHashSetNode<WTF::RefPtr<WebCore::WebAnimation, WTF::DumbPtrTraits<WebCore::WebAnimation> > >*, WTF::ListHashSetNode<WTF::RefPtr<WebCore::WebAnimation, WTF::DumbPtrTraits<WebCore::WebAnimation> > >*, WTF::IdentityExtractor, WTF::ListHashSetNodeHashFunctions<WTF::PtrHash<WTF::RefPtr<WebCore::WebAnimation, WTF::DumbPtrTraits<WebCore::WebAnimation> > > >, WTF::HashTraits<WTF::ListHashSetNode<WTF::RefPtr<WebCore::WebAnimation, WTF::DumbPtrTraits<WebCore::WebAnimation> > >*>, WTF::HashTraits<WTF::ListHashSetNode<WTF::RefPtr<WebCore::WebAnimation, WTF::DumbPtrTraits<WebCore::WebAnimation> > >*> >::keyCount() const + 33 (HashTable.h:544)
1   com.apple.WebCore             	0x0000000121dd2e85 WTF::HashTable<WTF::ListHashSetNode<WTF::RefPtr<WebCore::WebAnimation, WTF::DumbPtrTraits<WebCore::WebAnimation> > >*, WTF::ListHashSetNode<WTF::RefPtr<WebCore::WebAnimation, WTF::DumbPtrTraits<WebCore::WebAnimation> > >*, WTF::IdentityExtractor, WTF::ListHashSetNodeHashFunctions<WTF::PtrHash<WTF::RefPtr<WebCore::WebAnimation, WTF::DumbPtrTraits<WebCore::WebAnimation> > > >, WTF::HashTraits<WTF::ListHashSetNode<WTF::RefPtr<WebCore::WebAnimation, WTF::DumbPtrTraits<WebCore::WebAnimation> > >*>, WTF::HashTraits<WTF::ListHashSetNode<WTF::RefPtr<WebCore::WebAnimation, WTF::DumbPtrTraits<WebCore::WebAnimation> > >*> >::isEmpty() const + 21 (HashTable.h:406)
2   com.apple.WebCore             	0x0000000121d859d5 WTF::ListHashSet<WTF::RefPtr<WebCore::WebAnimation, WTF::DumbPtrTraits<WebCore::WebAnimation> >, WTF::PtrHash<WTF::RefPtr<WebCore::WebAnimation, WTF::DumbPtrTraits<WebCore::WebAnimation> > > >::isEmpty() const + 21 (ListHashSet.h:389)
3   com.apple.WebCore             	0x0000000121d85869 WebCore::DocumentTimeline::detachFromDocument() + 137 (DocumentTimeline.cpp:90)
4   com.apple.WebCore             	0x00000001223b47f6 WebCore::Document::prepareForDestruction() + 1638 (Document.cpp:2582)
5   com.apple.WebCore             	0x000000012315c8d0 WebCore::Frame::setView(WTF::RefPtr<WebCore::FrameView, WTF::DumbPtrTraits<WebCore::FrameView> >&&) + 192
6   com.apple.WebKitLegacy        	0x0000000111c1abc5 WebFrameLoaderClient::transitionToCommittedForNewPage() + 837 (WebFrameLoaderClient.mm:1467)
7   com.apple.WebCore             	0x0000000122f616f7 WebCore::FrameLoader::transitionToCommitted(WebCore::CachedPage*) + 1543 (FrameLoader.cpp:2225)
8   com.apple.WebCore             	0x0000000122f60732 WebCore::FrameLoader::commitProvisionalLoad() + 1122 (FrameLoader.cpp:2045)
9   com.apple.WebCore             	0x0000000122eef41c WebCore::DocumentLoader::commitIfReady() + 60 (DocumentLoader.cpp:369)
10  com.apple.WebCore             	0x0000000122eef932 WebCore::DocumentLoader::finishedLoading() + 306 (DocumentLoader.cpp:434)
11  com.apple.WebCore             	0x0000000122efa51e WebCore::DocumentLoader::maybeLoadEmpty() + 1118 (DocumentLoader.cpp:1790)
12  com.apple.WebCore             	0x0000000122efa6a3 WebCore::DocumentLoader::startLoadingMainResource() + 355 (DocumentLoader.cpp:1803)
13  com.apple.WebCore             	0x0000000122f85b27 WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WebCore::FormState*, WebCore::NavigationPolicyDecision, WebCore::AllowNavigationToInvalidURL)::$_11::operator()() + 951
14  com.apple.WebCore             	0x0000000122f856d9 WTF::Detail::CallableWrapper<WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WebCore::FormState*, WebCore::NavigationPolicyDecision, WebCore::AllowNavigationToInvalidURL)::$_11, void>::call() + 25 (Function.h:52)
15  com.apple.WebCore             	0x000000011fba714a WTF::Function<void ()>::operator()() const + 138 (Function.h:84)
16  com.apple.WebCore             	0x000000011fc160ba WTF::CompletionHandler<void ()>::operator()() + 250 (CompletionHandler.h:62)
17  com.apple.WebCore             	0x0000000122f5e17b WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WebCore::FormState*, WebCore::NavigationPolicyDecision, WebCore::AllowNavigationToInvalidURL) + 2603 (FrameLoader.cpp:3540)
18  com.apple.WebCore             	0x0000000122f82f44 WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::RefPtr<WebCore::FormState, WTF::DumbPtrTraits<WebCore::FormState> >&&, WebCore::AllowNavigationToInvalidURL, WebCore::ShouldTreatAsContinuingLoad, WTF::CompletionHandler<void ()>&&)::$_8::operator()(WebCore::ResourceRequest const&, WTF::WeakPtr<WebCore::FormState>&&, WebCore::NavigationPolicyDecision) + 100 (FrameLoader.cpp:1645)
19  com.apple.WebCore             	0x0000000122f82e0c WTF::Detail::CallableWrapper<WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::RefPtr<WebCore::FormState, WTF::DumbPtrTraits<WebCore::FormState> >&&, WebCore::AllowNavigationToInvalidURL, WebCore::ShouldTreatAsContinuingLoad, WTF::CompletionHandler<void ()>&&)::$_8, void, WebCore::ResourceRequest&&, WTF::WeakPtr<WebCore::FormState>&&, WebCore::NavigationPolicyDecision>::call(WebCore::ResourceRequest&&, WTF::WeakPtr<WebCore::FormState>&&, WebCore::NavigationPolicyDecision) + 92 (Function.h:52)
20  com.apple.WebCore             	0x0000000122fbce60 WTF::Function<void (WebCore::ResourceRequest&&, WTF::WeakPtr<WebCore::FormState>&&, WebCore::NavigationPolicyDecision)>::operator()(WebCore::ResourceRequest&&, WTF::WeakPtr<WebCore::FormState>&&, WebCore::NavigationPolicyDecision) const + 208 (Function.h:84)
21  com.apple.WebCore             	0x0000000122faf6c5 WTF::CompletionHandler<void (WebCore::ResourceRequest&&, WTF::WeakPtr<WebCore::FormState>&&, WebCore::NavigationPolicyDecision)>::operator()(WebCore::ResourceRequest&&, WTF::WeakPtr<WebCore::FormState>&&, WebCore::NavigationPolicyDecision) + 309 (CompletionHandler.h:62)
22  com.apple.WebCore             	0x0000000122fae8ff WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest&&, WebCore::ResourceResponse const&, WebCore::DocumentLoader*, WTF::RefPtr<WebCore::FormState, WTF::DumbPtrTraits<WebCore::FormState> >&&, WTF::CompletionHandler<void (WebCore::ResourceRequest&&, WTF::WeakPtr<WebCore::FormState>&&, WebCore::NavigationPolicyDecision)>&&, WebCore::PolicyDecisionMode) + 687 (PolicyChecker.cpp:131)
23  com.apple.WebCore             	0x0000000122f5d2c3 WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::RefPtr<WebCore::FormState, WTF::DumbPtrTraits<WebCore::FormState> >&&, WebCore::AllowNavigationToInvalidURL, WebCore::ShouldTreatAsContinuingLoad, WTF::CompletionHandler<void ()>&&) + 3523 (FrameLoader.cpp:1644)
24  com.apple.WebCore             	0x0000000122f582a6 WebCore::FrameLoader::load(WebCore::DocumentLoader&) + 886 (FrameLoader.cpp:1554)
25  com.apple.WebCore             	0x0000000122f5bf18 WebCore::FrameLoader::load(WebCore::FrameLoadRequest&&) + 1512 (FrameLoader.cpp:1496)
26  com.apple.WebKitLegacy        	0x0000000111c36c21 -[WebFrame loadRequest:] + 673 (WebFrame.mm:2490)
27  DumpRenderTree                	0x0000000106516e15 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 4613 (DumpRenderTree.mm:2174)
28  DumpRenderTree                	0x0000000106515b6a runTestingServerLoop() + 218 (DumpRenderTree.mm:1229)
29  DumpRenderTree                	0x00000001065152a3 dumpRenderTree(int, char const**) + 611 (DumpRenderTree.mm:1344)
30  DumpRenderTree                	0x000000010651735d DumpRenderTreeMain(int, char const**) + 109 (DumpRenderTree.mm:1463)
31  DumpRenderTree                	0x00000001065a5032 main + 34 (DumpRenderTreeMain.mm:34)
32  libdyld.dylib                 	0x00007fff728be7fd start + 1

[Antoine Quint]

Test doesn't crash when run in isolation, investigating if previous tests make it crash based on test runs when the crash happens.

[Antoine Quint]

This command reproduces the crash:

    run-webkit-tests --debug --child-processes=1 imported/w3c/web-platform-tests/web-animations/timing-model/timelines/document-timelines.html imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events.html

[Antoine Quint]

Created attachment 390066 [details]
Patch

[youenn fablet]

Comment on attachment 390066 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390066&action=review

> Source/WebCore/dom/Document.cpp:2582
> +    Vector<RefPtr<DocumentTimeline>> timelines;

How about Vector<Ref<>>?

> Source/WebCore/dom/Document.cpp:2584
> +        timelines.append(&timeline);

reserveInitialCapacity/uncheckedAppend.
But we should really have a WeakHashSet -> Vector routine.

> Source/WebCore/dom/Document.cpp:2586
> +        timeline->detachFromDocument();

I am not clear why we need to take a strong reference. Can you detail why in the change log?

Also, can we do something like (without the vector creation):
for (auto& timeline : std::exchange(timelines, { }))
    timeline->detachFromDocument();

[youenn fablet]

Comment on attachment 390066 [details]
Patch

After some discussion with Antoine, it seems we should probably protect 'this' in DocumentTimeline::detachFromDocument.

[Antoine Quint]

Created attachment 390072 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 390072 [details]
Patch

Clearing flags on attachment: 390072

Committed r256017: <https://trac.webkit.org/changeset/256017>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

*** Bug 207335 has been marked as a duplicate of this bug. ***