Fix a use-after-free condition in spellcheck request
SpellCheckRequest::didCancel() and SpellCheckRequest::didSucceed() call into
SpellChecker::didCheck(), which can delete the SpellCheckRequest object. After
this happens, the methods write to a private member variable in the deleted
instance. That's a use-after-free bug.
The bug was caught with help of ASAN on content shell. Content shell does not
set spellcheck client. The lack of the spellcheck client reduces the number of
references to the SpellCheckRequest object by 1. When SpellChecker::didCheck()
calls clear() on the ref-ptr of the SpellCheckRequest object, the object deletes
itself, because there're no more references to it.
The test WebFrameTest.CancelSpellingRequestCrash works by setting a NULL
spellcheck client and canceling a pending spellcheck request. This setup hits
the bug in SpellCheckRequest::didCancel(). It's not possible to hit the same bug
in SpellCheckRequest::didSucceed(), because it happens when there's no
spellcheck client, but only a spellcheck client calls didSucceed().
TEST=WebFrameTest.CancelSpellingRequestCrash
TEST=LayoutTests/editing/spelling/copy-paste-crash.html
BUG=259984
Review URL: https://chromiumcodereview.appspot.com/19275006
git-svn-id: svn://svn.chromium.org/blink/trunk@154357 bbb929c8-8fbe-4397-9dbb-9b2b20218538
diff --git a/LayoutTests/editing/spelling/copy-paste-crash-expected.txt b/LayoutTests/editing/spelling/copy-paste-crash-expected.txt
new file mode 100644
index 0000000..7b8cd602
--- /dev/null
+++ b/LayoutTests/editing/spelling/copy-paste-crash-expected.txt
@@ -0,0 +1,10 @@
+Spell check does not crash after Ctrl-X/Ctrl-V/type text in ASAN. To test manually, type 'A', Ctrl-A, Ctrl-X, Ctrl-V, and start typing again. The test succeeds when there's no crash in the last step.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Did not crash
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/editing/spelling/copy-paste-crash.html b/LayoutTests/editing/spelling/copy-paste-crash.html
new file mode 100644
index 0000000..ac590c2
--- /dev/null
+++ b/LayoutTests/editing/spelling/copy-paste-crash.html
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../fast/js/resources/js-test-pre.js"></script>
+<script src="resources/util.js"></script>
+</head>
+<body>
+<div id="container">
+ <input id="destination" type="text" name="type" value="">
+</div>
+
+<script>
+
+description("Spell check does not crash after Ctrl-X/Ctrl-V/type text in ASAN. " +
+ "To test manually, type 'A', Ctrl-A, Ctrl-X, Ctrl-V, and start typing again. " +
+ "The test succeeds when there's no crash in the last step.");
+
+initSpellTest("destination", "A", function(textNode) {
+ var behaviors = ["win", "mac"];
+ for (var i = 0; i < behaviors.length; i++) {
+ internals.settings.setEditingBehavior(behaviors[i]);
+ document.execCommand("SelectAll");
+ document.execCommand("Cut");
+ document.execCommand("Paste");
+ document.execCommand("InsertText", false, "A");
+ }
+ log("PASS Did not crash");
+});
+
+</script>
+<script src="../../fast/js/resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/editing/spelling/resources/util.js b/LayoutTests/editing/spelling/resources/util.js
index a178e06..6bedc1a 100644
--- a/LayoutTests/editing/spelling/resources/util.js
+++ b/LayoutTests/editing/spelling/resources/util.js
@@ -5,9 +5,9 @@
function verifySpellTest(nretry)
{
- var node = destination;
- if (destination.childNodes.length > 0)
- node = destination.childNodes[0];
+ var node = window.destination;
+ if (window.destination.childNodes.length > 0)
+ node = window.destination.childNodes[0];
if (nretry && !internals.markerCountForNode(node, "spelling")) {
window.setTimeout(function() { verifySpellTest(nretry - 1); }, 0);
return;
@@ -29,8 +29,8 @@
internals.settings.setSelectTrailingWhitespaceEnabled(false);
internals.settings.setUnifiedTextCheckerEnabled(true);
internals.settings.setEditingBehavior("win");
- var destination = document.getElementById(testElementId);
- destination.focus();
+ window.destination = document.getElementById(testElementId);
+ window.destination.focus();
document.execCommand("InsertText", false, testText);
window.setTimeout(function() { verifySpellTest(10); }, 0);
}
diff --git a/Source/WebKit/chromium/tests/WebFrameTest.cpp b/Source/WebKit/chromium/tests/WebFrameTest.cpp
index 2384279..919f71b 100644
--- a/Source/WebKit/chromium/tests/WebFrameTest.cpp
+++ b/Source/WebKit/chromium/tests/WebFrameTest.cpp
@@ -59,7 +59,9 @@
#include "core/dom/DocumentMarkerController.h"
#include "core/dom/MouseEvent.h"
#include "core/dom/Range.h"
+#include "core/editing/Editor.h"
#include "core/editing/FrameSelection.h"
+#include "core/editing/SpellChecker.h"
#include "core/html/HTMLFormElement.h"
#include "core/loader/FrameLoadRequest.h"
#include "core/page/EventHandler.h"
@@ -3162,6 +3164,30 @@
m_webView = 0;
}
+// This test verifies that cancelling spelling request does not cause a
+// write-after-free when there's no spellcheck client set.
+TEST_F(WebFrameTest, CancelSpellingRequestCrash)
+{
+ registerMockedHttpURLLoad("spell.html");
+ m_webView = FrameTestHelpers::createWebViewAndLoad(m_baseURL + "spell.html");
+ m_webView->setSpellCheckClient(0);
+
+ WebFrameImpl* frame = static_cast<WebFrameImpl*>(m_webView->mainFrame());
+ Document* document = frame->frame()->document();
+ Element* element = document->getElementById("data");
+
+ m_webView->settings()->setAsynchronousSpellCheckingEnabled(true);
+ m_webView->settings()->setUnifiedTextCheckerEnabled(true);
+ m_webView->settings()->setEditingBehavior(WebSettings::EditingBehaviorWin);
+
+ element->focus();
+ frame->frame()->editor()->replaceSelectionWithText("A", false, false);
+ frame->frame()->editor()->spellChecker()->cancelCheck();
+
+ m_webView->close();
+ m_webView = 0;
+}
+
class TestAccessInitialDocumentWebFrameClient : public WebFrameClient {
public:
TestAccessInitialDocumentWebFrameClient() : m_didAccessInitialDocument(false)
diff --git a/Source/core/editing/SpellChecker.cpp b/Source/core/editing/SpellChecker.cpp
index bfa17c3..417dfd1 100644
--- a/Source/core/editing/SpellChecker.cpp
+++ b/Source/core/editing/SpellChecker.cpp
@@ -89,16 +89,18 @@
{
if (!m_checker)
return;
- m_checker->didCheckSucceed(m_requestData.sequence(), results);
+ SpellChecker* checker = m_checker;
m_checker = 0;
+ checker->didCheckSucceed(m_requestData.sequence(), results);
}
void SpellCheckRequest::didCancel()
{
if (!m_checker)
return;
- m_checker->didCheckCancel(m_requestData.sequence());
+ SpellChecker* checker = m_checker;
m_checker = 0;
+ checker->didCheckCancel(m_requestData.sequence());
}
void SpellCheckRequest::setCheckerAndSequence(SpellChecker* requester, int sequence)
