Bug 185284 – ResourceLoader::cancel() shouldn't synchronously fire load event on document

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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)

[Radar WebKit Bug Importer]

<rdar://problem/39994507>

[Ryosuke Niwa]

Created attachment 341647 [details]
WIP3

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ryosuke Niwa]

Created attachment 341741 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 341743 [details]
Fixes the bug

[Antti Koivisto]

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)?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 341786 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 341786 [details]
Patch for landing

Wait for EWS.

[WebKit Commit Bot]

Comment on attachment 341786 [details]
Patch for landing

Clearing flags on attachment: 341786

Committed r232419: <https://trac.webkit.org/changeset/232419>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dawei Fenton (:realdawei)]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Committed r232433: <https://trac.webkit.org/changeset/232433>

[Ryosuke Niwa]

<rdar://problem/37591394> is the correct radar associated with this bug.

[Ryosuke Niwa]

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

[Dawei Fenton (:realdawei)]

(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!

[Michael Catanzaro]

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

[Peng Liu]

Also in HTMLMediaElement.cpp

// FIXME: Remove this code once https://webkit.org/b/185284 is fixed.
static unsigned s_destructorCount = 0;

[Ryosuke Niwa]

Taking care of that code removal in https://bugs.webkit.org/show_bug.cgi?id=203600.