Attachment 343765 Details for Bug 187121 – Patch

[patch] Patch

bug-187121-20180627161239.patch (text/plain), 6.21 KB, created by Brent Fulgham on 2018-06-27 16:12:40 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Brent Fulgham

Created: 2018-06-27 16:12:40 PDT

Size: 6.21 KB

patch

obsolete

>Subversion Revision: 233277
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index e99eb0ba5e571f0178eeb399330954fb44b4988a..743951d0d48467b99f93a14a7a9bb493126c6794 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,23 @@
>+2018-06-27  Brent Fulgham  <bfulgham@apple.com>
>+
>+        WebKitLegacy: Can trigger recursive loads
>+        https://bugs.webkit.org/show_bug.cgi?id=187121
>+        <rdar://problem/41259430>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        While investigating Bug 187008 I found that some WebKitLegacy clients trigger recursive
>+        loads while cancelling the loading of web content into a WebView.
>+
>+        FrameLoader::continueLoadAfterNavigationPolicy gets entered with a nullptr Policy Document
>+        Loader as well as a nullptr Provisional Document Loader. If we continue in this state,
>+        we hit a ton of assertions, and eventually crash with a nullptr exception. If we return
>+        early, the cancel and alternate page load complete properly.
>+
>+        * loader/FrameLoader.cpp:
>+        (WebCore::FrameLoader::continueLoadAfterNavigationPolicy): Return early if the load has
>+        already been cancelled.
>+
> 2018-06-27  Youenn Fablet  <youenn@apple.com>
> 
>         NetworkLoadChecker should not need to hard ref NetworkConnectionToWebProcess
>diff --git a/Source/WebKitLegacy/mac/ChangeLog b/Source/WebKitLegacy/mac/ChangeLog
>index 61a241964f2eeed98eaf04b921da951c03a6e98f..27f0c4c4edd31b760e8f5915084a66fd379a8870 100644
>--- a/Source/WebKitLegacy/mac/ChangeLog
>+++ b/Source/WebKitLegacy/mac/ChangeLog
>@@ -1,3 +1,26 @@
>+2018-06-27  Brent Fulgham  <bfulgham@apple.com>
>+
>+        WebKitLegacy: Can trigger recursive loads
>+        https://bugs.webkit.org/show_bug.cgi?id=187121
>+        <rdar://problem/41259430>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        While investigating Bug 187008 I found that some WebKitLegacy clients trigger recursive
>+        loads while cancelling the loading of web content into a WebView.
>+
>+        WebFrameLoaderClient::dispatchDidStartProvisionalLoad can be re-entered which triggers
>+        a set of assertions and eventually a nullptr dereference. If we keep track of whether we
>+        have started a load on the current client object, and return early in those cases, the
>+        cancel and alternate page load complete properly.
>+
>+        * WebCoreSupport/WebFrameLoaderClient.h:
>+        * WebCoreSupport/WebFrameLoaderClient.mm:
>+        (WebFrameLoaderClient::dispatchDidStartProvisionalLoad): Mark a load as starting.
>+        (WebFrameLoaderClient::dispatchDidCommitLoad): Clear loading state.
>+        (WebFrameLoaderClient::dispatchDidFailProvisionalLoad): Ditto.
>+        (WebFrameLoaderClient::frameLoadCompleted): Ditto.
>+
> 2018-06-26  Eric Carlson  <eric.carlson@apple.com>
> 
>         [Mac] AirPlay picker uses incorrect theme in Dark mode
>diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp
>index 4ee524631672df923543cd4245ea68e011b0f3f1..01053595813ddccf95edccfa19d72ae52483c2b8 100644
>--- a/Source/WebCore/loader/FrameLoader.cpp
>+++ b/Source/WebCore/loader/FrameLoader.cpp
>@@ -3262,6 +3262,10 @@ bool FrameLoader::dispatchBeforeUnloadEvent(Chrome& chrome, FrameLoader* frameLo
> 
> void FrameLoader::continueLoadAfterNavigationPolicy(const ResourceRequest& request, FormState* formState, ShouldContinue shouldContinue, AllowNavigationToInvalidURL allowNavigationToInvalidURL)
> {
>+    // If an alternate page is being loaded in WebKitLegacy, we can reenter this routine without any loaders.
>+    if (!m_policyDocumentLoader && !m_provisionalDocumentLoader)
>+        return;
>+
>     // If we loaded an alternate page to replace an unreachableURL, we'll get in here with a
>     // nil policyDataSource because loading the alternate page will have passed
>     // through this method already, nested; otherwise, policyDataSource should still be set.
>diff --git a/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h b/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h
>index 388d14abf8aa6293c46885bd065b3c420176431a..26f37fead5e6088f2db715953bfa28e779521884 100644
>--- a/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h
>+++ b/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h
>@@ -262,4 +262,5 @@ private:
>     RetainPtr<WebFrame> m_webFrame;
> 
>     WeakObjCPtr<WebFramePolicyListener> m_policyListener;
>+    bool m_alreadyPerformingLoad { false };
> };
>diff --git a/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm b/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm
>index 49870deb59d4b3b5a7d80bfcfb31a6f62c492fb4..b762fa68f231d6bf12607f7df08e5e6c5ffb0d42 100644
>--- a/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm
>+++ b/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm
>@@ -665,8 +665,12 @@ void WebFrameLoaderClient::dispatchWillClose()
> 
> void WebFrameLoaderClient::dispatchDidStartProvisionalLoad()
> {
>+    if (m_alreadyPerformingLoad)
>+        return;
>+
>     ASSERT(!m_webFrame->_private->provisionalURL);
>     m_webFrame->_private->provisionalURL = core(m_webFrame.get())->loader().provisionalDocumentLoader()->url().string();
>+    m_alreadyPerformingLoad = true;
> 
>     WebView *webView = getWebView(m_webFrame.get());
> #if !PLATFORM(IOS)
>@@ -706,6 +710,7 @@ void WebFrameLoaderClient::dispatchDidCommitLoad(std::optional<HasInsecureConten
> 
>     m_webFrame->_private->url = m_webFrame->_private->provisionalURL;
>     m_webFrame->_private->provisionalURL = nullptr;
>+    m_alreadyPerformingLoad = false;
> 
> #if PLATFORM(IOS)
>     [[webView _UIKitDelegateForwarder] webView:webView didCommitLoadForFrame:m_webFrame.get()];
>@@ -718,6 +723,7 @@ void WebFrameLoaderClient::dispatchDidCommitLoad(std::optional<HasInsecureConten
> 
> void WebFrameLoaderClient::dispatchDidFailProvisionalLoad(const ResourceError& error)
> {
>+    m_alreadyPerformingLoad = false;
>     m_webFrame->_private->provisionalURL = nullptr;
> 
>     WebView *webView = getWebView(m_webFrame.get());
>@@ -1231,6 +1237,8 @@ String WebFrameLoaderClient::generatedMIMETypeForURLScheme(const String& URLSche
> 
> void WebFrameLoaderClient::frameLoadCompleted()
> {
>+    m_alreadyPerformingLoad = false;
>+
>     // Note: Can be called multiple times.
> 
>     // See WebFrameLoaderClient::provisionalLoadStarted.

Actions: View | Formatted Diff | Diff

Attachments on bug 187121: 343765 | 343783 | 343934 | 343949