Fix removeSpan crash on Android

[wildan-m]

Details

When applying formatting, there may be a race condition if we are trying to remove a span that has already been removed.

Related Issues

Expensify/App#46908

Manual Tests

1. type: #a
2. double tap a, ensure only a letter selected
3. when tap action shown, choose cut

Kapture.2024-09-03.at.14.03.01.mp4

Linked PRs

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[wildan-m]

I have read the CLA Document and I hereby sign the CLA

[wildan-m]

recheck

[tomekzaw]

@wildan-m Thanks for the PR! I will review it soon.

edit: I believe there was a reason why applyMarkdownFormatting is called from onTextChanged rather then afterTextChanged but I can't remember what it was exactly, I need to run git blame and will let you know once I find out.

[wildan-m]

@tomekzaw Sure, thank you.

Another option could be to include try-catch blocks within setSpan and removeSpan methods to handle any errors and log them, but it's uncertain if this would be deemed appropriate.

android/src/main/java/com/expensify/livemarkdown/MarkdownUtils.java

private void setSpan(SpannableStringBuilder ssb, MarkdownSpan span, int start, int end) {
  try {
    ssb.setSpan(span, start, end, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE);
  } catch (Exception e) {
    Log.e("MarkdownUtils", "Error setting span", e);
  }
}

private void removeSpans(SpannableStringBuilder ssb) {
  // We shouldn't use `removeSpans()` because it also removes SpellcheckSpan, SuggestionSpan etc.
  MarkdownSpan[] spans = ssb.getSpans(0, ssb.length(), MarkdownSpan.class);
  for (MarkdownSpan span : spans) {
    try {
      ssb.removeSpan(span);
    } catch (Exception e) {
      Log.e("MarkdownUtils", "Error removing span: " + span, e);
    }
  }
}

[tomekzaw]

Thanks again for the PR!

I think we can go even one more step further and get rid of mShouldSkip completely:

  @Override
  public void beforeTextChanged(CharSequence s, int start, int count, int after) {
    // do nothing
  }

  @Override
  public void onTextChanged(CharSequence s, int start, int before, int count) {
    // do nothing
  }

  @Override
  public void afterTextChanged(Editable editable) {
    if (editable instanceof SpannableStringBuilder) {
      mMarkdownUtils.applyMarkdownFormatting((SpannableStringBuilder) editable);
    }
  }

I've tested out this change and it seems to work correctly both on Fabric and Paper.

FYI Another option suggested by @j-piasecki would be to wrap removeSpan call inside a try/catch block:

private void removeSpans(SpannableStringBuilder ssb) {
    // We shouldn't use `removeSpans()` because it also removes SpellcheckSpan, SuggestionSpan etc.
    MarkdownSpan[] spans = ssb.getSpans(0, ssb.length(), MarkdownSpan.class);
    for (MarkdownSpan span : spans) {
      try {
        ssb.removeSpan(span);
      } catch (Exception e) {
        // do nothing
      }
    }
  }

Let's proceed with the first suggestion, i.e. removing mShouldSkip completely.

[wildan-m]

@tomekzaw Will we get duplicate calls without the flag?
https://stackoverflow.com/questions/17535415/textwatcher-events-are-being-fired-multiple-times

[wildan-m]

@tomekzaw I see why applyMarkdownFormatting is in onTextChanged to be called only once for each change. Let me know if I'm wrong.

On each change:

• If we stick to the current code, applyMarkdownFormatting is invoked once.
• In my approach, applyMarkdownFormatting is invoked twice.
• Without the mShouldSkip flag, applyMarkdownFormatting is invoked four times in afterTextChanged.

Is it acceptable to use the second solution to ensure applyMarkdownFormatting is only called once? beside removeSpan, adding a try-catch block for setSpan might be necessary to handle applying formatting to a deleted range. Your thoughts?

edit:

I think I've found a new approach that might be better. We can call applyMarkdownFormatting on afterTextChanged and introduce mPreviousEditable to compare with existing value and skip apply formatting if the value the same

          if (mPreviousEditable != null && mPreviousEditable.toString().equals(editable.toString())) {
              return;
          }

Solution 3 - Compare prevValue

package com.expensify.livemarkdown;

import android.text.Editable;
import android.text.SpannableStringBuilder;
import android.text.TextWatcher;

import androidx.annotation.NonNull;

public class MarkdownTextWatcher implements TextWatcher {
  private final MarkdownUtils mMarkdownUtils;
  private boolean mShouldSkip = false;
  private Editable mPreviousEditable;

  public MarkdownTextWatcher(@NonNull MarkdownUtils markdownUtils) {
    mMarkdownUtils = markdownUtils;
  }

  @Override
  public void beforeTextChanged(CharSequence s, int start, int count, int after) {

  }

  @Override
  public void onTextChanged(CharSequence s, int start, int before, int count) {
   
  }

  @Override
  public void afterTextChanged(Editable editable) {
      if (editable instanceof SpannableStringBuilder) {
          if (mPreviousEditable != null && mPreviousEditable.toString().equals(editable.toString())) {
              return;
          }
          System.out.println("[wildebug] applyMarkdownFormatting editable: " + editable);
          mMarkdownUtils.applyMarkdownFormatting((SpannableStringBuilder) editable);
          mPreviousEditable = new SpannableStringBuilder(editable);
      }      
  }
}

[wildan-m]

@tomekzaw bump

[tomekzaw]

@wildan-m I was curious why afterTextChanged is called multiple times and read the Stack Overflow thread you've linked to: https://stackoverflow.com/questions/17535415/textwatcher-events-are-being-fired-multiple-times. My understanding is that whenever the underlying editable changes, TextWatcher calls afterTextChanged.

One user suggested that setting android:inputType="textNoSuggestions" eliminates multiple calls. So I guess that the subsequent calls to afterTextChanged are made as a result of text suggestion and spellcheck (which by the way are also treated as spans).

As for the solution with try/catch block around removeSpan – I guess this just ignores the problem so I'm rather against this approach now.

As for mShouldSkip flag – there's an edge case I'd like to notice here – assume that the user has typed "someone@example". This is not a valid email so it will not be underlined. However, the OS can suggest appending ".com" so now the text input value is "[email protected]" (where .com is inside a SuggestionSpan) which is a valid e-mail address so this should be underlined. That's why we can't just skip applyMarkdownFormatting using mShouldSkip flag.

As for the solution with comparing the previous value (if (mPreviousEditable != null && mPreviousEditable.toString().equals(editable.toString())) – note that this just compares the underlying string (without spans). I think this should work correctly in our case so let's proceed with this approach from #469 (comment), please just remove println.

[tomekzaw]

Do we need to store mPreviousEditable as a SpannableStringBuilder? We just use mPreviousEditable.toString() so maybe let's keep it as a string?

How about mPreviousText?

[wildan-m]

That's make sense, please wait

[wildan-m]

done

[wildan-m]

@tomekzaw Thanks for your review. The PR has been updated with unnecessary code removed.

[tomekzaw]

Let's restore the trailing newline

[tomekzaw]

@wildan-m I've left some final suggestions

[tomekzaw]

Suggested change

[tomekzaw]

Thanks for fixing this! I will merge this PR soon.

[tomekzaw]

@wildan-m During testing I found one regression – markdownStyle changes (e.g. link color) are not reflected until the text is changed:

regression.mov

This would cause a regression in E/App when toggling light/dark mode.

I was able to fix this by resetting mPreviousText in onTextChanged:

   @Override
   public void onTextChanged(CharSequence s, int start, int before, int count) {
+    mPreviousText = null;
   }

This could be also performed in beforeTextChanged for better consistency.

Here's the correct behavior with this change:

correct.mov

Alternatively, we could check if MarkdownStyle inside MarkdownUtils has not changed in the condition.

edit: Finally, we can just get rid of this optimization at all:

   @Override
   public void afterTextChanged(Editable editable) {
     if (editable instanceof SpannableStringBuilder) {
-       String currentText = editable.toString();
-      if (currentText.equals(mPreviousText)) {
-        return;
-      }
       mMarkdownUtils.applyMarkdownFormatting((SpannableStringBuilder) editable);
-      mPreviousText = currentText;
     }
   }

[tomekzaw]

Let's also remove this field as it's no longer used.

[wildan-m]

Finally, we can just get rid of this optimization at all

@tomekzaw is it ok to call the apply markdown four times? #469 (comment)

• Without the mShouldSkip flag, applyMarkdownFormatting is invoked four times in afterTextChanged.

checking the regression issue

[wildan-m]

@tomekzaw I believe using a try-catch solution is the most practical approach here. Ignoring duplicate calls could impact performance, while adding a debounce might introduce noticeable delays even with a 100ms delay.

This issue typically requires a debouncer, but it may not be relevant in this case. Here is the result:

Kapture.2024-09-11.at.20.25.49.mp4

What are your thoughts? is it ok to move with try-catch approach?

[tomekzaw]

is it ok to call the apply markdown four times?

@wildan-m I think we can call applyMarkdownFormatting several times. If this introduces noticeable performance regressions, we'll address it in a separate issue and separate PR. For now, let's focus on correctness of the formatting. What do you think?

Also, have you checked my suggestion with resetting mPreviousText in beforeTextChanged? I think this approach will still call applyMarkdownFormatting only once and should prevent the described regression.

I'm quite astonished with how debouncing works – I thought debouncing would be a poor user experience but this doesn't look bad at all. However, one of the best features of live-markdown library is synchronous formatting – but maybe we could add debouncing as a new separate feature? Would you like to share the debouncing code with me in form of a patch or a draft PR?

[wildan-m]

@tomekzaw resetting mPreviousText will call apply formatting twice on each change

Kapture.2024-09-11.at.20.59.18.mp4

let's focus on correctness of the formatting. What do you think?

In that case, I suggest keeping the shouldSkip flag and reverting to our original solution to minimize unnecessary comparisons. Is that alright with you?

https://github.com/Expensify/react-native-live-markdown/blob/0dfd7b9d6576968f80cdbb59f7144fc5b58afa64/android/src/main/java/com/expensify/livemarkdown/MarkdownTextWatcher.java

This is the debouncer snippet

debounce solution

package com.expensify.livemarkdown;

import android.os.Handler;
import android.os.Looper;
import android.text.Editable;
import android.text.SpannableStringBuilder;
import android.text.TextWatcher;

import androidx.annotation.NonNull;

public class MarkdownTextWatcher implements TextWatcher {
  private final MarkdownUtils mMarkdownUtils;
  private final Handler mHandler = new Handler(Looper.getMainLooper());
  private final long DEBOUNCE_DELAY = 100; // 300 milliseconds delay
  private Runnable mRunnable;


  public MarkdownTextWatcher(@NonNull MarkdownUtils markdownUtils) {
    mMarkdownUtils = markdownUtils;
  }

  @Override
  public void beforeTextChanged(CharSequence s, int start, int count, int after) {

  }

  @Override
  public void onTextChanged(CharSequence s, int start, int before, int count) {

  }

  @Override
  public void afterTextChanged(final Editable editable) {
    if (editable instanceof SpannableStringBuilder) {
      if (mRunnable != null) {
        mHandler.removeCallbacks(mRunnable);
      }

      mRunnable = new Runnable() {
        @Override
        public void run() {
          System.out.println("[wildebug] applyMarkdownFormatting");
          mMarkdownUtils.applyMarkdownFormatting((SpannableStringBuilder) editable);
        }
      };

      mHandler.postDelayed(mRunnable, DEBOUNCE_DELAY);
    }
  }
}

[tomekzaw]

Regression found, not ready to be merged yet

[wildan-m]

@tomekzaw the optimization has been removed

[tomekzaw]

@wildan-m Thanks again for your PR. This is now ready to merge. Let's focus on performance improvements in a separate PR.

[tomekzaw]

Regression fixed on Android ✅

Screen.Recording.2024-09-12.at.09.37.04.mov

[tomekzaw]

Issue fixed ✅

Screen.Recording.2024-09-12.at.09.38.51.mov

[tomekzaw]

No regressions with height calculation while typing ✅ (content jumps on newlines also happen on main branch)

Screen.Recording.2024-09-12.at.09.40.19.mov