Attachment 341647 Details for Bug 185284 – WIP3

[patch] WIP3

delay-cancel-checkcomplete-3.patch (text/plain), 13.06 KB, created by Ryosuke Niwa on 2018-05-31 00:39:33 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-05-31 00:39:33 PDT

Size: 13.06 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 232336)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,46 @@
>+2018-05-30  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Need a short description (OOPS!).
>+        Need the bug URL (OOPS!).
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        No new tests (OOPS!).
>+
>+        * css/CSSFontSelector.cpp:
>+        (WebCore::CSSFontSelector::beginLoadTimerFired): Removed superfluous call to checkLoadComplete since
>+        cachedResourceLoader's loadDone would call checkLoadComplete anyway.
>+        * html/HTMLFrameOwnerElement.cpp:
>+        (WebCore::HTMLFrameOwnerElement::disconnectContentFrame): Removed the misleading comment added in r140090.
>+        Firefox DOES indeed fire unload event in the content document of a removed frame. While this comment made
>+        it sound like this function isn't called when a frame is removed from the tree when in reality we simply
>+        remove a frame prior to removing the node via disconnectSubframesIfNeeded.
>+        * loader/DocumentLoader.cpp:
>+        (WebCore::DocumentLoader::removeSubresourceLoader):
>+        * loader/DocumentLoader.h:
>+        * loader/FrameLoader.cpp:
>+        (WebCore::FrameLoader::loadDone):
>+        (WebCore::FrameLoader::subresourceLoadDone):
>+        (WebCore::FrameLoader::stopAllLoaders): Removed the code to stop m_checkTimer introduced in r53655.
>+        Stopping the timer here would prevent FrameLoader::frameDetached to detect the case when stopping the loader
>+        scheduled a load completion check. Also stopping this timer without clearing the corresponding booleans:
>+        m_checkingLoadCompleteForDetachment and m_checkingLoadCompleteForDetachment is problematic. The assertion
>+        r53655 addressed is now addressed by explicitly checking & clearing the timer in frameDetached.
>+        (WebCore::FrameLoader::stopForUserCancel):
>+        (WebCore::FrameLoader::checkLoadCompleteForThisFrame):
>+        (WebCore::FrameLoader::frameDetached):
>+        * loader/FrameLoader.h:
>+        * loader/FrameLoaderTypes.h:
>+        * loader/SubresourceLoader.cpp:
>+        (WebCore::SubresourceLoader::didFinishLoading):
>+        (WebCore::SubresourceLoader::didFail):
>+        (WebCore::SubresourceLoader::didCancel):
>+        (WebCore::SubresourceLoader::notifyDone):
>+        * loader/SubresourceLoader.h:
>+        * loader/cache/CachedResourceLoader.cpp:
>+        (WebCore::CachedResourceLoader::loadDone):
>+        * loader/cache/CachedResourceLoader.h:
>+
> 2018-05-28  Darin Adler  <darin@apple.com>
> 
>         Straighten out HTMLInputElement attribute handling
>Index: Source/WebCore/css/CSSFontSelector.cpp
>===================================================================
>--- Source/WebCore/css/CSSFontSelector.cpp	(revision 232166)
>+++ Source/WebCore/css/CSSFontSelector.cpp	(working copy)
>@@ -365,11 +365,7 @@
>         cachedResourceLoader.decrementRequestCount(*fontHandle);
>     }
>     // Ensure that if the request count reaches zero, the frame loader will know about it.
>-    cachedResourceLoader.loadDone();
>-    // New font loads may be triggered by layout after the document load is complete but before we have dispatched
>-    // didFinishLoading for the frame. Make sure the delegate is always dispatched by checking explicitly.
>-    if (m_document && m_document->frame())
>-        m_document->frame()->loader().checkLoadComplete();
>+    cachedResourceLoader.loadDone(LoadCompletionType::Finish);
> }
> 
> 
>Index: Source/WebCore/html/HTMLFrameOwnerElement.cpp
>===================================================================
>--- Source/WebCore/html/HTMLFrameOwnerElement.cpp	(revision 232166)
>+++ Source/WebCore/html/HTMLFrameOwnerElement.cpp	(working copy)
>@@ -77,10 +77,6 @@
> 
> void HTMLFrameOwnerElement::disconnectContentFrame()
> {
>-    // FIXME: Currently we don't do this in removedFrom because this causes an
>-    // unload event in the subframe which could execute script that could then
>-    // reach up into this document and then attempt to look back down. We should
>-    // see if this behavior is really needed as Gecko does not allow this.
>     if (RefPtr<Frame> frame = contentFrame()) {
>         Ref<Frame> protect(*frame);
>         frame->loader().frameDetached();
>Index: Source/WebCore/loader/DocumentLoader.cpp
>===================================================================
>--- Source/WebCore/loader/DocumentLoader.cpp	(revision 232166)
>+++ Source/WebCore/loader/DocumentLoader.cpp	(working copy)
>@@ -1618,7 +1618,7 @@
>     m_subresourceLoaders.add(loader->identifier(), loader);
> }
> 
>-void DocumentLoader::removeSubresourceLoader(ResourceLoader* loader)
>+void DocumentLoader::removeSubresourceLoader(LoadCompletionType type, ResourceLoader* loader)
> {
>     ASSERT(loader->identifier());
> 
>@@ -1626,7 +1626,7 @@
>         return;
>     checkLoadComplete();
>     if (Frame* frame = m_frame)
>-        frame->loader().checkLoadComplete();
>+        frame->loader().subresourceLoadDone(type);
> }
> 
> void DocumentLoader::addPlugInStreamLoader(ResourceLoader& loader)
>Index: Source/WebCore/loader/DocumentLoader.h
>===================================================================
>--- Source/WebCore/loader/DocumentLoader.h	(revision 232166)
>+++ Source/WebCore/loader/DocumentLoader.h	(working copy)
>@@ -269,7 +269,7 @@
>     void setPopUpPolicy(PopUpPolicy popUpPolicy) { m_popUpPolicy = popUpPolicy; }
> 
>     void addSubresourceLoader(ResourceLoader*);
>-    void removeSubresourceLoader(ResourceLoader*);
>+    void removeSubresourceLoader(LoadCompletionType, ResourceLoader*);
>     void addPlugInStreamLoader(ResourceLoader&);
>     void removePlugInStreamLoader(ResourceLoader&);
> 
>Index: Source/WebCore/loader/FrameLoader.cpp
>===================================================================
>--- Source/WebCore/loader/FrameLoader.cpp	(revision 232166)
>+++ Source/WebCore/loader/FrameLoader.cpp	(working copy)
>@@ -783,11 +783,22 @@
>     m_frame.view()->restoreScrollbar();
> }
> 
>-void FrameLoader::loadDone()
>+void FrameLoader::loadDone(LoadCompletionType type)
> {
>-    checkCompleted();
>+    if (type == LoadCompletionType::Finish)
>+        checkCompleted();
>+    else
>+        scheduleCheckCompleted();
> }
> 
>+void FrameLoader::subresourceLoadDone(LoadCompletionType type)
>+{
>+    if (type == LoadCompletionType::Finish)
>+        checkLoadComplete();
>+    else
>+        scheduleCheckLoadComplete();
>+}
>+
> bool FrameLoader::allChildrenAreComplete() const
> {
>     for (Frame* child = m_frame.tree().firstChild(); child; child = child->tree().nextSibling()) {
>@@ -1761,7 +1772,7 @@
> 
>     setProvisionalDocumentLoader(nullptr);
> 
>-    m_checkTimer.stop();
>+//    m_checkTimer.stop();
> 
>     m_inStopAllLoaders = false;    
> }
>@@ -1783,8 +1794,10 @@
> 
>     if (deferCheckLoadComplete)
>         scheduleCheckLoadComplete();
>-    else if (m_frame.page())
>+    else if (m_frame.page()) {
>+//        checkCompleted();
>         checkLoadComplete();
>+    }
> }
> 
> DocumentLoader* FrameLoader::activeDocumentLoader() const
>@@ -2392,7 +2405,7 @@
>         
>         case FrameStateCommittedPage: {
>             DocumentLoader* dl = m_documentLoader.get();            
>-            if (!dl || (dl->isLoadingInAPISense() && !dl->isStopping()))
>+            if (!dl || (dl->isLoadingInAPISense() && !dl->isStopping() && !m_checkingLoadCompleteForDetachment))
>                 return;
> 
>             setState(FrameStateComplete);
>@@ -2618,11 +2631,21 @@
>     // Calling stopAllLoaders() can cause the frame to be deallocated, including the frame loader.
>     Ref<Frame> protectedFrame(m_frame);
> 
>-    if (m_frame.document()->pageCacheState() != Document::InPageCache) {
>+    bool documentIsInPageCache = m_frame.document()->pageCacheState() == Document::InPageCache;
>+    if (!documentIsInPageCache)
>         stopAllLoaders();
>-        m_frame.document()->stopActiveDOMObjects();
>+
>+    // Stopping all loaders may have scheduled a check.
>+    if (m_checkTimer.isActive()) {
>+        m_checkTimer.stop();
>+        m_checkingLoadCompleteForDetachment = true;
>+        checkTimerFired();
>+        m_checkingLoadCompleteForDetachment = false;
>     }
> 
>+    if (!documentIsInPageCache)
>+        m_frame.document()->stopActiveDOMObjects();
>+
>     detachFromParent();
> }
> 
>Index: Source/WebCore/loader/FrameLoader.h
>===================================================================
>--- Source/WebCore/loader/FrameLoader.h	(revision 232166)
>+++ Source/WebCore/loader/FrameLoader.h	(working copy)
>@@ -253,7 +253,8 @@
> 
>     void setOutgoingReferrer(const URL&);
> 
>-    void loadDone();
>+    void loadDone(LoadCompletionType);
>+    void subresourceLoadDone(LoadCompletionType);
>     void finishedParsing();
>     void checkCompleted();
> 
>@@ -467,6 +468,8 @@
>     bool m_isStrictRawResourceValidationPolicyDisabledForTesting { false };
>     bool m_currentLoadShouldCheckNavigationPolicy { true };
> 
>+    bool m_checkingLoadCompleteForDetachment { false };
>+
>     URL m_previousURL;
>     RefPtr<HistoryItem> m_requestedHistoryItem;
> };
>Index: Source/WebCore/loader/FrameLoaderTypes.h
>===================================================================
>--- Source/WebCore/loader/FrameLoaderTypes.h	(revision 232166)
>+++ Source/WebCore/loader/FrameLoaderTypes.h	(working copy)
>@@ -155,6 +155,11 @@
>     bool isSystemPreview { false };
> };
> 
>+enum class LoadCompletionType {
>+    Finish,
>+    Cancel
>+};
>+
> } // namespace WebCore
> 
> namespace WTF {
>Index: Source/WebCore/loader/SubresourceLoader.cpp
>===================================================================
>--- Source/WebCore/loader/SubresourceLoader.cpp	(revision 232166)
>+++ Source/WebCore/loader/SubresourceLoader.cpp	(working copy)
>@@ -647,7 +647,7 @@
>     m_resource->finish();
>     ASSERT(!reachedTerminalState());
>     didFinishLoadingOnePart(networkLoadMetrics);
>-    notifyDone();
>+    notifyDone(LoadCompletionType::Finish);
> 
>     if (reachedTerminalState())
>         return;
>@@ -683,7 +683,7 @@
>         MemoryCache::singleton().remove(*m_resource);
>     m_resource->error(CachedResource::LoadError);
>     cleanupForError(error);
>-    notifyDone();
>+    notifyDone(LoadCompletionType::Cancel);
>     if (reachedTerminalState())
>         return;
>     releaseResources();
>@@ -725,7 +725,7 @@
>         tracePoint(SubresourceLoadDidEnd);
> 
>     m_resource->cancelLoad();
>-    notifyDone();
>+    notifyDone(LoadCompletionType::Cancel);
> }
> 
> void SubresourceLoader::didRetrieveDerivedDataFromCache(const String& type, SharedBuffer& buffer)
>@@ -735,20 +735,21 @@
>     m_resource->didRetrieveDerivedDataFromCache(type, buffer);
> }
> 
>-void SubresourceLoader::notifyDone()
>+void SubresourceLoader::notifyDone(LoadCompletionType type)
> {
>     if (reachedTerminalState())
>         return;
> 
>     m_requestCountTracker = std::nullopt;
>+    bool shouldPerformPostLoadActions = true;
> #if PLATFORM(IOS)
>-    m_documentLoader->cachedResourceLoader().loadDone(m_state != CancelledWhileInitializing);
>-#else
>-    m_documentLoader->cachedResourceLoader().loadDone();
>+    if (m_state == CancelledWhileInitializing)
>+        shouldPerformPostLoadActions = false;
> #endif
>+    m_documentLoader->cachedResourceLoader().loadDone(type, shouldPerformPostLoadActions);
>     if (reachedTerminalState())
>         return;
>-    m_documentLoader->removeSubresourceLoader(this);
>+    m_documentLoader->removeSubresourceLoader(type, this);
> }
> 
> void SubresourceLoader::releaseResources()
>Index: Source/WebCore/loader/SubresourceLoader.h
>===================================================================
>--- Source/WebCore/loader/SubresourceLoader.h	(revision 232166)
>+++ Source/WebCore/loader/SubresourceLoader.h	(working copy)
>@@ -95,7 +95,7 @@
> 
>     void didReceiveDataOrBuffer(const char*, int, RefPtr<SharedBuffer>&&, long long encodedDataLength, DataPayloadType);
> 
>-    void notifyDone();
>+    void notifyDone(LoadCompletionType);
> 
>     void reportResourceTiming(const NetworkLoadMetrics&);
> 
>Index: Source/WebCore/loader/cache/CachedResourceLoader.cpp
>===================================================================
>--- Source/WebCore/loader/cache/CachedResourceLoader.cpp	(revision 232166)
>+++ Source/WebCore/loader/cache/CachedResourceLoader.cpp	(working copy)
>@@ -1299,13 +1299,13 @@
>     m_documentResources.remove(resource.url());
> }
> 
>-void CachedResourceLoader::loadDone(bool shouldPerformPostLoadActions)
>+void CachedResourceLoader::loadDone(LoadCompletionType type, bool shouldPerformPostLoadActions)
> {
>     RefPtr<DocumentLoader> protectDocumentLoader(m_documentLoader);
>     RefPtr<Document> protectDocument(m_document);
> 
>     if (frame())
>-        frame()->loader().loadDone();
>+        frame()->loader().loadDone(type);
> 
>     if (shouldPerformPostLoadActions)
>         performPostLoadActions();
>Index: Source/WebCore/loader/cache/CachedResourceLoader.h
>===================================================================
>--- Source/WebCore/loader/cache/CachedResourceLoader.h	(revision 232166)
>+++ Source/WebCore/loader/cache/CachedResourceLoader.h	(working copy)
>@@ -132,7 +132,7 @@
> 
>     void removeCachedResource(CachedResource&);
> 
>-    void loadDone(bool shouldPerformPostLoadActions = true);
>+    void loadDone(LoadCompletionType, bool shouldPerformPostLoadActions = true);
> 
>     WEBCORE_EXPORT void garbageCollectDocumentResources();
>

Actions: View | Formatted Diff | Diff

Attachments on bug 185284: 341647 | 341652 | 341653 | 341656 | 341678 | 341737 | 341741 | 341743 | 341745 | 341786