Fix double LoadFinished on URL errors

RenderFrameDeleted was called before DidFinishLoad, which meant
m_loadingErrorFrameList was empty and wouldn't detect the page loaded
was an error page. Instead this patch relies on the chromium error-page
url which we already asserted.

Additonally we delay emitting the loadFinished signal until the error
page is also done loading, since the error-page can be considered part
of the load, and we otherwise have a race condition on toPlainText.

Finally we were not getting error-pages when blocking requests because
we reported them as aborted them instead of blocked.

Change-Id: I945eb838b7f080d4e146f18354e8986e1b88b5cd
Task-number: QTBUG-50752
Reviewed-by: Michael Brüning <michael.bruning@theqtcompany.com>

5 files changed, 58 insertions, 7 deletions

diff --git a/src/core/network_delegate_qt.cpp b/src/core/network_delegate_qt.cpp
index 6fdabcd4c..0c0de64fd 100644
--- a/src/core/network_delegate_qt.cpp
+++ b/src/core/network_delegate_qt.cpp
@@ -111,7 +111,7 @@ int NetworkDelegateQt::OnBeforeURLRequest(net::URLRequest *request, const net::C
         QWebEngineUrlRequestInfo requestInfo(infoPrivate);
         interceptor->interceptRequest(requestInfo);
         if (requestInfo.changed()) {
-            int result = infoPrivate->shouldBlockRequest ? net::ERR_ABORTED : net::OK;
+            int result = infoPrivate->shouldBlockRequest ? net::ERR_BLOCKED_BY_CLIENT : net::OK;
 
             if (qUrl != infoPrivate->url)
                 *newUrl = toGurl(infoPrivate->url);
diff --git a/src/core/web_contents_delegate_qt.cpp b/src/core/web_contents_delegate_qt.cpp
index 81f16970d..baf61b5fc 100644
--- a/src/core/web_contents_delegate_qt.cpp
+++ b/src/core/web_contents_delegate_qt.cpp
@@ -191,7 +191,15 @@ void WebContentsDelegateQt::DidFailProvisionalLoad(content::RenderFrameHost* ren
 void WebContentsDelegateQt::DidFailLoad(content::RenderFrameHost* render_frame_host, const GURL& validated_url, int error_code, const base::string16& error_description, bool was_ignored_by_handler)
 {
     Q_UNUSED(was_ignored_by_handler);
-    if (m_loadingErrorFrameList.removeOne(render_frame_host->GetRoutingID()) || render_frame_host->GetParent())
+    if (validated_url.spec() == content::kUnreachableWebDataURL) {
+        m_loadingErrorFrameList.removeOne(render_frame_host->GetRoutingID());
+        qCritical("Loading error-page failed. This shouldn't happen.");
+        if (!render_frame_host->GetParent())
+            m_viewClient->loadFinished(false /* success */, toQt(validated_url), true /* isErrorPage */);
+        return;
+    }
+
+    if (render_frame_host->GetParent())
         return;
 
     m_viewClient->loadFinished(false /* success */ , toQt(validated_url), false /* isErrorPage */, error_code, toQt(error_description));
@@ -200,8 +208,9 @@ void WebContentsDelegateQt::DidFailLoad(content::RenderFrameHost* render_frame_h
 
 void WebContentsDelegateQt::DidFinishLoad(content::RenderFrameHost* render_frame_host, const GURL& validated_url)
 {
-    if (m_loadingErrorFrameList.removeOne(render_frame_host->GetRoutingID())) {
-        Q_ASSERT(validated_url.is_valid() && validated_url.spec() == content::kUnreachableWebDataURL);
+    Q_ASSERT(validated_url.is_valid());
+    if (validated_url.spec() == content::kUnreachableWebDataURL) {
+        m_loadingErrorFrameList.removeOne(render_frame_host->GetRoutingID());
         m_viewClient->iconChanged(QUrl());
 
         // Trigger LoadFinished signal for main frame's error page only.
diff --git a/src/webenginewidgets/api/qwebenginepage.cpp b/src/webenginewidgets/api/qwebenginepage.cpp
index b1bf33067..458ddae5f 100644
--- a/src/webenginewidgets/api/qwebenginepage.cpp
+++ b/src/webenginewidgets/api/qwebenginepage.cpp
@@ -200,13 +200,21 @@ void QWebEnginePagePrivate::loadFinished(bool success, const QUrl &url, bool isE
     Q_UNUSED(errorCode);
     Q_UNUSED(errorDescription);
 
-    if (isErrorPage)
+    if (isErrorPage) {
+        Q_ASSERT(settings->testAttribute(QWebEngineSettings::ErrorPageEnabled));
+        Q_ASSERT(success);
+        Q_EMIT q->loadFinished(false);
         return;
+    }
 
     isLoading = false;
     if (success)
         explicitUrl = QUrl();
-    Q_EMIT q->loadFinished(success);
+    // Delay notifying failure until the error-page is done loading.
+    // Error-pages are not loaded on failures due to abort.
+    if (success || errorCode == -3 /* ERR_ABORTED*/ || !settings->testAttribute(QWebEngineSettings::ErrorPageEnabled)) {
+        Q_EMIT q->loadFinished(success);
+    }
     updateNavigationActions();
 }
 
diff --git a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
index 690cf70e4..207eb019a 100644
--- a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
+++ b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
@@ -241,6 +241,8 @@ private Q_SLOTS:
     void loadInSignalHandlers();
 
     void restoreHistory();
+    void toPlainTextLoadFinishedRace_data();
+    void toPlainTextLoadFinishedRace();
 
 private:
     QWebEngineView* m_view;
@@ -5115,5 +5117,36 @@ void tst_QWebEnginePage::restoreHistory()
     delete channel;
 }
 
+void tst_QWebEnginePage::toPlainTextLoadFinishedRace_data()
+{
+    QTest::addColumn<bool>("enableErrorPage");
+    QTest::newRow("disableErrorPage") << false;
+    QTest::newRow("enableErrorPage") << true;
+}
+
+void tst_QWebEnginePage::toPlainTextLoadFinishedRace()
+{
+    QFETCH(bool, enableErrorPage);
+
+    QWebEnginePage *page = new QWebEnginePage;
+    page->settings()->setAttribute(QWebEngineSettings::ErrorPageEnabled, enableErrorPage);
+    QSignalSpy spy(page, SIGNAL(loadFinished(bool)));
+
+    page->load(QUrl("data:text/plain,foobarbaz"));
+    QTRY_VERIFY(spy.count() == 1);
+    QCOMPARE(toPlainTextSync(page), QString("foobarbaz"));
+
+    page->load(QUrl("fail:unknown/scheme"));
+    QTRY_VERIFY(spy.count() == 2);
+    QString s = toPlainTextSync(page);
+    QVERIFY(s.contains("foobarbaz") == !enableErrorPage);
+
+    page->load(QUrl("data:text/plain,lalala"));
+    QTRY_VERIFY(spy.count() == 3);
+    QCOMPARE(toPlainTextSync(page), QString("lalala"));
+    delete page;
+    QVERIFY(spy.count() == 3);
+}
+
 QTEST_MAIN(tst_QWebEnginePage)
 #include "tst_qwebenginepage.moc"
diff --git a/tests/auto/widgets/qwebengineprofile/tst_qwebengineprofile.cpp b/tests/auto/widgets/qwebengineprofile/tst_qwebengineprofile.cpp
index 9195a5190..7cd423356 100644
--- a/tests/auto/widgets/qwebengineprofile/tst_qwebengineprofile.cpp
+++ b/tests/auto/widgets/qwebengineprofile/tst_qwebengineprofile.cpp
@@ -182,9 +182,10 @@ void tst_QWebEngineProfile::urlSchemeHandlerFailRequest()
     QWebEngineView view;
     QSignalSpy loadFinishedSpy(&view, SIGNAL(loadFinished(bool)));
     view.setPage(new QWebEnginePage(&profile, &view));
+    view.settings()->setAttribute(QWebEngineSettings::ErrorPageEnabled, false);
     view.load(QUrl(QStringLiteral("foo://bar")));
     QVERIFY(loadFinishedSpy.wait());
-    QVERIFY(toPlainTextSync(view.page()).isEmpty());
+    QCOMPARE(toPlainTextSync(view.page()), QString());
 }
 
 QTEST_MAIN(tst_QWebEngineProfile)