1470786 - Duplicated text when typing in a website's textbox

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Emily Kager [:ekager]]

Reported on Slack - "When typing into the Google search box on google.com, each previous keypress gets repeated. So, typing "Berlin" actually gives "BBeBerBerlBerliBerlin". Is this already a known issue? I am on recent Nightly." 

Can also reproduce on github search, etc and with GeckoView Example app.

Comment 1

[Chris Peterson [:cpeterson]]

Emily, GV engineers can't reproduce this behavior. What Android device and OS version are you using? Are you using a third-party keyboard app?

Comment 2

[Sören Hentzschel]

My device is a HTC U11 with Android 8.0. Keyboard ist TouchPal - that's the default keyboard on current HTC devices. According to another user on GitHub the problem also exists with the LG G6 and Android 8.0.

Comment 3

[Emily Kager [:ekager]]

Attachment: Gif of text repeating

I can reproduce on emulator with x86 builds on Nexus 4 API22 and have attached a gif of the behavior.

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

We've had issues like this in the past. I see this on youtube.com when typing in their search bar on a Galaxy S8 US w/ Samsung keyboard w/ Focus Geckoview.

Comment 5

[Chris Peterson [:cpeterson]]

[geckoview:klar:p1] because this is a Focus+GV blocker.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1470786 - 1. Support async text changes from replacing text;

Currently, we expect editing operations in
GeckoEditableSupport::OnImeReplaceText to cause synchronous text change
notifications. However, under e10s, text change notifications can be
asynchornous. The new code keeps track of active OnImeReplaceText calls,
and look for async text changes before replying to the calls.

Review commit: https://reviewboard.mozilla.org/r/256970/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256970/

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1470786 - 2. Fix a text sync issue;

Fix an issue in GeckoEditable where spans can be mistakenly deleted when
deleting text. The new code re-copies all spans to make sure similar
issues don't happen in the future.

Review commit: https://reviewboard.mozilla.org/r/256972/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256972/

Comment 8

[Eugen Sawin [:esawin]]

Comment on attachment 8992052 [details]
Bug 1470786 - 1. Support async text changes from replacing text;

https://reviewboard.mozilla.org/r/256970/#review264124

Comment 9

[Eugen Sawin [:esawin]]

Comment on attachment 8992053 [details]
Bug 1470786 - 2. Fix a text sync issue;

https://reviewboard.mozilla.org/r/256972/#review264126

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8992052 [details]
Bug 1470786 - 1. Support async text changes from replacing text;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256970/diff/1-2/

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8992053 [details]
Bug 1470786 - 2. Fix a text sync issue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256972/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4c93d40e9aa
1. Support async text changes from replacing text; r=esawin
https://hg.mozilla.org/integration/autoland/rev/82da16c50e25
2. Fix a text sync issue; r=esawin

Comment 13

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/d4c93d40e9aa
https://hg.mozilla.org/mozilla-central/rev/82da16c50e25

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8992052 [details]
Bug 1470786 - 1. Support async text changes from replacing text;

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Text input in general is buggy under e10s GeckoView
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Slightly
[Why is the change risky/not risky?]: The code fixes text input behavior for GeckoView (e10s) but should have no impact on Fennec (under non-e10s) 
[String changes made/needed]: None

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8992052 [details]
Bug 1470786 - 1. Support async text changes from replacing text;

Geckoview fix, let's uplift for beta 11.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8992053 [details]
Bug 1470786 - 2. Fix a text sync issue;

[Triage Comment]

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7e6f51f7fca4
https://hg.mozilla.org/releases/mozilla-beta/rev/ddfa017fb3b0

Comment 18

[Sorina Florean [:sflorean]]

Tested with HTC Desire 820(Android 6.0.1) and HTC 10(Android 8.0.0) on latest Beta build (62.0b11) and couldn't reproduce the issue.

Comment 19

[Sorina Florean [:sflorean]]

Verified as fixed on latest Nightly build (08/16).
Device: OnePlus 5T(Android 8.1.0).

Comment 20

[Sören Hentzschel]

Reopening because the bugfix has been reverted in bug 1505161 and makes Firefox Focus unusable again. See https://github.com/mozilla-mobile/focus-android/issues/3965.

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

Matt said he'd take this - thanks!

Comment 22

[Matt Brubeck (:mbrubeck)]

Current status:

- This fix was backed out of 63 (Release) and 64 (Beta), so those branches are affected (again).

- On 65 (Nightly), bug 1490391 fixed the regression instead, so no back-out was needed.  65 is unaffacted.

In bug 1490391 comment 30, jchen said the release/beta backout should include "an exception for Focus."  Chris, can we re-land this fix on a GV-only or Focus-only branch for 63 and/or 64?  Or would we need to add a conditional in the code, so that the same source code works for both GV and Fennec?

Alternately, we could reconsider bug 1490391 for uplift.

Comment 23

[Makoto Kato [:m_kato]]

(In reply to Matt Brubeck (:mbrubeck) from comment #22)
>
> Alternately, we could reconsider bug 1490391 for uplift.

Although I am investigating bug 1508552, I believe that landing this and bug 1490391 to beta is better.

Comment 24

[Chris Peterson [:cpeterson]]

(In reply to Makoto Kato [:m_kato] from comment #23)
> (In reply to Matt Brubeck (:mbrubeck) from comment #22)
> >
> > Alternately, we could reconsider bug 1490391 for uplift.
> 
> Although I am investigating bug 1508552, I believe that landing this and bug
> 1490391 to beta is better.

Makoto, if we uplift a fix for bug 1508552 and bug 1490391 to 64 Beta, will we still need to limit the bug 1490391 fix to just GV or Focus?

If so, I can work with Relman to create a new GV-only branch of 64 Beta. Creating a beta branch will be a hassle. Alternately, since 64 Release is only two weeks away (December 10), we could wait and create the GECKOVIEW_64_RELBRANCH (in mozilla-release) that we will probably need anyways for Focus.

What do you recommend?

(In reply to Matt Brubeck (:mbrubeck) from comment #22)
> In bug 1490391 comment 30, jchen said the release/beta backout should
> include "an exception for Focus."  Chris, can we re-land this fix on a
> GV-only or Focus-only branch for 63 and/or 64?  Or would we need to add a
> conditional in the code, so that the same source code works for both GV and
> Fennec?

Comment 25

[Makoto Kato [:m_kato]]

(In reply to Chris Peterson [:cpeterson] from comment #24)
> (In reply to Makoto Kato [:m_kato] from comment #23)
> > (In reply to Matt Brubeck (:mbrubeck) from comment #22)
> > >
> > > Alternately, we could reconsider bug 1490391 for uplift.
> > 
> > Although I am investigating bug 1508552, I believe that landing this and bug
> > 1490391 to beta is better.
> 
> Makoto, if we uplift a fix for bug 1508552 and bug 1490391 to 64 Beta, will
> we still need to limit the bug 1490391 fix to just GV or Focus?

Yes and No.  Hmm, I need test it for GV.  Now although I test bug 1493428's situation on GV 62 (Focus 7.0. This bug's fix is landed), it has another problem. (Some IMEs works well, but POBOX (bug 1493428) doesn't work well.  It seems to be just bug 1490391)

So I will answer it tomorrow again after testing it.  (inc. I will test whether bug 1490391 occurs on GV 64 after re-landing this fix)

> If so, I can work with Relman to create a new GV-only branch of 64 Beta.
> Creating a beta branch will be a hassle. Alternately, since 64 Release is
> only two weeks away (December 10), we could wait and create the
> GECKOVIEW_64_RELBRANCH (in mozilla-release) that we will probably need
> anyways for Focus.

Since 64 release is within 2 weeks, GV only branch is better.  After GV branch is created, we should re-land this to it since this bug is fixed at least.

BTW, what release date of Firefox Focus 8?

Comment 26

[Chris Peterson [:cpeterson]]

(In reply to Makoto Kato [:m_kato] from comment #25)
> Since 64 release is within 2 weeks, GV only branch is better.  After GV
> branch is created, we should re-land this to it since this bug is fixed at
> least.
> 
> BTW, what release date of Firefox Focus 8?

Focus 8's release date is December 17. GV 64's release date is December 11. The Focus team will probably want a fix in GV 64 Beta before December 11 so they have more beta test time for GV 64 in Focus 8 Beta.

I will ask Releng about making a GV-only beta branch.

Comment 27

[Chris Peterson [:cpeterson]]

Makoto, if these IME bugs affect Focus+GV but not Fennec, will they affect Custom Tabs that use Fennec's GV? If so, then maybe we do need a run-time check for an IME workaround?

Comment 28

[Makoto Kato [:m_kato]]

(In reply to Chris Peterson [:cpeterson] from comment #27)
> Makoto, if these IME bugs affect Focus+GV but not Fennec, will they affect
> Custom Tabs that use Fennec's GV? If so, then maybe we do need a run-time
> check for an IME workaround?

This issue depends on e10s.  Custom tab doesn't use e10s. (https://searchfox.org/mozilla-central/rev/0859e6b10fb901875c80de8f8fc33cbb77b2505e/mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java#127)  Focus will use e10s.

Comment 29

[Makoto Kato [:m_kato]]

(In reply to Makoto Kato [:m_kato] from comment #25)
> (In reply to Chris Peterson [:cpeterson] from comment #24)
> > (In reply to Makoto Kato [:m_kato] from comment #23)
> > > (In reply to Matt Brubeck (:mbrubeck) from comment #22)
> > > >
> > > > Alternately, we could reconsider bug 1490391 for uplift.
> > > 
> > > Although I am investigating bug 1508552, I believe that landing this and bug
> > > 1490391 to beta is better.
> > 
> > Makoto, if we uplift a fix for bug 1508552 and bug 1490391 to 64 Beta, will
> > we still need to limit the bug 1490391 fix to just GV or Focus?
> 
> Yes and No.  Hmm, I need test it for GV.  Now although I test bug 1493428's
> situation on GV 62 (Focus 7.0. This bug's fix is landed), it has another
> problem. (Some IMEs works well, but POBOX (bug 1493428) doesn't work well. 
> It seems to be just bug 1490391)
> 
> So I will answer it tomorrow again after testing it.  (inc. I will test
> whether bug 1490391 occurs on GV 64 after re-landing this fix)

Today, I test some fixes and environments.

bug 1490391 is for non-e10s.  Even if landing bug 1490391, it isn't unrelated to Focus, so it is unnecessary for Focus  So if we have GV only branch, it is OK to re-land this bug (bug 1470786) only.

Although some input methods still have bugs that composing string is committed unexpectedly, it isn't fixed even if the latest GV 65.  So I will investigate it by bug 1510527.

Comment 30

[Chris Peterson [:cpeterson]]

In anticipation of the creation of the GECKOVIEW_64_RELBRANCH next week, I am moving this bug from status-firefox64=affected to status-geckoview64=affected.

Comment 31

[Chris Peterson [:cpeterson]]

Matt or Makoto, do these patches need to be uplifted to the GV 64 relbranch? If so, can you please request approval-mozilla-geckoview64 uplift approval so RyanVM knows land these patches when he creates the GV 64 relbranch on December 3?

Comment 32

[Makoto Kato [:m_kato]]

(In reply to Chris Peterson [:cpeterson] from comment #31)
> Matt or Makoto, do these patches need to be uplifted to the GV 64 relbranch?
> If so, can you please request approval-mozilla-geckoview64 uplift approval
> so RyanVM knows land these patches when he creates the GV 64 relbranch on
> December 3?

I will add revert patch for uplifting to GV64 relbranch.

Comment 33

[Makoto Kato [:m_kato]]

mbrubeck, do you already have revert patch for this?   Since I need sync 64 release branch from https://hg.mozilla.org/releases/mozilla-release.  If no, I will sync tree, then I will create a patch.

Comment 34

[Matt Brubeck (:mbrubeck)]

The original patch here applies cleanly to the 64 release branch, using these commands:

hg pull release
hg update GECKOVIEW_64_RELBRANCH
hg import https://hg.mozilla.org/releases/mozilla-beta/rev/7e6f51f7fca4

I'll request approval on the existing patch.

Comment 35

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8992052 [details]
Bug 1470786 - 1. Support async text changes from replacing text;

[GeckoView Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for consideration: This fixes a critical bug in Android apps that use GeckoView.

User impact if declined: Users of Firefox Focus are unable to enter text in web pages using certain devices or keyboards.

Fix Landed on Version: 65

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Risk is minimal because this is landing on a GeckoView-only branch.  This patch has been in Nightly 65 for several months, and previously landed in Beta 64 for several weeks.  In both branches, it did not cause any problems in GeckoView apps; it was backed out only as a low-risk fix for a non-GeckoView regression.

String or UUID changes made by this patch: n/a

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Approving for GV64, calling this fixed for 65+ by bug 1490391.

Comment 37

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/6891b272d3fd