Avoid Assertion Failure in HarfBuzzRun::characterIndexForXPosition
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 30 Jul 2012 23:13:25 +0000 (23:13 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 30 Jul 2012 23:13:25 +0000 (23:13 +0000)
https://bugs.webkit.org/show_bug.cgi?id=92376

Patch by Dominik Röttsches <dominik.rottsches@intel.com> on 2012-07-30
Reviewed by Tony Chang.

.:

Added manual test to reliably reproduce assertion failure which is solved by this patch.

* ManualTests/harfbuzz-mouse-selection-crash.html: Added.

Source/WebCore:

Previously, the if condition in offsetForPosition gating the call to
characterIndexForXPosition was comparing a different value than what was actually used
as the argument to calling it. In some cases, this can lead to a minuscule difference
when comparing the two floats - enough to trigger the assertion. To resolve this,
the accuracy of the index calculation is improved by changing the types from int
to floats and rephrasing the if condition to be exactly the same as what is checked
for in the assertion.

Manual test ManualTests/harfbuzz-mouse-selection-crash.html added
which reliably reproduces the assertion failure before this change.

* platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:
(WebCore::HarfBuzzShaper::HarfBuzzRun::characterIndexForXPosition): Types changed to float.
(WebCore::HarfBuzzShaper::offsetForPosition): Types changed to float, if condition rephrased.
* platform/graphics/harfbuzz/ng/HarfBuzzShaper.h:
(HarfBuzzRun):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@124111 268f45cc-cd09-0410-ab3c-d52691b4dbfc

ChangeLog
ManualTests/harfbuzz-mouse-selection-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp
Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.h

index 0b16cbb..cbc669a 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2012-07-30  Dominik Röttsches  <dominik.rottsches@intel.com>
+
+        Avoid Assertion Failure in HarfBuzzRun::characterIndexForXPosition
+        https://bugs.webkit.org/show_bug.cgi?id=92376
+
+        Reviewed by Tony Chang.
+
+        Added manual test to reliably reproduce assertion failure which is solved by this patch.
+
+        * ManualTests/harfbuzz-mouse-selection-crash.html: Added.
+
 2012-07-29  Vsevolod Vlasov  <vsevik@chromium.org>
 
         Web Inspector: Resource agent's reference to cached resources should be weak.
diff --git a/ManualTests/harfbuzz-mouse-selection-crash.html b/ManualTests/harfbuzz-mouse-selection-crash.html
new file mode 100644 (file)
index 0000000..a7e85a1
--- /dev/null
@@ -0,0 +1,54 @@
+<html>
+<head>
+<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
+<style type="text/css">
+#telugu {
+font-size: 2cm;
+line-height: 1.5em;
+font-family: "Lohit Telugu";
+}
+</style>
+<script>
+var ITERATIONS = 10000;
+
+if (window.testRunner)
+    testRunner.waitUntilDone();
+
+function mouseSelection() {
+    var body = document.body;
+    body.focus();
+
+    xStart = 0;
+    yStart = 0;
+
+    if (window.eventSender) {
+        eventSender.mouseMoveTo(xStart, yStart);
+        eventSender.mouseDown();
+        for(i=0;i<ITERATIONS;i++) {
+            randomCoord = randomCoordinate();
+            eventSender.mouseMoveTo(randomCoord.x, randomCoord.y);
+        }
+    }
+    if (window.testRunnder)
+        testRunner.notifyDone();
+}
+
+function randomCoordinate(axis) {
+    return { x: Math.floor(Math.random()*window.innerWidth),
+             y: Math.floor(Math.random()* window.innerHeight) }
+}
+
+</script>
+</head>
+<body onload="mouseSelection()"><p>
+   This test fuzzes HarfBuzzShaper::offsetForPosition to trigger an assertion in the downstream function characterIndexForXPosition().
+   See https://bugs.webkit.org/show_bug.cgi?id=92376 - reason of the assertion being hit is a minuscule floating point inaccuracy 
+   between an if condition that gates the call to characterIndexForXPosition and the actual argument that's then given to the function.
+   This test works on WebKit EFL, with the Ubuntu Telugu Lohit font installed to the EFL jhbuild font directory, like:</p>
+     <pre>$ cp /usr/share/fonts/truetype/ttf-telugu-fonts/lohit_te.ttf WebKitBuild/Dependencies/Source/webkitgtk-test-fonts-0.0.3</pre>
+   <p>Run with:</p>
+     <pre>$ WebKitBuild/Debug/bin/DumpRenderTree ManualTests/harfbuzz-mouse-selection-crash.html</pre>
+   <p>Before the change, this would crash 10/10 times. Test passes if it does not crash.</p>
+<p id="telugu">చేనేత కార్మికుల సమస్యలను ప్రభుత్వం దృష్టికి తీసుకువెళ్లేందుకు సిరిసిల్లలో వైఎస్ఆర్ సిపి గౌరవాధ్యక్షురాలు విజయమ్మ చేపట్టినదీక్ష విజయవంతమైంది.</p>
+</body>
+</html>
index 8ad7963..9b5ea43 100644 (file)
@@ -1,3 +1,27 @@
+2012-07-30  Dominik Röttsches  <dominik.rottsches@intel.com>
+
+        Avoid Assertion Failure in HarfBuzzRun::characterIndexForXPosition
+        https://bugs.webkit.org/show_bug.cgi?id=92376
+
+        Reviewed by Tony Chang.
+
+        Previously, the if condition in offsetForPosition gating the call to
+        characterIndexForXPosition was comparing a different value than what was actually used
+        as the argument to calling it. In some cases, this can lead to a minuscule difference
+        when comparing the two floats - enough to trigger the assertion. To resolve this,
+        the accuracy of the index calculation is improved by changing the types from int
+        to floats and rephrasing the if condition to be exactly the same as what is checked
+        for in the assertion.
+
+        Manual test ManualTests/harfbuzz-mouse-selection-crash.html added
+        which reliably reproduces the assertion failure before this change.
+
+        * platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:
+        (WebCore::HarfBuzzShaper::HarfBuzzRun::characterIndexForXPosition): Types changed to float.
+        (WebCore::HarfBuzzShaper::offsetForPosition): Types changed to float, if condition rephrased.
+        * platform/graphics/harfbuzz/ng/HarfBuzzShaper.h:
+        (HarfBuzzRun):
+
 2012-06-29  James Robinson  <jamesr@chromium.org>
 
         [chromium] Remove WebTransformationMatrix::mapPoint overrides
index 17ad2b6..c3bb54d 100644 (file)
@@ -117,14 +117,14 @@ void HarfBuzzShaper::HarfBuzzRun::setGlyphAndAdvance(unsigned index, uint16_t gl
     m_advances[index] = advance;
 }
 
-int HarfBuzzShaper::HarfBuzzRun::characterIndexForXPosition(int targetX)
+int HarfBuzzShaper::HarfBuzzRun::characterIndexForXPosition(float targetX)
 {
-    ASSERT(static_cast<unsigned>(targetX) <= m_width);
-    int currentX = 0;
+    ASSERT(targetX <= m_width);
+    float currentX = 0;
     float prevAdvance = 0;
     for (unsigned i = 0; i < m_numGlyphs; ++i) {
         float currentAdvance = m_advances[i] / 2.0;
-        int nextX = currentX + roundf(prevAdvance + currentAdvance);
+        float nextX = currentX + prevAdvance + currentAdvance;
         if (currentX <= targetX && targetX <= nextX)
             return m_glyphToCharacterIndex[i] + (rtl() ? 1 : 0);
         currentX = nextX;
@@ -369,25 +369,27 @@ void HarfBuzzShaper::setGlyphPositionsForHarfBuzzRun(HarfBuzzRun* currentRun, un
 int HarfBuzzShaper::offsetForPosition(float targetX)
 {
     int charactersSoFar = 0;
-    int currentX = 0;
+    float currentX = 0;
 
     if (m_run.rtl()) {
         charactersSoFar = m_normalizedBufferLength;
         for (int i = m_harfbuzzRuns.size() - 1; i >= 0; --i) {
             charactersSoFar -= m_harfbuzzRuns[i]->numCharacters();
-            int nextX = currentX + m_harfbuzzRuns[i]->width();
-            if (currentX <= targetX && targetX <= nextX) {
+            float nextX = currentX + m_harfbuzzRuns[i]->width();
+            float offsetForRun = targetX - currentX;
+            if (offsetForRun >= 0 && offsetForRun <= m_harfbuzzRuns[i]->width()) {
                 // The x value in question is within this script run.
-                const unsigned index = m_harfbuzzRuns[i]->characterIndexForXPosition(targetX - currentX);
+                const unsigned index = m_harfbuzzRuns[i]->characterIndexForXPosition(offsetForRun);
                 return charactersSoFar + index;
             }
             currentX = nextX;
         }
     } else {
         for (unsigned i = 0; i < m_harfbuzzRuns.size(); ++i) {
-            int nextX = currentX + m_harfbuzzRuns[i]->width();
-            if (currentX <= targetX && targetX <= nextX) {
-                const unsigned index = m_harfbuzzRuns[i]->characterIndexForXPosition(targetX - currentX);
+            float nextX = currentX + m_harfbuzzRuns[i]->width();
+            float offsetForRun = targetX - currentX;
+            if (offsetForRun >= 0 && offsetForRun <= m_harfbuzzRuns[i]->width()) {
+                const unsigned index = m_harfbuzzRuns[i]->characterIndexForXPosition(offsetForRun);
                 return charactersSoFar + index;
             }
             charactersSoFar += m_harfbuzzRuns[i]->numCharacters();
index 87844cc..8260fc9 100644 (file)
@@ -70,7 +70,7 @@ private:
         void setGlyphAndAdvance(unsigned index, uint16_t glyphId, float advance);
         void setWidth(float width) { m_width = width; }
 
-        int characterIndexForXPosition(int targetX);
+        int characterIndexForXPosition(float targetX);
         float xPositionForOffset(unsigned offset);
 
         const SimpleFontData* fontData() { return m_fontData; }