Attachment 349599 Details for Bug 189568 – Patch

[patch] Patch

bug-189568-20180912170632.patch (text/plain), 4.05 KB, created by Keith Rollin on 2018-09-12 17:06:33 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Keith Rollin

Created: 2018-09-12 17:06:33 PDT

Size: 4.05 KB

patch

obsolete

>Subversion Revision: 235624
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index bf5ae182f65845d8d269f27b7421481f8baa3446..4cb2cae9afc8980cd95df44132beec3bea60ba67 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,37 @@
>+2018-09-12  Keith Rollin  <krollin@apple.com>
>+
>+        WebPageProxy::reportPageLoadResult can crash on some code paths
>+        https://bugs.webkit.org/show_bug.cgi?id=189568
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        WebPageProxy::reportPageLoadResult (which is called from
>+        WebPageProxy::didFinishLoadForFrame) can sometimes crash when
>+        accessing m_pageLoadStart (a std::optional) in its unloaded state.
>+        Normally, m_pageLoadStart is initialized in
>+        WebPageProxy::didStartProvisionalLoadForFrame, which one would expect
>+        would be called before WebPageProxy::didFinishLoadForFrame. But that
>+        turns out to not always be the case. It's not apparent under what
>+        conditions didStartProvisionalLoadForFrame will not be called, but
>+        it's happening in the wild, leading to crashes now that std::optional
>+        asserts in release builds on bad accesses (see
>+        https://bugs.webkit.org/show_bug.cgi?id=189568).
>+
>+        Fix this by adding checks of m_pageLoadState before all calls to
>+        reportPageLoadResult. The check of m_pageLoadState is made outside of
>+        reportPageLoadResult rather than inside of it because of review
>+        comments suggesting that they worked better outside and ended up being
>+        more efficient (https://bugs.webkit.org/show_bug.cgi?id=183221#c30).
>+
>+        Also remove an ASSERT that is no longer needed now that std::optional
>+        provides its own.
>+
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::didFailProvisionalLoadForFrame):
>+        (WebKit::WebPageProxy::didFinishLoadForFrame):
>+        (WebKit::WebPageProxy::didFailLoadForFrame):
>+        (WebKit::WebPageProxy::reportPageLoadResult):
>+
> 2018-09-04  Frederic Wang  <fwang@igalia.com>
> 
>         Bug 189190 - REGRESSION(r235398) ASSERTION failure !m_client.didFinishDocumentLoadForFrame
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index e28310ba60a928337a08f7693d8edc9156eba436..89513eec5d8fe4281f9d3bd9dc41072b00c469bd 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -3561,7 +3561,8 @@ void WebPageProxy::didFailProvisionalLoadForFrame(uint64_t frameID, const Securi
>     auto transaction = m_pageLoadState.transaction();
> 
>     if (frame->isMainFrame()) {
>-        reportPageLoadResult(error);
>+        if (m_pageLoadStart)
>+            reportPageLoadResult(error);
>         m_pageLoadState.didFailProvisionalLoad(transaction);
>         m_pageClient.didFailProvisionalLoadForMainFrame();
>     }
>@@ -3740,7 +3741,8 @@ void WebPageProxy::didFinishLoadForFrame(uint64_t frameID, uint64_t navigationID
>         m_loaderClient->didFinishLoadForFrame(*this, *frame, navigation.get(), m_process->transformHandlesToObjects(userData.object()).get());
> 
>     if (isMainFrame) {
>-        reportPageLoadResult();
>+        if (m_pageLoadStart)
>+            reportPageLoadResult();
>         m_pageClient.didFinishLoadForMainFrame();
> 
>         resetRecentCrashCountSoon();
>@@ -3787,7 +3789,8 @@ void WebPageProxy::didFailLoadForFrame(uint64_t frameID, uint64_t navigationID,
>         m_loaderClient->didFailLoadWithErrorForFrame(*this, *frame, navigation.get(), error, m_process->transformHandlesToObjects(userData.object()).get());
> 
>     if (isMainFrame) {
>-        reportPageLoadResult(error);
>+        if (m_pageLoadStart)
>+            reportPageLoadResult(error);
>         m_pageClient.didFailLoadForMainFrame();
>     }
> }
>@@ -7885,8 +7888,6 @@ void WebPageProxy::reportPageLoadResult(const ResourceError& error)
>         { CompletionCondition::Timeout, Seconds::infinity(), DiagnosticLoggingKeys::timedOutKey() }
>         });
> 
>-    ASSERT(m_pageLoadStart);
>-
>     auto pageLoadTime = MonotonicTime::now() - *m_pageLoadStart;
>     m_pageLoadStart = std::nullopt;
>

Actions: View | Formatted Diff | Diff

Attachments on bug 189568: 349599 | 349678