449076 - hang with 100% CPU on keyboard arrow trough text field

Status: RESOLVED FIXED

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

Description

[eddie]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; nl; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; nl; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

I have made some text in my reaction form, when I tried to reset the cursur somewhere in the text nothing happened, when I used the arrow keys on my keyboard firefox freezes.
This only happens in the text form with "Reactie:" next to it (see URL, Tip: use arrow keys on my site to scroll down to reaction form!)

Reproducible: Always

Steps to Reproduce:
1. Scroll to bottom (reaction form).
2. Type some text in the text form (next to "Reactie:".
(optionel) 3. Try to put the cursor somewhere in the text (mouse).
4. Nothing happens, so try it with the arrow keys (keyboard)
Actual Results:  
Firefox freezes

Expected Results:  
put the cursor somewher in the field (specified place)

Comment 1

[Arie Paap [:wildmyron]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1 ID:2008070208

confirmed with slight modification to STR:
1. load URL
2. scroll to bottom of frame with scroll arrows on RHS
3. click in text field labeled "Reactie:"
4. Press down arrow key followed by up arrow key

Actual results:
Hang with 100% CPU

marking qawanted for minimised testcase

Comment 2

[Tobias Fischer]

This affects the Trunk too, reproduced using the steps from C#1 with: 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/2008080400 Mnenhy/0.7.5.20005 SeaMonkey/2.0a1pre

Comment 3

[Arie Paap [:wildmyron]]

Attachment: Minimal testcase

Combination of '-moz-user-select: none' on the body, 'clear: left' and input + textarea required.

Similar str:
1) load testcase
2) click mouse in textarea
3) Press up arrow followed by down arrow

Comment 4

[Ria Klaassen (not reading all bugmail)]

Works: 2006083004
Fails: 2006083009

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-08-30+03%3A00&maxdate=2006-08-30+10%3A00

I guess from bug 334626.

Comment 5

[Arie Paap [:wildmyron]]

Some additional details which may be helpful:

replacing the <input> in the testcase with a <textarea></textarea> also hangs (with caret in either textarea)

I looked into the hang and found that the do loop around calls to nsFrame::GetNextPrevLineFromeBlockFrame() never exits. See http://mxr.mozilla.org/firefox/source/layout/generic/nsFrame.cpp#4877 

hth

Comment 6

[Uri Bernstein]

Attachment: patch

Sorry for ignoring this for so long.

This patch might not address the root cause of the hang, but it prevents it by preventing GetLineNumber() from breaking outside of a ScrollFrame, which I think is a good idea anyway.

The behavior of up/down arrows here is still inconsistent with that of left/right (up/down don't allow the caret to be placed inside the unselectable text, whereas left/right do). I'm not sure what the semantics of -moz-user-select:none should be (or even what's the point of having editable-but-not-selectable content).

The eSelectLine case in PeekOffset(), together with GetNextPrevLineFromeBlockFrame(), probably deserve a good rewrite.

Comment 7

[Uri Bernstein]

Attachment: direct fix

This is a more direct fix to the problem:
In our case, nsFrameIterator::CurrentItem was returning a null item and NS_ENUMERATOR_FALSE, which is not considered a failure. This (apparently) confuses the code in PeekOffset(), which keeps trying forever.

I'm not exactly sure about the correctness of this patch, as the convoluted logic here is beyond my grasp right now. Even if we take this fix, I still think we should also take the one I suggested earlier.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 341742 [details] [diff] [review]
patch

Can we have a mochitest for this?

Comment 9

[Uri Bernstein]

Fixed by http://hg.mozilla.org/mozilla-central/rev/729436f1f0d3. I'll work on a mochitest (I might need some help).

Comment 10

[Martijn Wargers (dead)]

Attachment: mochitest

This mochitest hangs in my debug build, because it doesn't contain the patch in this bug yet. Because my build doesn't contain the patch yet, I'm not really sure if the ok() tests are ok. But it seems to me they should pass with this patch. So if you could check that, Uri, that would be nice.

Comment 11

[Uri Bernstein]

Comment on attachment 342413 [details] [diff] [review]
mochitest

Thanks, Martijn!
You're missing a "[0]" at the end of line 39, but once I fixed that, the test passed for me.
Also, please remember to change the bug number everywhere from 448860 to 449076.

Comment 12

[Uri Bernstein]

BTW, why do you need the setTimeout() here (as opposed to calling focusarea() directly)?

Comment 13

[Martijn Wargers (dead)]

I didn't really need the setTimeout, it's just something that I kept from the original. I didn't really bother checking if it could be removed. If it can be removed, that's fine.

Comment 14

[Uri Bernstein]

The test still passes for me when it's removed (i.e., I do "addLoadEvent(focusarea);").