Closed Bug 449076 Opened 12 years ago Closed 12 years ago

hang with 100% CPU on keyboard arrow trough text field

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: erwin, Assigned: uriber)

References

()

Details

(Keywords: hang, regression, testcase)

Attachments

(4 files)

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)
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
Status: UNCONFIRMED → NEW
Component: General → Event Handling
Ever confirmed: true
Keywords: hang, qawanted
Product: Firefox → Core
Summary: crash on keyboard arrow trough text field → hang with 100% CPU on keyboard arrow trough text field
Version: unspecified → 1.9.0 Branch
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
Attached file 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
Keywords: qawantedtestcase
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
Attached patch patchSplinter Review
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.
Assignee: nobody → uriber
Status: NEW → ASSIGNED
Attachment #341742 - Flags: review?(roc)
Attached patch direct fixSplinter Review
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 on attachment 341742 [details] [diff] [review]
patch

Can we have a mochitest for this?
Attachment #341742 - Flags: superreview+
Attachment #341742 - Flags: review?(roc)
Attachment #341742 - Flags: review+
Flags: in-testsuite?
OS: Windows Vista → All
Hardware: PC → All
Version: 1.9.0 Branch → Trunk
Fixed by http://hg.mozilla.org/mozilla-central/rev/729436f1f0d3. I'll work on a mochitest (I might need some help).
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Attached patch mochitestSplinter Review
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 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.
BTW, why do you need the setTimeout() here (as opposed to calling focusarea() directly)?
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.
The test still passes for me when it's removed (i.e., I do "addLoadEvent(focusarea);").
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.