978918 - [B2G][Keyboard] Double tapping the space bar after an auto correction makes the messenger app unusable

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

[Harvey]

Description: Double tapping the space button during an auto correction will sometimes render the Messenger app unusable. 


Repro Steps:
1) Updated Buri to BuildID: 20140227040202
2) Select the messenger app.
3) Select the new message icon.
4) Tap on the message field.
5) Tap on random letters that do not make up a word 
6) Quickly press space twice.

Actual:
The messenger app is unusable.

Expected:
The messenger app always functions correctly.

NOTE: If the app does not freeze the first time, delete the letters and repeat steps 5 & 6.

Environmental Variables:
Device: Buri v1.4 Mozilla
BuildID: 20140227040202
Gaia: 22d48b62df7901ad45044f66e15e7d8943884a06
Gecko: a98a1d78817f
Version: 30.0a1
Firmware Version: v1.2-device.cfg


Repro frequency: 4/5
Link to failed test case: https://moztrap.mozilla.org/manage/case/6527/
See attached: http://www.youtube.com/watch?v=gEXTpHmx58s&feature=youtu.be

Comment 1

[Jason Smith [:jsmith]]

1. Does this repro on 1.1?
2. The video here does not work for me. Can you include a new video?

Comment 2

[Harvey]

This issue does not reproduce on 1.1

Environmental Variables:
Device: Buri v1.1 Mozilla
BuildID: 20140218041202
Gaia: c5cb252e5d1aa45d14f3a2ea182e73e2271e4496
Gecko: d7f2811943d1
Version: 18.0
Firmware Version: v1.2-device.cfg

New video: Keyboard Issue.mp4

Comment 3

[Harvey]

Attachment: Keyboard Issue.mp4

Comment 4

[Jason Smith [:jsmith]]

What about 1.3?

Comment 5

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #4)
> What about 1.3?

Actually forgot that this was already marked as 1.3 affected. Disregard.

Comment 6

[Preeti Raghunath(:Preeti)]

Ivan

Please reassign.

1.3+

Comment 7

[Matthew Vaughan]

This issue started reproducing on the 01/04/14 1.4 build.

- Last Working -
Device: Buri v1.4 MOZ RIL
BuildID: 20140103040201
Gaia: 83cc63f728489a24256731adf558354bb2012a59
Gecko: 49d2fce9a86c
Version: 29.0a1
Firmware Version: V1.2-device.cfg

- First Broken -
Device: Buri v1.4 MOZ RIL
BuildID: 20140104040201
Gaia: fc6080c035c99c1483da693265eac8755e1d8255
Gecko: 8f0d4afd151e
Version: 29.0a1
Firmware Version: V1.2-device.cfg

Swapping the gaia/gecko for the builds yields an inconclusive result (i.e. issue reproduces on both builds after the swap).

Gaia push log: https://github.com/mozilla-b2g/gaia/compare/83cc63f728489a24256731adf558354bb2012a59...fc6080c035c99c1483da693265eac8755e1d8255

Gecko push log: http://hg.mozilla.org//mozilla-central/pushloghtml?fromchange=49d2fce9a86c&tochange=8f0d4afd151e


**NOTE**: I spoke with the reporter and we determined that this issue actually does not reproduce on the 1.3 build. I will adjust the status flags to properly reflect that.

Comment 8

[Jason Smith [:jsmith]]

Looks like this was probably caused by bug 952724.

Comment 9

[Ivan Tsay (:ITsay)]

Rudy,

Please help on this 1.4 regression blocker bug on keyboard. Thanks.

Comment 10

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Found that this could be reproduced with content editable element, but not <input type="text">.
Would be much easier to be reproduced with:
  1. Click on a content editable, and switch to Spanish layout
  2. Type httvb
  3. Double tap space key.
  4. See the word would be auto corrected to "Hito" and then the keyboard app itself would cease to function.


Yuan,

Do you have any clue about the above symptom and if it is possible that bug 952724 has anything to do with this?
Thanks.

Comment 11

[Yuan Xulei [:yxl] (Leave from May 18)]

I'll help you to diagnose this bug tomorrow from gecko part. bug 952724 should not cause this bug, because it doesn't change the API behavior, but only the form of parameters.

Comment 12

[Jason Smith [:jsmith]]

(In reply to Yuan Xulei [:yxl] from comment #11)
> I'll help you to diagnose this bug tomorrow from gecko part. bug 952724
> should not cause this bug, because it doesn't change the API behavior, but
> only the form of parameters.

Note - If you changed API parameters but made an incorrect update in Gaia, then it's possible to get a regression of this type. So I wouldn't rule this out just because the parameter form changed without behavioral modifications - we've already had regressions like this in the past.

The only other possibility would be bug 953044.

Comment 13

[Yuan Xulei [:yxl] (Leave from May 18)]

It is a gecko issue. With the steps of comment 10, setSelectionRange of forms.js (
http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#1005) enters 
an infinite loop and then makes app not responding.

setSelectionRange is used to select a specified range of text. For content editable,
sometimes we failed to move the selection start position to the beginning with 
|sel.collapse(element, 0);|, but |getContentEditableSelectionStart| returns 0. This 
inconsistent state makes the second while loop infinitely.

The related code is:

function setSelectionRange(element, start, end) {
  ...
    // Move the caret to the start position
    sel.collapse(element, 0);
    for (let i = 0; i < start; i++) {
      sel.modify("move", "forward", "character");
    }

    while (getContentEditableSelectionStart(element, sel) < start) {
      sel.modify("move", "forward", "character");
    }

    // Extend the selection to the end position
    for (let i = start; i < end; i++) {
      sel.modify("extend", "forward", "character");
    }

    let selectionLength = end - start;
    let i = 0;
    while (getContentEditableSelectionLength(element, sel) < selectionLength) {
      // getContentEditableSelectionLength always returns 0 and the code goes into infinite loop.
      sel.modify("extend", "forward", "character");
    }
  ...
}

Comment 14

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Yuan, thanks for the investigation.
I'll change the component to reflect this finding.

Comment 15

[Jan Jongboom [:janjongboom] (inactive)]

This can happen when the state in gaia is invalid from gecko, and should be yet another reason why we should not duplicate the state in Gaia.

That it enters an infinite loop is a bug of course.

Comment 16

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jan Jongboom [:janjongboom] from comment #15)
> This can happen when the state in gaia is invalid from gecko, and should be
> yet another reason why we should not duplicate the state in Gaia.
> 
> That it enters an infinite loop is a bug of course.

Do you know what is the cause of the invalid state? I don't understand why gaia enters such a strange state?

Comment 17

[Yuan Xulei [:yxl] (Leave from May 18)]

By a few days' investigation with Wei Deng([e-mail]), the issue is possibly caused by the documentEncoder component, which is used to calculate the cursor position for the content editable element.

I'll assign the bug to Wei and ask him to help resolve this bug.

Comment 18

[Yuan Xulei [:yxl] (Leave from May 18)]

The cause of this bug is the same as that adressed in https://bugzilla.mozilla.org/show_bug.cgi?id=941329#c7

Comment 19

[[e-mail]]

Attachment: gecko patch v1

Sometimes we can't change the selection range, it result in an infinite loop. We should break the loop in this case.

Comment 20

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8393304 [details] [diff] [review]
gecko patch v1

Review of attachment 8393304 [details] [diff] [review]:
-----------------------------------------------------------------

It looks good to me.

I asked Deng Wei to help fix this issue.The bug is caused by the documentEncoder with content editable.

A hidden <br> is added to the end of content editable of the message field (see bug 414223 about how it 
works). We use documentEncoder to calculate text content length of the message field. The documentEncoder 
didn't filter the hidden <br> and gave us a length 1 greater than the actual. When we need to select the 
whole content of the content editable, we tried to extend the selection range 1 greater than the content 
length and went into an infinite while loop.

This patch fixed the documentEncoder issue by dropping the hidden <br> when calculate the text content 
length. It also makes a safe guard for the while loops to avoid endless running.

::: dom/inputmethod/forms.js
@@ +1036,2 @@
>        sel.modify("move", "forward", "character");
> +      let selectionStart = getContentEditableSelectionStart(element, sel);

It should be |start = getContentEditableSelectionStart(element, sel)|

@@ +1060,2 @@
>        sel.modify("extend", "forward", "character");
> +      let newSelectionLength = getContentEditableSelectionLength(element, sel);

It should be |selectionLength = getContentEditableSelectionStart(element, sel)|

::: dom/inputmethod/mochitest/file_test_sms_app.html
@@ +2,5 @@
> +<html>
> +<body>
> +  <div id="messages-input" x-inputmode="-moz-sms" contenteditable="true"
> +  autofocus="autofocus">Httvb<br></div>
> + <script type="application/javascript;version=1.7">

Indentation.

Comment 21

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Yuan Xulei [:yxl] from comment #20)
> Comment on attachment 8393304 [details] [diff] [review]
> gecko patch v1
> 
> Review of attachment 8393304 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks good to me.
> 
> I asked Deng Wei to help fix this issue.The bug is caused by the
> documentEncoder with content editable.
> 
> A hidden <br> is added to the end of content editable of the message field
> (see bug 414223 about how it 
> works). We use documentEncoder to calculate text content length of the
> message field. The documentEncoder 
> didn't filter the hidden <br> and gave us a length 1 greater than the
> actual. When we need to select the 
> whole content of the content editable, we tried to extend the selection
> range 1 greater than the content 
> length and went into an infinite while loop.
> 
> This patch fixed the documentEncoder issue by dropping the hidden <br> when
> calculate the text content 
> length. It also makes a safe guard for the while loops to avoid endless
> running.
> 
> ::: dom/inputmethod/forms.js
> @@ +1036,2 @@
> >        sel.modify("move", "forward", "character");
> > +      let selectionStart = getContentEditableSelectionStart(element, sel);
> 
> It should be |start = getContentEditableSelectionStart(element, sel)|
> 
> @@ +1060,2 @@
> >        sel.modify("extend", "forward", "character");
> > +      let newSelectionLength = getContentEditableSelectionLength(element, sel);
> 
> It should be |selectionLength = getContentEditableSelectionStart(element,
> sel)|
Sorry, I made a mistake. The above lines are correct and need no change.

Comment 22

[[e-mail]]

Attachment: gecko patch v2

fixed the indentation and changed a varable name

Comment 23

[Jan Jongboom [:janjongboom] (inactive)]

\o/

Comment 24

[[:fabrice] Fabrice Desré]

Comment on attachment 8393357 [details] [diff] [review]
gecko patch v2

Review of attachment 8393357 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/inputmethod/forms.js
@@ +24,4 @@
>  });
>  
>  const RESIZE_SCROLL_DELAY = 20;
> +const MAX_BLOCKED_COUNT = 20;

can you add a comment to explain why we use that?

Comment 25

[[e-mail]]

Attachment: gecko patch v3

added comment

Comment 26

[[e-mail]]

Attachment: gecko patch v3

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/929c248096f6

Please update your hg commit information to include your full name.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/929c248096f6

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/026e58a7a241