Check for index bounds in RTL handling for trailing whitespace runs. #12336

GaryQian · 2019-09-18T16:09:42Z

This prevents crashing when leading whitespace occurs in RTL text. This is a semi-temporary fix to prevent rolling back the change that fixes RTL text input in general.

Fixes flutter/flutter#39553

jason-simmons · 2019-09-18T18:16:57Z

third_party/txt/src/txt/paragraph_txt.cc

@@ -415,7 +415,11 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector<BidiRun>* result) {
  //
  // This only applies to the final whitespace at the end as other whitespace is
  // no longer ambiguous when surrounded by additional text.
+
+  // TODO(garyq): Handle this in the text editor caret code instead at layout


I'd still prefer to revert this instead of adding more complex post-processing to the output of the bidi algorithm.

I don't think it is viable for the RTL users of flutter if we revert this without a complete solution ready to go, as it causes all RTL input to look completely wrong. Reverting would make RTL essentially unusable until the final solution is rolled.

jason-simmons · 2019-09-18T18:20:29Z

third_party/txt/src/txt/paragraph_txt.cc

@@ -429,6 +433,9 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector<BidiRun>* result) {
      if (u_hasBinaryProperty(last_char, UCHAR_WHITE_SPACE)) {
        has_trailing_whitespace = true;
        bidi_run_count--;
+        if (bidi_run_start == 0) {


IIUC, the goal here is merging the last visual run into the second-to-last run if the last run consists of a single whitespace character.

Would it work to check whether the last run's start index follows the second-to-last run's end index here? If that is the case, then it's safe to reduce the bidi_run_count and increase the final run's length.

Yeah, that should be a more robust check. Implemented.

jason-simmons · 2019-09-18T19:33:46Z

third_party/txt/src/txt/paragraph_txt.cc

+        ubidi_getVisualRun(bidi.get(), bidi_run_count - 2,
+                           &second_last_bidi_run_start,
+                           &second_last_bidi_run_length);
+        if (bidi_run_start <= second_last_bidi_run_start) {


There are two cases:

last_bidi_run_start == second_last_bidi_run_end + 1

last_bidi_run_start == second_last_bidi_run_start - 1

For #1, it's safe to merge the last run into the second-to-last run.

For #2, the is_leading flag will merge the last two runs to create a run where the last whitespace occurs at the start of the final run. Is that desirable? Or would we be better off leaving the bidi runs unmodified in this case?

Let me try both modifying and not to see which one we want in terms of text input.

Ok, it looks like both produce reasonable results during text input. In this case, I will prefer not modifying it when it is leading.

GaryQian · 2019-09-23T18:08:12Z

@jason-simmons Ready for additional review!

jason-simmons · 2019-09-23T18:56:05Z

third_party/txt/src/txt/paragraph_txt.cc

+        ubidi_getVisualRun(bidi.get(), bidi_run_count - 2,
+                           &second_last_bidi_run_start,
+                           &second_last_bidi_run_length);
+        if (bidi_run_start >= second_last_bidi_run_start) {


Check whether bidi_run_start == second_last_bidi_run_start + second_last_bidi_run_length

has_trailing_whitespace will extend the second-to-last run by one character. This only makes sense if the additional character is the whitespace character comprising the final run

Oops, forgot to push -f, thought I had pushed the changes already!

…e runs. (flutter/engine#12336)

[email protected]:flutter/engine.git/compare/e7fd442410f4...953ac71 git log e7fd442..953ac71 --no-merges --oneline 2019-09-24 [email protected] Roll src/third_party/dart acac9ab11b..d53d355c6c (12 commits) 2019-09-24 [email protected] Roll src/third_party/skia b4b1005d485f..c096654fa7c6 (1 commits) (flutter/engine#12415) 2019-09-24 [email protected] Roll src/third_party/skia b3542d95da65..b4b1005d485f (6 commits) (flutter/engine#12414) 2019-09-24 [email protected] Write MINIMAL_SDK to exception message (flutter/engine#11345) 2019-09-23 [email protected] Track "mouse leave" event (flutter/engine#12363) 2019-09-23 [email protected] Roll src/third_party/dart 77ff89b223..acac9ab11b (6 commits) 2019-09-23 [email protected] Don't send pointer events when the framework isn't ready yet (flutter/engine#12403) 2019-09-23 [email protected] Update test to verify that secondary isolate gets shutdown before root isolate exits. (flutter/engine#12342) 2019-09-23 [email protected] Roll src/third_party/skia 57ef68077574..b3542d95da65 (12 commits) (flutter/engine#12409) 2019-09-23 [email protected] Update --dart-vm-flags whitelist to include --write-service-info and --sample-buffer-duration (flutter/engine#12395) 2019-09-23 [email protected] Add system font change listener for windows (flutter/engine#12276) 2019-09-23 [email protected] Roll fuchsia/sdk/core/mac-amd64 from xPX7p... to kgFwg... (flutter/engine#12405) 2019-09-23 [email protected] Check for index bounds in RTL handling for trailing whitespace runs. (flutter/engine#12336) 2019-09-23 [email protected] Roll fuchsia/sdk/core/linux-amd64 from F-g18... to NY2A5... (flutter/engine#12407) 2019-09-23 [email protected] Roll src/third_party/dart f546362691..77ff89b223 (6 commits) 2019-09-23 [email protected] Updating text field location in IOS as a pre-work for spellcheck (flutter/engine#12192) 2019-09-23 [email protected] Roll buildroot and remove toolchain prefix. (flutter/engine#12324) 2019-09-23 [email protected] Roll src/third_party/dart 94dd49cdb6..f546362691 (1 commits) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

GaryQian requested a review from jason-simmons September 18, 2019 16:10

googlebot added the cla: yes label Sep 18, 2019

GaryQian mentioned this pull request Sep 18, 2019

Leading space in RTL text crashes app flutter/flutter#39553

Closed

GaryQian requested review from shihaohong, chinmaygarde and HansMuller September 18, 2019 17:45

jason-simmons reviewed Sep 18, 2019

View reviewed changes

GaryQian added 8 commits September 23, 2019 10:12

Check for special case index out of bounds condition for leading space

946a1f7

Add TODO

a35fa71

Rework to pass tests

326308f

More robust check for leading

a30248d

Minor adjustment

aa9dbb8

Fix order bug

5e119a6

Do not modify for leading space

5b33075

Fix test value

74eb100

GaryQian requested a review from jason-simmons September 23, 2019 17:21

jason-simmons reviewed Sep 23, 2019

View reviewed changes

Condition

443a61d

GaryQian force-pushed the rtlleadingspacecrash branch from 4ea01db to 443a61d Compare September 23, 2019 19:36

jason-simmons approved these changes Sep 23, 2019

View reviewed changes

GaryQian merged commit 21b456d into flutter:master Sep 23, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 24, 2019

21b456d Check for index bounds in RTL handling for trailing whitespac…

71de124

…e runs. (flutter/engine#12336)

engine-flutter-autoroll mentioned this pull request Sep 24, 2019

Roll engine e7fd442410f4..953ac71749ee (18 commits) flutter/flutter#41163

Merged

eitanaviv mentioned this pull request Nov 4, 2019

App crashes for uncertain reason flutter/flutter#44060

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Check for index bounds in RTL handling for trailing whitespace runs. #12336

Check for index bounds in RTL handling for trailing whitespace runs. #12336

Uh oh!

GaryQian commented Sep 18, 2019

Uh oh!

jason-simmons Sep 18, 2019

Uh oh!

GaryQian Sep 18, 2019

Uh oh!

jason-simmons Sep 18, 2019

Uh oh!

GaryQian Sep 18, 2019

Uh oh!

jason-simmons Sep 18, 2019

Uh oh!

GaryQian Sep 18, 2019

Uh oh!

GaryQian Sep 18, 2019

Uh oh!

GaryQian commented Sep 23, 2019

Uh oh!

jason-simmons Sep 23, 2019

Uh oh!

GaryQian Sep 23, 2019

Uh oh!

Uh oh!

Check for index bounds in RTL handling for trailing whitespace runs. #12336

Check for index bounds in RTL handling for trailing whitespace runs. #12336

Uh oh!

Conversation

GaryQian commented Sep 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaryQian commented Sep 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!