ResourceLoader::cancel() can end up synchronously firing a load event via FrameLoader::checkCompleted(). This is not safe. We need to scheduleCheckCompleted instead. e.g. 0 WebCore 0x000000018af6e574 WebCore::ScriptController::canExecuteScripts(WebCore::ReasonForCallingCanExecuteScripts) + 552 (Source/WebCore/bindings/js/ScriptController.cpp:672) 1 WebCore 0x000000018af6e380 WebCore::ScriptController::canExecuteScripts(WebCore::ReasonForCallingCanExecuteScripts) + 52 (Source/WebCore/bindings/js/ScriptController.cpp:672) 2 WebCore 0x000000018b9a0b0c WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&) + 316 (Source/WebCore/bindings/js/JSEventListener.cpp:113) 3 WebCore 0x000000018bbdd674 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>) + 760 (Source/WebCore/dom/EventTarget.cpp:289) 4 WebCore 0x000000018bbd922c WebCore::EventTarget::fireEventListeners(WebCore::Event&) + 596 (Source/WebCore/dom/EventTarget.cpp:231) 5 WebCore 0x000000018bf12140 WebCore::DOMWindow::dispatchEvent(WebCore::Event&, WebCore::EventTarget*) + 284 (Source/WebCore/page/DOMWindow.cpp:2053) 6 WebCore 0x000000018aff6b78 WebCore::DOMWindow::dispatchLoadEvent() + 160 (Source/WebCore/page/DOMWindow.cpp:2005) 7 WebCore 0x000000018afb3218 WebCore::Document::implicitClose() + 440 (Source/WebCore/dom/Document.cpp:4326) 8 WebCore 0x000000018afb282c WebCore::FrameLoader::checkCompleted() + 476 (Source/WebCore/loader/FrameLoader.cpp:910) 9 WebCore 0x000000018bef2e80 WebCore::CachedResourceLoader::loadDone(bool) + 84 (Source/WebCore/loader/cache/CachedResourceLoader.cpp:1287) 10 WebCore 0x000000018afe401c WebCore::SubresourceLoader::didCancel(WebCore::ResourceError const&) + 128 (Source/WebCore/loader/SubresourceLoader.cpp:699) 11 WebCore 0x000000018afe39d4 WebCore::ResourceLoader::cancel(WebCore::ResourceError const&) + 492 (Source/WebCore/loader/ResourceLoader.cpp:642) 12 WebCore 0x000000018afe3744 WebCore::ResourceLoader::cancel() + 64 (Source/WebCore/loader/ResourceLoader.cpp:598) 13 WebCore 0x000000018beec9b4 WebCore::CachedResource::removeClient(WebCore::CachedResourceClient&) + 264 (Source/WebCore/loader/cache/CachedResource.cpp:573)
<rdar://problem/39994507>
Created attachment 341647 [details] WIP3
Attachment 341647 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 341647 [details] WIP3 Attachment 341647 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/7887380 Number of test failures exceeded the failure limit.
Created attachment 341652 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 341647 [details] WIP3 Attachment 341647 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/7887358 Number of test failures exceeded the failure limit.
Created attachment 341653 [details] Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 341647 [details] WIP3 Attachment 341647 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7889847 New failing tests: http/tests/xmlhttprequest/reentrant-cancel.html
Created attachment 341656 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 341647 [details] WIP3 Attachment 341647 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7901449 New failing tests: http/tests/xmlhttprequest/reentrant-cancel.html
Created attachment 341678 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 341647 [details] WIP3 Attachment 341647 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7918155 New failing tests: http/tests/xmlhttprequest/reentrant-cancel.html http/tests/loading/unfinished-load-back-to-cached-page-callbacks.html
Created attachment 341737 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 341741 [details] Fixes the bug
Created attachment 341743 [details] Fixes the bug
Comment on attachment 341743 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=341743&action=review > Source/WebCore/loader/FrameLoader.cpp:799 > + if (type == LoadCompletionType::Finish) > + checkLoadComplete(); > + else > + scheduleCheckLoadComplete(); Could we in principle always do these asynchronously? > Source/WebCore/loader/FrameLoader.cpp:896 > -void FrameLoader::checkTimerFired() > +void FrameLoader::checkCompletenessNow() I'd slightly prefer keeping the timerFired function and just calling the new function from that. Canonical naming makes it easier to find what happens when a timer fires. > Source/WebCore/loader/FrameLoader.cpp:1782 > + if (m_checkTimer.isActive()) { You could early return instead. > Source/WebCore/loader/FrameLoader.cpp:2417 > - DocumentLoader* dl = m_documentLoader.get(); > - if (!dl || (dl->isLoadingInAPISense() && !dl->isStopping())) > + DocumentLoader* dl = m_documentLoader.get(); > + if (!dl || (dl->isLoadingInAPISense() && !dl->isStopping() && !m_checkingLoadCompleteForDetachment)) > return; The awkwardly named local ('dl') seem unnecessary. Also this would read better as two ifs, !m_documentLoader and the rest. > Source/WebCore/loader/FrameLoader.h:141 > + void stopAllLoadersAndCheckcompleteness(); Capitalization: stopAllLoadersAndCheckCompleteness > Source/WebCore/loader/cache/CachedResourceLoader.cpp:1302 > +void CachedResourceLoader::loadDone(LoadCompletionType type, bool shouldPerformPostLoadActions) I wonder if shouldPerformPostLoadActions could be merged into LoadCompletionType? It is set when m_state is CancelledWhileInitializing which sounds like a load completion type... Could we at least ASSERT(shouldPerformPostLoadActions || type == LoadCompletionType::Cancel)?
Comment on attachment 341743 [details] Fixes the bug Attachment 341743 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7921848 New failing tests: editing/pasteboard/drag-image-to-contenteditable-in-iframe.html
Created attachment 341745 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
(In reply to Build Bot from comment #17) > Comment on attachment 341743 [details] > Fixes the bug > > Attachment 341743 [details] did not pass ios-sim-ews (ios-simulator-wk2): > Output: http://webkit-queues.webkit.org/results/7921848 > > New failing tests: > editing/pasteboard/drag-image-to-contenteditable-in-iframe.html Huh, weird that this test wasn't already skipped since drag & drop isn't supported by WTR in the first place.
(In reply to Antti Koivisto from comment #16) > Comment on attachment 341743 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=341743&action=review > > > Source/WebCore/loader/FrameLoader.cpp:799 > > + if (type == LoadCompletionType::Finish) > > + checkLoadComplete(); > > + else > > + scheduleCheckLoadComplete(); > > Could we in principle always do these asynchronously? Possibly. I think there are cases in which load has to happen synchronously for compatibility reasons but I haven't studied those in detail yet. > > Source/WebCore/loader/FrameLoader.cpp:896 > > -void FrameLoader::checkTimerFired() > > +void FrameLoader::checkCompletenessNow() > > I'd slightly prefer keeping the timerFired function and just calling the new > function from that. Canonical naming makes it easier to find what happens > when a timer fires. Sure, fixed. > > Source/WebCore/loader/FrameLoader.cpp:1782 > > + if (m_checkTimer.isActive()) { > > You could early return instead. Fixed. > > Source/WebCore/loader/FrameLoader.cpp:2417 > > - DocumentLoader* dl = m_documentLoader.get(); > > - if (!dl || (dl->isLoadingInAPISense() && !dl->isStopping())) > > + DocumentLoader* dl = m_documentLoader.get(); > > + if (!dl || (dl->isLoadingInAPISense() && !dl->isStopping() && !m_checkingLoadCompleteForDetachment)) > > return; > > The awkwardly named local ('dl') seem unnecessary. Also this would read > better as two ifs, !m_documentLoader and the rest. Sure, done that. > > Source/WebCore/loader/FrameLoader.h:141 > > + void stopAllLoadersAndCheckcompleteness(); > > Capitalization: stopAllLoadersAndCheckCompleteness Oops, fixed. Not sure what happened there. > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:1302 > > +void CachedResourceLoader::loadDone(LoadCompletionType type, bool shouldPerformPostLoadActions) > > I wonder if shouldPerformPostLoadActions could be merged into > LoadCompletionType? It is set when m_state is CancelledWhileInitializing > which sounds like a load completion type... > > Could we at least ASSERT(shouldPerformPostLoadActions || type == > LoadCompletionType::Cancel)? Hm... but FrameLoader::loadDone isn't going to differentiate that state so it's a bit weird to have that enum value. I think an argument can be made to make CachedResourceLoader::loadDone take a different enum which has three values. Just added an assertion for now.
Created attachment 341786 [details] Patch for landing
Comment on attachment 341786 [details] Patch for landing Wait for EWS.
Comment on attachment 341786 [details] Patch for landing Clearing flags on attachment: 341786 Committed r232419: <https://trac.webkit.org/changeset/232419>
All reviewed patches have been landed. Closing bug.
Looks like we are getting an API test failure on macOS Wk1, WK2 and iOS11 after this latest patch: sample results: TestWebKitAPI.CancelLoading.CancelFontSubresource https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/9752/steps/run-api-tests/logs/stdio
(In reply to David Fenton from comment #25) > Looks like we are getting an API test failure on macOS Wk1, WK2 and iOS11 > after this latest patch: > > sample results: > > TestWebKitAPI.CancelLoading.CancelFontSubresource > > https://build.webkit.org/builders/ > Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/9752/steps/run-api-tests/ > logs/stdio Looking into this.
Committed r232433: <https://trac.webkit.org/changeset/232433>
<rdar://problem/37591394> is the correct radar associated with this bug.
(In reply to David Fenton from comment #25) > Looks like we are getting an API test failure on macOS Wk1, WK2 and iOS11 > after this latest patch: > > sample results: > > TestWebKitAPI.CancelLoading.CancelFontSubresource > > https://build.webkit.org/builders/ > Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/9752/steps/run-api-tests/ > logs/stdio https://trac.webkit.org/changeset/232433 should have fixed this.
(In reply to Ryosuke Niwa from comment #29) > (In reply to David Fenton from comment #25) > > Looks like we are getting an API test failure on macOS Wk1, WK2 and iOS11 > > after this latest patch: > > > > sample results: > > > > TestWebKitAPI.CancelLoading.CancelFontSubresource > > > > https://build.webkit.org/builders/ > > Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/9752/steps/run-api-tests/ > > logs/stdio > > https://trac.webkit.org/changeset/232433 should have fixed this. many thanks!
Ryosuke, there is this loose end in FrameLoader::checkCompleted: #if ENABLE(VIDEO) // FIXME: Remove this code once https://webkit.org/b/185284 is fixed. if (HTMLMediaElement::isRunningDestructor()) { ASSERT_NOT_REACHED(); scheduleCheckCompleted(); return; } #endif (That's this bug.)
Also in HTMLMediaElement.cpp // FIXME: Remove this code once https://webkit.org/b/185284 is fixed. static unsigned s_destructorCount = 0;
Taking care of that code removal in https://bugs.webkit.org/show_bug.cgi?id=203600.