Attachment 361033 Details for Bug 194131 – Patch

[patch] Patch

bug-194131-20190203184157.patch (text/plain), 16.88 KB, created by Michael Catanzaro on 2019-02-03 16:41:58 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Michael Catanzaro

Created: 2019-02-03 16:41:58 PST

Size: 16.88 KB

patch

obsolete

>Subversion Revision: 240850
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 61e73b490be963d03dc2065b5a59c20020462986..f789ec2ed670cea373589ba818df877c7a95944f 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,30 @@
>+2019-02-03  Michael Catanzaro  <mcatanzaro@igalia.com>
>+
>+        [WPE][GTK] Load events may occur in unexpected order when JS redirects page before subresource load finishes
>+        https://bugs.webkit.org/show_bug.cgi?id=194131
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        If a new page load starts before the previous one completes, run LOAD_FINISHED before
>+        LOAD_STARTED to avoid confusing applications that expect LOAD_FINISHED to occur. We could
>+        also perhaps run load-failed as well, with a cancellation error, but this patch doesn't
>+        implement that.
>+
>+        (It's still possible for LOAD_COMMITTED to be skipped, although not when the new load is
>+        triggered by JS, since of course there's no JS to run before LOAD_COMMITTED.)
>+
>+        Also, add a state tracker to ensure we don't make unexpected state transitions like this.
>+        Without the state tracker, we are putting too much trust in FrameLoader to emit events in
>+        the order we expect, and FrameLoader is extremely complex so this is difficult to guarantee.
>+
>+        * UIProcess/API/glib/WebKitNavigationClient.cpp:
>+        (NavigationStateTracker::NavigationStateTracker):
>+        (NavigationStateTracker::isFinishedState):
>+        (NavigationStateTracker::advanceState):
>+        (NavigationClient::NavigationClient):
>+        * UIProcess/API/glib/WebKitWebView.cpp:
>+        (webkit_web_view_class_init):
>+
> 2019-02-01  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         [SOUP] Improve use of PAL::SessionID in WebKitCookieManager
>diff --git a/Source/WebKit/UIProcess/API/glib/WebKitNavigationClient.cpp b/Source/WebKit/UIProcess/API/glib/WebKitNavigationClient.cpp
>index 08ab69a05018423748cf132a03610012145e2cd8..fc1c943123c5d2e48d7fd7cbdac8749f7e1b478a 100644
>--- a/Source/WebKit/UIProcess/API/glib/WebKitNavigationClient.cpp
>+++ b/Source/WebKit/UIProcess/API/glib/WebKitNavigationClient.cpp
>@@ -34,21 +34,87 @@
> using namespace WebKit;
> using namespace WebCore;
> 
>+enum class NavigationState {
>+    NotStarted,
>+    Started,
>+    Redirected,
>+    FailedProvisionalNavigation,
>+    Committed,
>+    FailedNavigation,
>+    Finished,
>+};
>+
>+class NavigationStateTracker {
>+public:
>+    using MustFinishNavigationCallback = WTF::Function<void (void)>;
>+
>+    explicit NavigationStateTracker(MustFinishNavigationCallback&& mustFinishNavigationCallback)
>+        : m_mustFinishNavigationCallback(WTFMove(mustFinishNavigationCallback))
>+    {
>+    }
>+
>+    static bool isFinishedState(NavigationState state)
>+    {
>+        return state == NavigationState::FailedProvisionalNavigation
>+            || state == NavigationState::FailedNavigation
>+            || state == NavigationState::Finished;
>+    }
>+
>+    void advanceState(NavigationState state)
>+    {
>+        switch (state) {
>+        case NavigationState::NotStarted:
>+            ASSERT_NOT_REACHED();
>+            break;
>+        case NavigationState::Started:
>+            // If Started is received before Committed or Finished, we want to
>+            // ensure the Finished event occurs. (But Committed may skipped.)
>+            if (m_currentState != NavigationState::NotStarted && !isFinishedState(m_currentState))
>+                m_mustFinishNavigationCallback();
>+            break;
>+        case NavigationState::Redirected:
>+            ASSERT(m_currentState == NavigationState::Started || m_currentState == NavigationState::Redirected);
>+            break;
>+        case NavigationState::FailedProvisionalNavigation:
>+            ASSERT(m_currentState == NavigationState::Started || m_currentState == NavigationState::Redirected);
>+            break;
>+        case NavigationState::Committed:
>+            ASSERT(m_currentState == NavigationState::Started || m_currentState == NavigationState::Redirected);
>+            break;
>+        case NavigationState::FailedNavigation:
>+            ASSERT(m_currentState == NavigationState::Committed);
>+            break;
>+        case NavigationState::Finished:
>+            ASSERT(m_currentState == NavigationState::Committed);
>+            break;
>+        }
>+
>+        m_currentState = state;
>+    }
>+
>+private:
>+    NavigationState m_currentState = NavigationState::NotStarted;
>+    MustFinishNavigationCallback m_mustFinishNavigationCallback;
>+};
>+
> class NavigationClient : public API::NavigationClient {
> public:
>     explicit NavigationClient(WebKitWebView* webView)
>         : m_webView(webView)
>+        , m_navigationStateTracker([this] { webkitWebViewLoadChanged(m_webView, WEBKIT_LOAD_FINISHED); })
>     {
>     }
> 
> private:
>     void didStartProvisionalNavigation(WebPageProxy&, API::Navigation*, API::Object* /* userData */) override
>     {
>+        m_navigationStateTracker.advanceState(NavigationState::Started);
>         webkitWebViewLoadChanged(m_webView, WEBKIT_LOAD_STARTED);
>     }
> 
>     void didReceiveServerRedirectForProvisionalNavigation(WebPageProxy&, API::Navigation*, API::Object* /* userData */) override
>     {
>+        m_navigationStateTracker.advanceState(NavigationState::Redirected);
>         webkitWebViewLoadChanged(m_webView, WEBKIT_LOAD_REDIRECTED);
>     }
> 
>@@ -56,6 +122,7 @@ private:
>     {
>         if (!frame.isMainFrame())
>             return;
>+        m_navigationStateTracker.advanceState(NavigationState::FailedProvisionalNavigation);
>         GUniquePtr<GError> error(g_error_new_literal(g_quark_from_string(resourceError.domain().utf8().data()),
>             toWebKitError(resourceError.errorCode()), resourceError.localizedDescription().utf8().data()));
>         if (resourceError.tlsErrors()) {
>@@ -67,11 +134,13 @@ private:
> 
>     void didCommitNavigation(WebPageProxy&, API::Navigation*, API::Object* /* userData */) override
>     {
>+        m_navigationStateTracker.advanceState(NavigationState::Committed);
>         webkitWebViewLoadChanged(m_webView, WEBKIT_LOAD_COMMITTED);
>     }
> 
>     void didFinishNavigation(WebPageProxy&, API::Navigation*, API::Object* /* userData */) override
>     {
>+        m_navigationStateTracker.advanceState(NavigationState::Finished);
>         webkitWebViewLoadChanged(m_webView, WEBKIT_LOAD_FINISHED);
>     }
> 
>@@ -79,6 +148,7 @@ private:
>     {
>         if (!frame.isMainFrame())
>             return;
>+        m_navigationStateTracker.advanceState(NavigationState::FailedNavigation);
>         GUniquePtr<GError> error(g_error_new_literal(g_quark_from_string(resourceError.domain().utf8().data()),
>             toWebKitError(resourceError.errorCode()), resourceError.localizedDescription().utf8().data()));
>         webkitWebViewLoadFailed(m_webView, WEBKIT_LOAD_COMMITTED, resourceError.failingURL().string().utf8().data(), error.get());
>@@ -136,6 +206,7 @@ private:
>     }
> 
>     WebKitWebView* m_webView;
>+    NavigationStateTracker m_navigationStateTracker;
> };
> 
> void attachNavigationClientToView(WebKitWebView* webView)
>diff --git a/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp b/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
>index 67201d89eaa1f3f20df8b91f1da15d9c258b638f..2979b8a5238c7e8d635bdb440f52948c5fc8534f 100644
>--- a/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
>+++ b/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
>@@ -1176,6 +1176,12 @@ static void webkit_web_view_class_init(WebKitWebViewClass* webViewClass)
>      * When the page content starts arriving the signal is emitted with
>      * %WEBKIT_LOAD_COMMITTED event.
>      *
>+     * If a new load is started when the previous load has not yet
>+     * finished, or if the load fails, then %WEBKIT_LOAD_COMMITTED may
>+     * or may not be emitted. In contrast, %WEBKIT_LOAD_FINISHED is
>+     * guaranteed to be emitted before the next %WEBKIT_LOAD_STARTED,
>+     * even if the load did not actually complete.
>+     *
>      * You can handle this signal and use a switch to track any ongoing
>      * load operation.
>      *
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index fa05746bc41b0b70d8ae060fb6ea985a9d78f34e..d1459fdab8f9e2124d7258328713371194e81035 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,20 @@
>+2019-02-03  Michael Catanzaro  <mcatanzaro@igalia.com>
>+
>+        [WPE][GTK] Load events may occur in unexpected order when JS redirects page before subresource load finishes
>+        https://bugs.webkit.org/show_bug.cgi?id=194131
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * TestWebKitAPI/Tests/WebKitGLib/TestLoaderClient.cpp:
>+        (testUnfinishedSubresourceLoad):
>+        (serverCallback):
>+        (beforeAll):
>+        * TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.cpp:
>+        (loadChangedCallback):
>+        (loadFailedCallback):
>+        (LoadTrackingTest::LoadTrackingTest):
>+        * TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.h:
>+
> 2019-02-01  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC] Adjust replaced element's intrinsic ratio
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitGLib/TestLoaderClient.cpp b/Tools/TestWebKitAPI/Tests/WebKitGLib/TestLoaderClient.cpp
>index c67af1684ffcdf23859bc475d0905d2af98a4bad..17e40514da0c047c4056795736ffd523edf9f051 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitGLib/TestLoaderClient.cpp
>+++ b/Tools/TestWebKitAPI/Tests/WebKitGLib/TestLoaderClient.cpp
>@@ -240,6 +240,25 @@ static void testWebViewLoadTwiceAndReload(LoadTwiceAndReloadTest* test, gconstpo
>     test->waitUntilFinished();
> }
> 
>+static void testUnfinishedSubresourceLoad(LoadTrackingTest* test, gconstpointer)
>+{
>+    // Verify that LoadFinished occurs even if the next load starts before the
>+    // previous load actually finishes.
>+    test->m_loadMayTriggerSubsequentLoad = true;
>+    test->loadURI(kServer->getURIForPath("/unfinished-subresource-load").data());
>+    test->waitUntilLoadFinished();
>+    test->waitUntilLoadFinished();
>+
>+    Vector<LoadTrackingTest::LoadEvents>& events = test->m_loadEvents;
>+    g_assert_cmpint(events.size(), ==, 6);
>+    g_assert_cmpint(events[0], ==, LoadTrackingTest::ProvisionalLoadStarted);
>+    g_assert_cmpint(events[1], ==, LoadTrackingTest::LoadCommitted);
>+    g_assert_cmpint(events[2], ==, LoadTrackingTest::LoadFinished);
>+    g_assert_cmpint(events[3], ==, LoadTrackingTest::ProvisionalLoadStarted);
>+    g_assert_cmpint(events[4], ==, LoadTrackingTest::LoadCommitted);
>+    g_assert_cmpint(events[5], ==, LoadTrackingTest::LoadFinished);
>+}
>+
> class ViewURITrackingTest: public LoadTrackingTest {
> public:
>     MAKE_GLIB_TEST_FIXTURE(ViewURITrackingTest);
>@@ -590,6 +609,16 @@ static void serverCallback(SoupServer* server, SoupMessage* message, const char*
>         "Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!"
>         "Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!</body></html>";
> 
>+    static const char* unfinishedSubresourceLoadResponseString = "<html><body>"
>+        "<img src=\"/stall\"/>"
>+        "<script>"
>+        "  function run() {"
>+        "      location = '/normal';"
>+        "  }"
>+        "  setInterval(run(), 50);"
>+        "</script>"
>+        "</body></html>";
>+
>     if (message->method != SOUP_METHOD_GET) {
>         soup_message_set_status(message, SOUP_STATUS_NOT_IMPLEMENTED);
>         return;
>@@ -625,6 +654,12 @@ static void serverCallback(SoupServer* server, SoupMessage* message, const char*
>     } else if (g_str_equal(path, "/redirect-to-data")) {
>         soup_message_set_status(message, SOUP_STATUS_MOVED_PERMANENTLY);
>         soup_message_headers_append(message->response_headers, "Location", "data:text/plain;charset=utf-8,data-uri");
>+    } else if (g_str_equal(path, "/unfinished-subresource-load")) {
>+        soup_message_body_append(message->response_body, SOUP_MEMORY_STATIC, unfinishedSubresourceLoadResponseString, strlen(unfinishedSubresourceLoadResponseString));
>+    } else if (g_str_equal(path, "/stall")) {
>+        // This request is never unpaused and stalls forever.
>+        soup_server_pause_message(server, message);
>+        return;
>     } else
>         soup_message_set_status(message, SOUP_STATUS_NOT_FOUND);
> 
>@@ -655,6 +690,7 @@ void beforeAll()
>     LoadTrackingTest::add("WebKitWebView", "reload", testWebViewReload);
>     LoadTrackingTest::add("WebKitWebView", "history-load", testWebViewHistoryLoad);
>     LoadTwiceAndReloadTest::add("WebKitWebView", "load-twice-and-reload", testWebViewLoadTwiceAndReload);
>+    LoadTrackingTest::add("WebKitWebView", "unfinished-subresource-load", testUnfinishedSubresourceLoad);
> 
>     // This test checks that web view notify::uri signal is correctly emitted
>     // and the uri is already updated when loader client signals are emitted.
>diff --git a/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.cpp b/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.cpp
>index 2dddeca362e9b2282476f995d326771b5a88520e..3c1a2a573b21bb2a30b99b977b48542c7bfe12a6 100644
>--- a/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.cpp
>+++ b/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.cpp
>@@ -25,7 +25,8 @@ static void loadChangedCallback(WebKitWebView* webView, WebKitLoadEvent loadEven
>     switch (loadEvent) {
>     case WEBKIT_LOAD_STARTED:
>         g_assert_true(webkit_web_view_is_loading(webView));
>-        g_assert_cmpstr(test->m_activeURI.data(), ==, webkit_web_view_get_uri(webView));
>+        if (!test->m_loadMayTriggerSubsequentLoad)
>+            g_assert_cmpstr(test->m_activeURI.data(), ==, webkit_web_view_get_uri(webView));
>         test->provisionalLoadStarted();
>         break;
>     case WEBKIT_LOAD_REDIRECTED:
>@@ -48,14 +49,15 @@ static void loadChangedCallback(WebKitWebView* webView, WebKitLoadEvent loadEven
>         break;
>     }
>     case WEBKIT_LOAD_FINISHED:
>-        if (!test->m_loadFailed) {
>-            g_assert_false(webkit_web_view_is_loading(webView));
>-            g_assert_cmpstr(test->m_activeURI.data(), ==, webkit_web_view_get_uri(webView));
>-        } else if (!g_error_matches(test->m_error.get(), WEBKIT_NETWORK_ERROR, WEBKIT_NETWORK_ERROR_CANCELLED)) {
>-            // When a new load is started before the previous one has finished, we receive the load-finished signal
>-            // of the ongoing load while we already have a provisional URL for the new load. This is the only case
>-            // where isloading is true when the load has finished.
>-            g_assert_false(webkit_web_view_is_loading(webView));
>+        if (!test->m_loadMayTriggerSubsequentLoad) {
>+            if (!test->m_loadFailed) {
>+                g_assert_false(webkit_web_view_is_loading(webView));
>+                g_assert_cmpstr(test->m_activeURI.data(), ==, webkit_web_view_get_uri(webView));
>+            } else if (!g_error_matches(test->m_error.get(), WEBKIT_NETWORK_ERROR, WEBKIT_NETWORK_ERROR_CANCELLED)) {
>+                // When a new load is started before the previous one has finished, we receive the load-finished signal
>+                // of the ongoing load while we already have a provisional URL for the new load.
>+                g_assert_false(webkit_web_view_is_loading(webView));
>+            }
>         }
>         test->loadFinished();
>         break;
>@@ -71,10 +73,9 @@ static void loadFailedCallback(WebKitWebView* webView, WebKitLoadEvent loadEvent
>     g_assert_nonnull(error);
>     test->m_error.reset(g_error_copy(error));
> 
>-    if (!g_error_matches(error, WEBKIT_NETWORK_ERROR, WEBKIT_NETWORK_ERROR_CANCELLED)) {
>+    if (!test->m_loadMayTriggerSubsequentLoad && !g_error_matches(error, WEBKIT_NETWORK_ERROR, WEBKIT_NETWORK_ERROR_CANCELLED)) {
>         // When a new load is started before the previous one has finished, we receive the load-failed signal
>-        // of the ongoing load while we already have a provisional URL for the new load. This is the only case
>-        // where is-loading is true when the load has failed.
>+        // of the ongoing load while we already have a provisional URL for the new load.
>         g_assert_false(webkit_web_view_is_loading(webView));
>     }
> 
>@@ -108,8 +109,6 @@ static void estimatedProgressChangedCallback(GObject*, GParamSpec*, LoadTracking
> }
> 
> LoadTrackingTest::LoadTrackingTest()
>-    : m_runLoadUntilCompletion(false)
>-    , m_loadFailed(false)
> {
>     g_signal_connect(m_webView, "load-changed", G_CALLBACK(loadChangedCallback), this);
>     g_signal_connect(m_webView, "load-failed", G_CALLBACK(loadFailedCallback), this);
>diff --git a/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.h b/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.h
>index 02880904a742f21b14a8936601b71e2d6b1e7985..80d1c733d881e843c4616ddedea51d418e4b7242 100644
>--- a/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.h
>+++ b/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.h
>@@ -59,8 +59,9 @@ public:
>         LoadFailed,
>         LoadFailedWithTLSErrors
>     };
>-    bool m_runLoadUntilCompletion;
>-    bool m_loadFailed;
>+    bool m_runLoadUntilCompletion = false;
>+    bool m_loadFailed = false;
>+    bool m_loadMayTriggerSubsequentLoad = false;
>     GUniquePtr<GError> m_error;
>     Vector<LoadEvents> m_loadEvents;
>     float m_estimatedProgress;

Actions: View | Formatted Diff | Diff

Attachments on bug 194131: 361033 | 362880 | 362881 | 363333 | 363867 | 363868 | 364239