Bug 1809657: Start next line break chunk from start of previous search when no...

Bug 1809657: Start next line break chunk from start of previous search when no breaks found. r=jfkthame This means that we start from a known non-break and that we shouldn't be in any danger of causing false breaks once Uniscribe gets to unprocessed characters. This also makes the crash tests manual and debug only. Manual because now that the win32k pref is default on and not dynamic the tests will not run on try any more. Debug only so that we don't include code in opt builds that is only for manual tests. Differential Revision: https://phabricator.services.mozilla.com/D167271

Bug 1809657: Start next line break chunk from start of previous search when no...
72886c79 · Bob Owen · 012cbbbd · 72886c79 · 72886c79 · 72886c79
Commit 72886c79 authored 2 years ago by Bob Owen
--- a/intl/lwbrk/crashtests/Lo_test_page_no_uniscribe_breaks.html
+++ b/intl/lwbrk/crashtests/Lo_test_page_no_uniscribe_breaks.html
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>Lao - test page no breaks</title>
+<style>
+div {
+    width: 6em;
+}
+</style>
+
+<p>The text below does not produce any breaks with the Uniscribe breaker and is longer than the test buffer length used for brokering.</p>
+
+<div lang="lo">ການຮັບຮູ້ກຽດຕິສັກອັນມີປະຈຳຢູ່ຕົວບຸກຄົນໃນວົງສະກຸນຂອງມະນຸດທຸກໆຄົນການຮັບຮູ້ກຽດຕິສັກອັນມີປະຈຳຢູ່ຕົວບຸກຄົນໃນວົງສະກຸນຂອງມະນຸດທຸກໆຄົນ</div>
--- a/intl/lwbrk/crashtests/crashtests.list
+++ b/intl/lwbrk/crashtests/crashtests.list
 load 416721.html
-skip-if(!winWidget) pref(intl.compare_against_brokered_complex_line_breaks,true) pref(security.sandbox.content.win32k-disable,false) load UDHR_Thai_test_page_long_sequences.html
--- a/intl/lwbrk/crashtests/crashtests_manual.list
+++ b/intl/lwbrk/crashtests/crashtests_manual.list
+# Tests need to be run with --setpref security.sandbox.content.win32k-disable=false
+# This is because the pref is not dynamic and is also the reason that these tests
+# can only be run manually. They are also DEBUG only.
+defaults pref(intl.compare_against_brokered_complex_line_breaks,true)
+load Lo_test_page_no_uniscribe_breaks.html
+load UDHR_Thai_test_page_long_sequences.html
--- a/intl/lwbrk/nsUniscribeBreaker.cpp
+++ b/intl/lwbrk/nsUniscribeBreaker.cpp
@@ -19,7 +19,7 @@
 #  include "mozilla/sandboxTarget.h"
 #  include "nsXULAppAPI.h"

-#  if defined(ENABLE_TESTS)
+#  if defined(MOZ_DEBUG)
 #    include "mozilla/StaticPrefs_intl.h"
 #  endif
 #endif
@@ -100,7 +100,7 @@ void NS_GetComplexLineBreaks(const char16_t* aText, uint32_t aLength,
    }
  }

-#if defined(ENABLE_TESTS) && defined(MOZ_SANDBOX)
+#if defined(MOZ_DEBUG) && defined(MOZ_SANDBOX)
  // When tests are enabled and pref is set, we compare the line breaks returned
  // from the Uniscribe breaker in the content process, with the ones returned
  // from the brokered call to the parent. If they differ we crash so we can

--- a/security/sandbox/chromium-shim/sandbox/win/src/line_break_common.h
+++ b/security/sandbox/chromium-shim/sandbox/win/src/line_break_common.h
@@ -11,7 +11,7 @@

 namespace sandbox {

-#if defined(ENABLE_TESTS)
+#if defined(MOZ_DEBUG)
 // Set a low max brokered length for testing to exercise the chunking code.
 static const std::ptrdiff_t kMaxBrokeredLen = 50;


--- a/security/sandbox/chromium-shim/sandbox/win/src/line_break_interception.cc
+++ b/security/sandbox/chromium-shim/sandbox/win/src/line_break_interception.cc
@@ -49,6 +49,14 @@ ResultCode GetComplexLineBreaksProxy(const wchar_t* aText, uint32_t aLength,
      chunkEnd = textIterEnd;
    }

+    // Uniscribe seems to often (perhaps always) set the first element to a
+    // break, so we use chunk_start_reset to hold the known value of the first
+    // element of a chunk and reset it after Uniscribe processing. The only time
+    // we don't start from an already processed element is the first call, but
+    // resetting this to false is correct because whether we can break before
+    // the first character is decided by our caller.
+    uint8_t chunk_start_reset = *breakBeforeIter;
+
    uint32_t len = chunkEnd - textIter;
    // CountedBuffer takes a wchar_t* even though it doesn't change the buffer.
    CountedBuffer textBuf(const_cast<wchar_t*>(textIter),
@@ -66,24 +74,34 @@ ResultCode GetComplexLineBreaksProxy(const wchar_t* aText, uint32_t aLength,
      return SBOX_ERROR_GENERIC;
    }

+    *breakBeforeIter = chunk_start_reset;
+
    if (chunkEnd == textIterEnd) {
      break;
    }

-    // We couldn't process all of the text in one go, so back up by 32 chars
-    // and look for a break, then continue from that position.
+    // We couldn't process all of the text in one go, so back up by 32 chars and
+    // look for a break, then continue from that position. We back up 32 chars
+    // to try to avoid any false breaks at the end of the buffer caused by us
+    // splitting it into chunks.
    uint8_t* processedToEnd = breakBeforeIter + len;
    breakBeforeIter = processedToEnd - kBreakSearchRange;
    while (!*breakBeforeIter) {
      if (++breakBeforeIter == processedToEnd) {
+        // We haven't found a break in the search range, so go back to the start
+        // of our search range to try and ensure we don't get any false breaks
+        // at the start of the new chunk.
+        breakBeforeIter = processedToEnd - kBreakSearchRange;
+        // Make sure we don't split a surrogate pair.
+        if (IS_LOW_SURROGATE(
+                *(aText + (breakBeforeIter - aBreakBefore)))) {
+          ++breakBeforeIter;
+        }
        break;
      }
    }
  } while (true);

-  // Whether we can break before the first character is decided by our caller
-  aBreakBefore[0] = false;
-
  return SBOX_ALL_OK;
 }


--- a/testing/web-platform/meta/css/css-text/line-breaking/line-breaking-025.html.ini
+++ b/testing/web-platform/meta/css/css-text/line-breaking/line-breaking-025.html.ini
 [line-breaking-025.html]
  expected:
    if os == "mac": PASS
-    if os == "win" and not ccov: PASS
    FAIL