- Revision
- 143470
- Author
- t...@chromium.org
- Date
- 2013-02-20 09:48:41 -0800 (Wed, 20 Feb 2013)
Log Message
Source/WebKit/chromium: Fix use after free in ContextMenuClientImpl.cpp
https://bugs.webkit.org/show_bug.cgi?id=109220
Patch by Rouslan Solomakhin <rous...@chromium.org> on 2013-02-20
Reviewed by Tony Chang.
ContextMenuClientImpl can use a DocumentMarker after it is freed. The DocumentMarker is originally allocated
by the spell checker. When the user context-clicks on a misspelling, ContextMenuClientImpl saves a reference
to the clicked DocumentMarker, changes the selection, and then uses the DocumentMarker. Changing the selection
causes re-check of spelling. If the spell check client serves the spellcheck request from cache, then re-checking
spelling will delete the DocumentMarker and add a new one. This invalidates the DocumentMarker reference held by
ContextMenuClientImpl. When ContextMenuClientImpl attempts to use the DocumentMarker, Address Sanitizer detects
use after free. The fix is to save a copy of the DocumentMarker before changing selection.
* src/ContextMenuClientImpl.cpp:
(WebKit::selectMisspellingAsync): Save a copy of DocumentMarker before changing selection.
(WebKit::ContextMenuClientImpl::getCustomMenuFromDefaultItems): Use DocumentMarker instead of Vector<DocumentMarker*>.
Tools: [Chromium] Serve spellcheck suggestions for editing/spelling/spelling-changed-text.html from cache
https://bugs.webkit.org/show_bug.cgi?id=109220
Patch by Rouslan Solomakhin <rous...@chromium.org> on 2013-02-19
Reviewed by Tony Chang.
* DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp:
(WebTestRunner::MockSpellCheck::hasInCache): Added a method to detect whether spellcheck results can be served from cache.
(WebTestRunner):
(WebTestRunner::MockSpellCheck::fillSuggestionList): Modified to suggest "checker" for the word "cheher".
(WebTestRunner::MockSpellCheck::initializeIfNeeded): Modified to mark "cheher" as misspelling.
* DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.h:
(MockSpellCheck):
* DumpRenderTree/chromium/TestRunner/src/SpellCheckClient.cpp:
(WebTestRunner::SpellCheckClient::requestCheckingOfText): Modified to serve spellcheck suggestions from cache when possible.
LayoutTests: Spellchecker should not crash after text has changed and spellcheck results are served from cache
https://bugs.webkit.org/show_bug.cgi?id=109220
Patch by Rouslan Solomakhin <rous...@chromium.org> on 2013-02-20
Reviewed by Tony Chang.
* editing/spelling/spelling-changed-text-expected.txt: Added the expected output for serving spellcheck results from cache for edited text.
* editing/spelling/spelling-changed-text.html: Added a test for serving spellcheck results from cache for edited text.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (143469 => 143470)
--- trunk/LayoutTests/ChangeLog 2013-02-20 17:23:48 UTC (rev 143469)
+++ trunk/LayoutTests/ChangeLog 2013-02-20 17:48:41 UTC (rev 143470)
@@ -1,3 +1,13 @@
+2013-02-20 Rouslan Solomakhin <rous...@chromium.org>
+
+ Spellchecker should not crash after text has changed and spellcheck results are served from cache
+ https://bugs.webkit.org/show_bug.cgi?id=109220
+
+ Reviewed by Tony Chang.
+
+ * editing/spelling/spelling-changed-text-expected.txt: Added the expected output for serving spellcheck results from cache for edited text.
+ * editing/spelling/spelling-changed-text.html: Added a test for serving spellcheck results from cache for edited text.
+
2013-02-19 David Hyatt <hy...@apple.com>
[New Multicolumn] Make layers paint properly in columns.
Added: trunk/LayoutTests/editing/spelling/spelling-changed-text-expected.txt (0 => 143470)
--- trunk/LayoutTests/editing/spelling/spelling-changed-text-expected.txt (rev 0)
+++ trunk/LayoutTests/editing/spelling/spelling-changed-text-expected.txt 2013-02-20 17:48:41 UTC (rev 143470)
@@ -0,0 +1,10 @@
+Spellcheck should not crash after the text has changed and results are served from cache. To test manually, launch Chromium compiled with Address Sanitizer, enable 'Ask Google for Suggestions', type 'Spell cheher. Is it broken?', delete the words 'Is it broken?', and context-click on the word 'cheher'. The test suceeds when the browser does not crash and shows suggestions in the context menu.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS window.getSelection().toString() is " Is it broken?"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Property changes on: trunk/LayoutTests/editing/spelling/spelling-changed-text-expected.txt
___________________________________________________________________
Added: svn:eol-style
Added: trunk/LayoutTests/editing/spelling/spelling-changed-text.html (0 => 143470)
--- trunk/LayoutTests/editing/spelling/spelling-changed-text.html (rev 0)
+++ trunk/LayoutTests/editing/spelling/spelling-changed-text.html 2013-02-20 17:48:41 UTC (rev 143470)
@@ -0,0 +1,43 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+<script src=""
+</head>
+<body>
+<div id="container">
+ <div id="destination" contentEditable></div>
+</div>
+
+<script>
+
+description("Spellcheck should not crash after the text has changed and results are served from cache."
+ + " To test manually, launch Chromium compiled with Address Sanitizer, enable 'Ask Google for Suggestions', type 'Spell cheher. Is it broken?', delete the words 'Is it broken?', and context-click on the word 'cheher'."
+ + " The test suceeds when the browser does not crash and shows suggestions in the context menu.");
+
+initSpellTest("destination", "Spell cheher. Is it broken?", function(textNode) {
+ // Select the text "Is it broken?".
+ var deleteRange = document.createRange();
+ deleteRange.setStart(textNode, 13);
+ deleteRange.setEnd(textNode, 27);
+ window.getSelection().removeAllRanges();
+ window.getSelection().addRange(deleteRange);
+ shouldBeEqualToString("window.getSelection().toString()", " Is it broken?");
+ // Del key to delete the text "Is it broken?".
+ eventSender.keyDown(String.fromCharCode(0x007F), null);
+
+ // Context click to show the context menu.
+ var x = destination.offsetParent.offsetLeft + destination.offsetLeft + 50;
+ var y = destination.offsetParent.offsetTop + destination.offsetTop + destination.offsetHeight / 2;
+ eventSender.mouseMoveTo(x, y);
+ contextMenuElements = eventSender.contextClick();
+ // Esc key to hide the context menu.
+ eventSender.keyDown(String.fromCharCode(0x001B), null);
+
+ document.getElementById("destination").innerHTML = "";
+});
+
+</script>
+<script src=""
+</body>
+</html>
Property changes on: trunk/LayoutTests/editing/spelling/spelling-changed-text.html
___________________________________________________________________
Added: svn:mime-type
Added: svn:eol-style
Modified: trunk/Source/WebKit/chromium/ChangeLog (143469 => 143470)
--- trunk/Source/WebKit/chromium/ChangeLog 2013-02-20 17:23:48 UTC (rev 143469)
+++ trunk/Source/WebKit/chromium/ChangeLog 2013-02-20 17:48:41 UTC (rev 143470)
@@ -1,3 +1,22 @@
+2013-02-20 Rouslan Solomakhin <rous...@chromium.org>
+
+ Fix use after free in ContextMenuClientImpl.cpp
+ https://bugs.webkit.org/show_bug.cgi?id=109220
+
+ Reviewed by Tony Chang.
+
+ ContextMenuClientImpl can use a DocumentMarker after it is freed. The DocumentMarker is originally allocated
+ by the spell checker. When the user context-clicks on a misspelling, ContextMenuClientImpl saves a reference
+ to the clicked DocumentMarker, changes the selection, and then uses the DocumentMarker. Changing the selection
+ causes re-check of spelling. If the spell check client serves the spellcheck request from cache, then re-checking
+ spelling will delete the DocumentMarker and add a new one. This invalidates the DocumentMarker reference held by
+ ContextMenuClientImpl. When ContextMenuClientImpl attempts to use the DocumentMarker, Address Sanitizer detects
+ use after free. The fix is to save a copy of the DocumentMarker before changing selection.
+
+ * src/ContextMenuClientImpl.cpp:
+ (WebKit::selectMisspellingAsync): Save a copy of DocumentMarker before changing selection.
+ (WebKit::ContextMenuClientImpl::getCustomMenuFromDefaultItems): Use DocumentMarker instead of Vector<DocumentMarker*>.
+
2013-02-20 Sheriff Bot <webkit.review....@gmail.com>
Unreviewed. Rolled Chromium DEPS to r183552. Requested by
Modified: trunk/Source/WebKit/chromium/src/ContextMenuClientImpl.cpp (143469 => 143470)
--- trunk/Source/WebKit/chromium/src/ContextMenuClientImpl.cpp 2013-02-20 17:23:48 UTC (rev 143469)
+++ trunk/Source/WebKit/chromium/src/ContextMenuClientImpl.cpp 2013-02-20 17:48:41 UTC (rev 143470)
@@ -149,7 +149,7 @@
return isSpaceOrNewline(c) || WTF::Unicode::isPunct(c);
}
-static String selectMisspellingAsync(Frame* selectedFrame, Vector<DocumentMarker*>& markers)
+static String selectMisspellingAsync(Frame* selectedFrame, DocumentMarker& marker)
{
VisibleSelection selection = selectedFrame->selection()->selection();
if (!selection.isCaretOrRange())
@@ -157,14 +157,15 @@
// Caret and range selections always return valid normalized ranges.
RefPtr<Range> selectionRange = selection.toNormalizedRange();
- markers.append(selectedFrame->document()->markers()->markersInRange(selectionRange.get(), DocumentMarker::Spelling | DocumentMarker::Grammar));
+ Vector<DocumentMarker*> markers = selectedFrame->document()->markers()->markersInRange(selectionRange.get(), DocumentMarker::Spelling | DocumentMarker::Grammar);
if (markers.size() != 1)
return String();
+ marker = *markers[0];
// Cloning a range fails only for invalid ranges.
RefPtr<Range> markerRange = selectionRange->cloneRange(ASSERT_NO_EXCEPTION);
- markerRange->setStart(markerRange->startContainer(), markers[0]->startOffset());
- markerRange->setEnd(markerRange->endContainer(), markers[0]->endOffset());
+ markerRange->setStart(markerRange->startContainer(), marker.startOffset());
+ markerRange->setEnd(markerRange->endContainer(), marker.endOffset());
if (selection.isCaret()) {
selection = VisibleSelection(markerRange.get());
selectedFrame->selection()->setSelection(selection, WordGranularity);
@@ -310,11 +311,11 @@
// words and attaches suggestions to these markers in the background. Therefore, when a user right-clicks
// a mouse on a word, Chrome just needs to find a spelling marker on the word instead of spellchecking it.
if (selectedFrame->settings() && selectedFrame->settings()->asynchronousSpellCheckingEnabled()) {
- Vector<DocumentMarker*> markers;
- data.misspelledWord = selectMisspellingAsync(selectedFrame, markers);
- if (markers.size() == 1 && markers[0]->description().length()) {
+ DocumentMarker marker;
+ data.misspelledWord = selectMisspellingAsync(selectedFrame, marker);
+ if (marker.description().length()) {
Vector<String> suggestions;
- markers[0]->description().split('\n', suggestions);
+ marker.description().split('\n', suggestions);
data.dictionarySuggestions = suggestions;
} else if (m_webView->spellCheckClient()) {
int misspelledOffset, misspelledLength;
Modified: trunk/Tools/ChangeLog (143469 => 143470)
--- trunk/Tools/ChangeLog 2013-02-20 17:23:48 UTC (rev 143469)
+++ trunk/Tools/ChangeLog 2013-02-20 17:48:41 UTC (rev 143470)
@@ -1,3 +1,20 @@
+2013-02-19 Rouslan Solomakhin <rous...@chromium.org>
+
+ [Chromium] Serve spellcheck suggestions for editing/spelling/spelling-changed-text.html from cache
+ https://bugs.webkit.org/show_bug.cgi?id=109220
+
+ Reviewed by Tony Chang.
+
+ * DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp:
+ (WebTestRunner::MockSpellCheck::hasInCache): Added a method to detect whether spellcheck results can be served from cache.
+ (WebTestRunner):
+ (WebTestRunner::MockSpellCheck::fillSuggestionList): Modified to suggest "checker" for the word "cheher".
+ (WebTestRunner::MockSpellCheck::initializeIfNeeded): Modified to mark "cheher" as misspelling.
+ * DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.h:
+ (MockSpellCheck):
+ * DumpRenderTree/chromium/TestRunner/src/SpellCheckClient.cpp:
+ (WebTestRunner::SpellCheckClient::requestCheckingOfText): Modified to serve spellcheck suggestions from cache when possible.
+
2013-02-20 Xabier Rodriguez Calvar <calva...@igalia.com>
[Gtk] HTML5 Media controls require a design refresh
Modified: trunk/Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp (143469 => 143470)
--- trunk/Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp 2013-02-20 17:23:48 UTC (rev 143469)
+++ trunk/Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp 2013-02-20 17:48:41 UTC (rev 143470)
@@ -122,12 +122,19 @@
return false;
}
+bool MockSpellCheck::hasInCache(const WebString& word)
+{
+ return word == WebString::fromUTF8("Spell cheher. Is it broken?") || word == WebString::fromUTF8("Spell cheher.\x007F");
+}
+
void MockSpellCheck::fillSuggestionList(const WebString& word, WebVector<WebString>* suggestions)
{
if (word == WebString::fromUTF8("wellcome"))
append(suggestions, WebString::fromUTF8("welcome"));
else if (word == WebString::fromUTF8("upper case"))
append(suggestions, WebString::fromUTF8("uppercase"));
+ else if (word == WebString::fromUTF8("cheher"))
+ append(suggestions, WebString::fromUTF8("checker"));
}
bool MockSpellCheck::initializeIfNeeded()
@@ -173,7 +180,8 @@
"qwertyuiopasd",
"qwertyuiopasdf",
"upper case",
- "wellcome"
+ "wellcome",
+ "cheher"
};
m_misspelledWords.clear();
Modified: trunk/Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.h (143469 => 143470)
--- trunk/Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.h 2013-02-20 17:23:48 UTC (rev 143469)
+++ trunk/Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.h 2013-02-20 17:48:41 UTC (rev 143470)
@@ -60,6 +60,10 @@
// misspelledOffset and 2 to misspelledLength, respectively.
bool spellCheckWord(const WebKit::WebString& text, int* misspelledOffset, int* misspelledLength);
+ // Checks whether the specified text can be spell checked immediately using
+ // the spell checker cache.
+ bool hasInCache(const WebKit::WebString& text);
+
private:
// Initialize the internal resources if we need to initialize it.
// Initializing this object may take long time. To prevent from hurting
Modified: trunk/Tools/DumpRenderTree/chromium/TestRunner/src/SpellCheckClient.cpp (143469 => 143470)
--- trunk/Tools/DumpRenderTree/chromium/TestRunner/src/SpellCheckClient.cpp 2013-02-20 17:23:48 UTC (rev 143469)
+++ trunk/Tools/DumpRenderTree/chromium/TestRunner/src/SpellCheckClient.cpp 2013-02-20 17:48:41 UTC (rev 143470)
@@ -116,7 +116,10 @@
m_lastRequestedTextCheckingCompletion = completion;
m_lastRequestedTextCheckString = text;
- m_delegate->postDelayedTask(new HostMethodTask(this, &SpellCheckClient::finishLastTextCheck), 0);
+ if (m_spellcheck.hasInCache(text))
+ finishLastTextCheck();
+ else
+ m_delegate->postDelayedTask(new HostMethodTask(this, &SpellCheckClient::finishLastTextCheck), 0);
}
void SpellCheckClient::finishLastTextCheck()
