1838355 - mozilla::ContentEventHandler::GetTextLength takes lots of time in sp3 TipTap

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P2)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The patch for bug 1837482 improves the score about 20%, however, it's not enough since

+++ This bug was initially created as a clone of Bug #1837482 +++

See the profiles, mozilla::ContentEventHandler::GetTextLength takes a lot of time.
https://speedometer-preview.netlify.app/?developerMode=&suites=Editor-TipTap

The new profile:
https://profiler.firefox.com/public/18rz9c4gtc5kphjszkbh562fykwjycpfs45baqr/flame-graph/?globalTrackOrder=0&sourceView=hg%3Ahg.mozilla.org%2Ftry%3Adom%2Fevents%2FIMEContentObserver.cpp%3Ace3c56302c47550cd602ca7978b34dd69a4867d7&thread=0&transforms=fg-8~df-4252~df-789&v=9

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Why do we need to deal with windows line endings in a content process?

It's what is a possible solution. At implementing e10s, ContentEventHandler was kept as-is. Therefore, there is no concrete reason why it needs to handle the line break differences immediately.

Do other browsers care about \r\n?

Safari does not need to do it since not run in Windows. I'm not sure about Chrome, I don't find the code handling \r\n. Even if they don't do it, stop doing it is not safe because IMEs may switch behavior for specific window classes.

Could add/remove \r when dealing with OS level APIs in the parent process?

I think that making TextInputHandler handle it. It may run in content process if a TextInputProcessor works for testing.

Comment 2

[Markus Stange [:mstange]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #1)

Do other browsers care about \r\n?

Safari does not need to do it since not run in Windows. I'm not sure about Chrome, I don't find the code handling \r\n. Even if they don't do it, stop doing it is not safe because IMEs may switch behavior for specific window classes.

How many IMEs are there? Are we set up to check whether our changes break them?
It's not a good situation if window-class-specific IME behavior requires slow paths in Firefox that are not required in other browsers. Is there a way to influence IME changes? Do we have Mozilla contacts at IME makers? What other options do we have if Firefox-specific IME behavior is indeed what requires the \r\n handling here?

Is there a way to run this code only if IMEs are actually being used?

Could add/remove \r when dealing with OS level APIs in the parent process?

I think that making TextInputHandler handle it. It may run in content process if a TextInputProcessor works for testing.

If the line break detection ends up running on the parent process main thread, that's not really an improvement for our users because it'll cause UI jank, no? It would need to run on a background thread.
I think we should take another look at Chrome and see if it has similar code, and if not, dig deep to find out why it doesn't need it.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Markus Stange [:mstange] from comment #2)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #1)

Do other browsers care about \r\n?

Safari does not need to do it since not run in Windows. I'm not sure about Chrome, I don't find the code handling \r\n. Even if they don't do it, stop doing it is not safe because IMEs may switch behavior for specific window classes.

How many IMEs are there? Are we set up to check whether our changes break them?

Cannot count them, there are too many 3rd-party IMEs especially for Simplified/Traditional Chinese.

It's not a good situation if window-class-specific IME behavior requires slow paths in Firefox that are not required in other browsers. Is there a way to influence IME changes?

I think that it's impossible because some IMEs may have already been stopped maintained.

Do we have Mozilla contacts at IME makers? What other options do we have if Firefox-specific IME behavior is indeed what requires the \r\n handling here?

In this point, \r\n is the standard line breaks on Windows, so, IMEs may handle it in their default paths. If some apps return only \n, IME may need additional hack.

On the other hand, IMEs are typically not working with a line breaks because most IME creates a composition in a sentence at most. IIRC, we got some bug reports around line breaks of old Microsoft IME for hand-writing or something.

Is there a way to run this code only if IMEs are actually being used?

TSF interface is used by a11y tools too due to the performance reason of getting full a11y tree.

Could add/remove \r when dealing with OS level APIs in the parent process?

I think that making TextInputHandler handle it. It may run in content process if a TextInputProcessor works for testing.

If the line break detection ends up running on the parent process main thread, that's not really an improvement for our users because it'll cause UI jank, no?

Yes it is, but putting it off allows us to skip outdated notifications. E.g., multiple text changes may be coalesced to one text change.

It would need to run on a background thread.

In these days, we could do it because of ContentCache manages asynchronous composition handling. However, anyway, we need to make ContentEventHandler stop handling it directly first.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Is there a debugging tool that shows what a particular application exposes via the IME api?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)

Is there a debugging tool that shows what a particular application exposes via the IME api?

I don't know.

Chromium does not test line break treatments of ITextStoreACP instance:
https://source.chromium.org/chromium/chromium/src/+/main:ui/base/ime/win/tsf_text_store_unittest.cc

And setter method does not convert line breaks from \r\n to \n at storing it in ITextStoreACP instance:
https://source.chromium.org/chromium/chromium/src/+/main:ui/base/ime/win/tsf_text_store.cc;l=507;drc=a22ee7d4e044e70dc3ade06d2d579153bfebee42

And it's updated here when content may be changed:
https://source.chromium.org/chromium/chromium/src/+/main:ui/base/ime/win/tsf_text_store.cc;l=1302;drc=a22ee7d4e044e70dc3ade06d2d579153bfebee42

And it's retrieved from "client":
https://source.chromium.org/chromium/chromium/src/+/main:ui/base/ime/win/tsf_text_store.cc;l=1219;drc=a22ee7d4e044e70dc3ade06d2d579153bfebee42

So if they do not convert \n to \r\n, the method notifies IME of odd text change ranges at first time if IME sets multi-line text.

However, I've not found where converts it... Could be that they just ignore the issue.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

I built ime-rs and modified the code here https://github.com/saschanaz/ime-rs/blob/bfbe1f251a296f80865cfb6c01dde1a5dca58a33/cpp/SampleIME/TextEditSink.cpp#L19 to dump the text that the ime gets. I confirmed that Firefox is using '\r' in it's text and that Chrome does not.

However, I wasn't able to check what offsets are being provided to the OS. This information doesn't seem obviously exposed via the TSF api.

Masayuki, is there a way that an IME would know that we weren't doing the \n -> \r\n conversion other than by inspecting the contents of GetText() on a ITfRange?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)

I built ime-rs and modified the code here https://github.com/saschanaz/ime-rs/blob/bfbe1f251a296f80865cfb6c01dde1a5dca58a33/cpp/SampleIME/TextEditSink.cpp#L19 to dump the text that the ime gets. I confirmed that Firefox is using '\r' in it's text and that Chrome does not.

Thank you for the check, how about Word, Wordpad and Notepad? I guess that if they also use \n for line breaks, it's safer to change our behavior.

Masayuki, is there a way that an IME would know that we weren't doing the \n -> \r\n conversion other than by inspecting the contents of GetText() on a ITfRange?

As far as I know, there is no such API.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #7)

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)

I built ime-rs and modified the code here https://github.com/saschanaz/ime-rs/blob/bfbe1f251a296f80865cfb6c01dde1a5dca58a33/cpp/SampleIME/TextEditSink.cpp#L19 to dump the text that the ime gets. I confirmed that Firefox is using '\r' in it's text and that Chrome does not.

Thank you for the check, how about Word, Wordpad and Notepad? I guess that if they also use \n for line breaks, it's safer to change our behavior.

I tried Wordpad and Notepad and I believe they both used \n\r. I don't have Word so didn't try it. I tried libreoffice but don't remember the results. I'll recheck.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

I tried Libreoffice again and I couldn't get it to output any \n or \r. Wordpad actually seems to use just \r and not \n\r.

How would you feel about disabling conversion to \n\r on Nightly to see if anyone notices any breakage?

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1838355. Don't translate new lines for IME on Nightly.

This translation shows up prominently in the TipTap
Speedometer3 test. Chrome doesn't seem to do this translation
and other Windows programs also show inconsistency around
new lines.

Let's try disabling it on Nightly to check if anything breaks.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)

I tried Libreoffice again and I couldn't get it to output any \n or \r. Wordpad actually seems to use just \r and not \n\r.

How would you feel about disabling conversion to \n\r on Nightly to see if anyone notices any breakage?

Okay, then, it could be possible, but I think still this is risky. So I recommend to ship this with A/B testing, etc.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5a62ea631e5
Don't translate new lines for IME on Nightly. r=masayuki

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/a5a62ea631e5