Closed Bug 978918 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Runtime, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.4+, firefox29 wontfix, firefox30 fixed, firefox31 fixed, b2g18 unaffected, b2g-v1.3 unaffected, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S4 (28mar)
blocking-b2g 1.4+
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
b2g18 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: jharvey, Assigned: wdeng)

References

()

Details

(Keywords: regression, Whiteboard: burirun1.4-1)

Attachments

(2 files, 3 obsolete files)

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
1. Does this repro on 1.1?
2. The video here does not work for me. Can you include a new video?
Keywords: qawanted
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
Attached video Keyboard Issue.mp4
Keywords: qawanted
blocking-b2g: --- → 1.3?
What about 1.3?
blocking-b2g: 1.3? → 1.4?
(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.
blocking-b2g: 1.4? → 1.3?
Ivan

Please reassign.

1.3+
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(itsay)
QA Contact: mvaughan
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.
blocking-b2g: 1.3+ → 1.4+
Looks like this was probably caused by bug 952724.
Blocks: 952724
Rudy,

Please help on this 1.4 regression blocker bug on keyboard. Thanks.
Assignee: nobody → rlu
Flags: needinfo?(itsay)
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.
Flags: needinfo?(xyuan)
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.
Flags: needinfo?(xyuan)
(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.
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");
    }
  ...
}
Assignee: rlu → xyuan
Status: NEW → ASSIGNED
Yuan, thanks for the investigation.
I'll change the component to reflect this finding.
Component: Gaia::Keyboard → General
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.
(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?
Component: General → Runtime
By a few days' investigation with Wei Deng(wdeng@mozilla.com), 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.
Assignee: xyuan → wdeng
Flags: needinfo?(wdeng)
The cause of this bug is the same as that adressed in https://bugzilla.mozilla.org/show_bug.cgi?id=941329#c7
Attached patch gecko patch v1 (obsolete) — Splinter Review
Sometimes we can't change the selection range, it result in an infinite loop. We should break the loop in this case.
Attachment #8393304 - Flags: review?(xyuan)
Attachment #8393304 - Flags: feedback?(fabrice)
Flags: needinfo?(wdeng)
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.
(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.
Attachment #8393304 - Flags: review?(xyuan) → review+
Attached patch gecko patch v2 (obsolete) — Splinter Review
fixed the indentation and changed a varable name
Attachment #8393304 - Attachment is obsolete: true
Attachment #8393304 - Flags: feedback?(fabrice)
Attachment #8393357 - Flags: review+
Attachment #8393357 - Flags: feedback?(fabrice)
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?
Attachment #8393357 - Flags: feedback?(fabrice) → feedback+
Attached patch gecko patch v3 (obsolete) — Splinter Review
added comment
Attachment #8393357 - Attachment is obsolete: true
Attachment #8394038 - Flags: review+
Attachment #8394038 - Flags: feedback+
Attached patch gecko patch v3Splinter Review
Attachment #8394038 - Attachment is obsolete: true
Attachment #8394047 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/929c248096f6

Please update your hg commit information to include your full name.
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/929c248096f6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S4 (28mar)
You need to log in before you can comment on or make changes to this bug.