Fix uncalled callbacks with findText

Calling findText successively might prevent the previous pending
FindReply to be sent, which would leak the callback on the
application side.

This would cause a crash in qupzilla since we empty all pending
callbacks in the QWebEnginePage's destructor to catch this kind
of issue.

This also renames lastRequestId to nextRequestId to make it clear
that this is the ID generator for everything, including findText,
and that lastFindRequestId is only a tracker.

Change-Id: Ia78d553a58ed31af7237aad8772fa9828560c6d4
Reviewed-by: Andras Becsi <andras.becsi@digia.com>

4 files changed, 51 insertions, 10 deletions

diff --git a/src/core/web_contents_adapter.cpp b/src/core/web_contents_adapter.cpp
index 404af74ff..c765c31ab 100644
--- a/src/core/web_contents_adapter.cpp
+++ b/src/core/web_contents_adapter.cpp
@@ -305,13 +305,15 @@ public:
     scoped_ptr<WebContentsDelegateQt> webContentsDelegate;
     scoped_ptr<QtRenderViewObserverHost> renderViewObserverHost;
     WebContentsAdapterClient *adapterClient;
-    quint64 lastRequestId;
+    quint64 nextRequestId;
+    int lastFindRequestId;
 };
 
 WebContentsAdapterPrivate::WebContentsAdapterPrivate()
     // This has to be the first thing we create, and the last we destroy.
     : engineContext(WebEngineContext::current())
-    , lastRequestId(0)
+    , nextRequestId(1)
+    , lastFindRequestId(0)
 {
 }
 
@@ -645,28 +647,37 @@ quint64 WebContentsAdapter::runJavaScriptCallbackResult(const QString &javaScrip
     Q_D(WebContentsAdapter);
     content::RenderViewHost *rvh = d->webContents->GetRenderViewHost();
     Q_ASSERT(rvh);
-    content::RenderFrameHost::JavaScriptResultCallback callback = base::Bind(&callbackOnEvaluateJS, d->adapterClient, ++d->lastRequestId);
+    content::RenderFrameHost::JavaScriptResultCallback callback = base::Bind(&callbackOnEvaluateJS, d->adapterClient, d->nextRequestId++);
     rvh->GetMainFrame()->ExecuteJavaScript(toString16(javaScript), callback);
-    return d->lastRequestId;
+    return d->nextRequestId;
 }
 
 quint64 WebContentsAdapter::fetchDocumentMarkup()
 {
     Q_D(WebContentsAdapter);
-    d->renderViewObserverHost->fetchDocumentMarkup(++d->lastRequestId);
-    return d->lastRequestId;
+    d->renderViewObserverHost->fetchDocumentMarkup(d->nextRequestId++);
+    return d->nextRequestId;
 }
 
 quint64 WebContentsAdapter::fetchDocumentInnerText()
 {
     Q_D(WebContentsAdapter);
-    d->renderViewObserverHost->fetchDocumentInnerText(++d->lastRequestId);
-    return d->lastRequestId;
+    d->renderViewObserverHost->fetchDocumentInnerText(d->nextRequestId++);
+    return d->nextRequestId;
 }
 
 quint64 WebContentsAdapter::findText(const QString &subString, bool caseSensitively, bool findBackward)
 {
     Q_D(WebContentsAdapter);
+    if (d->lastFindRequestId > d->webContentsDelegate->lastReceivedFindReply()) {
+        // There are cases where the render process will overwrite a previous request
+        // with the new search and we'll have a dangling callback, leaving the application
+        // waiting for it forever.
+        // Assume that any unfinished find has been unsuccessful when a new one is started
+        // to cover that case.
+        d->adapterClient->didFindText(d->lastFindRequestId, 0);
+    }
+
     blink::WebFindOptions options;
     options.forward = !findBackward;
     options.matchCase = caseSensitively;
@@ -675,8 +686,9 @@ quint64 WebContentsAdapter::findText(const QString &subString, bool caseSensitiv
 
     // Find already allows a request ID as input, but only as an int.
     // Use the same counter but mod it to MAX_INT, this keeps the same likeliness of request ID clashing.
-    int shrunkRequestId = ++d->lastRequestId & 0x7fffffff;
+    int shrunkRequestId = d->nextRequestId++ & 0x7fffffff;
     d->webContents->Find(shrunkRequestId, toString16(subString), options);
+    d->lastFindRequestId = shrunkRequestId;
     return shrunkRequestId;
 }
 
diff --git a/src/core/web_contents_delegate_qt.cpp b/src/core/web_contents_delegate_qt.cpp
index b3c1cd70b..08f11cf5c 100644
--- a/src/core/web_contents_delegate_qt.cpp
+++ b/src/core/web_contents_delegate_qt.cpp
@@ -76,6 +76,7 @@ static WebContentsAdapterClient::JavaScriptConsoleMessageLevel mapToJavascriptCo
 
 WebContentsDelegateQt::WebContentsDelegateQt(content::WebContents *webContents, WebContentsAdapterClient *adapterClient)
     : m_viewClient(adapterClient)
+    , m_lastReceivedFindReply(0)
 {
     webContents->SetDelegate(this);
     Observe(webContents);
@@ -239,8 +240,10 @@ void WebContentsDelegateQt::FindReply(content::WebContents *source, int request_
     Q_UNUSED(source)
     Q_UNUSED(selection_rect)
     Q_UNUSED(active_match_ordinal)
-    if (final_update)
+    if (final_update) {
+        m_lastReceivedFindReply = request_id;
         m_viewClient->didFindText(request_id, number_of_matches);
+    }
 }
 
 void WebContentsDelegateQt::RequestMediaAccessPermission(content::WebContents *web_contents, const content::MediaStreamRequest &request, const content::MediaResponseCallback &callback)
diff --git a/src/core/web_contents_delegate_qt.h b/src/core/web_contents_delegate_qt.h
index e4bc143c1..8e1761a0a 100644
--- a/src/core/web_contents_delegate_qt.h
+++ b/src/core/web_contents_delegate_qt.h
@@ -65,6 +65,7 @@ public:
     WebContentsDelegateQt(content::WebContents*, WebContentsAdapterClient *adapterClient);
     QString lastSearchedString() const { return m_lastSearchedString; }
     void setLastSearchedString(const QString &s) { m_lastSearchedString = s; }
+    int lastReceivedFindReply() const { return m_lastReceivedFindReply; }
 
     virtual content::WebContents *OpenURLFromTab(content::WebContents *source, const content::OpenURLParams &params) Q_DECL_OVERRIDE;
     virtual void NavigationStateChanged(const content::WebContents* source, unsigned changed_flags) Q_DECL_OVERRIDE;
@@ -92,6 +93,7 @@ public:
 private:
     WebContentsAdapterClient *m_viewClient;
     QString m_lastSearchedString;
+    int m_lastReceivedFindReply;
 };
 
 #endif // WEB_CONTENTS_DELEGATE_QT_H
diff --git a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
index ca3740715..70fbba128 100644
--- a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
+++ b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp
@@ -175,6 +175,7 @@ private Q_SLOTS:
     void testStopScheduledPageRefresh();
     void findText();
     void findTextResult();
+    void findTextSuccessiveShouldCallAllCallbacks();
     void supportedContentType();
     // [Qt] tst_QWebEnginePage::infiniteLoopJS() timeouts with DFG JIT
     // https://bugs.webkit.org/show_bug.cgi?id=79040
@@ -3254,6 +3255,29 @@ void tst_QWebEnginePage::findTextResult()
     QCOMPARE(findTextSync(m_page, ""), false);
 }
 
+void tst_QWebEnginePage::findTextSuccessiveShouldCallAllCallbacks()
+{
+    CallbackSpy<bool> spy1;
+    CallbackSpy<bool> spy2;
+    CallbackSpy<bool> spy3;
+    CallbackSpy<bool> spy4;
+    CallbackSpy<bool> spy5;
+    QSignalSpy loadSpy(m_view, SIGNAL(loadFinished(bool)));
+    m_view->setHtml(QString("<html><head></head><body><div>abcdefg abcdefg abcdefg abcdefg abcdefg</div></body></html>"));
+    QTRY_COMPARE(loadSpy.count(), 1);
+    m_page->findText("abcde", 0, spy1.ref());
+    m_page->findText("abcd", 0, spy2.ref());
+    m_page->findText("abc", 0, spy3.ref());
+    m_page->findText("ab", 0, spy4.ref());
+    m_page->findText("a", 0, spy5.ref());
+    spy5.waitForResult();
+    QVERIFY(spy1.wasCalled());
+    QVERIFY(spy2.wasCalled());
+    QVERIFY(spy3.wasCalled());
+    QVERIFY(spy4.wasCalled());
+    QVERIFY(spy5.wasCalled());
+}
+
 static QString getMimeTypeForExtension(const QString &ext)
 {
     QMimeType mimeType = QMimeDatabase().mimeTypeForFile(QStringLiteral("filename.") + ext.toLower(), QMimeDatabase::MatchExtension);