hang with 100% CPU on keyboard arrow trough text field

RESOLVED FIXED in mozilla1.9.1b2

Status

()

defect
--
critical
RESOLVED FIXED
11 years ago
2 months ago

People

(Reporter: erwin, Assigned: uriber)

Tracking

({hang, regression, testcase})

Trunk
mozilla1.9.1b2
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(4 attachments)

Reporter

Description

11 years ago
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

Comment 2

11 years ago
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
Posted 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
Assignee

Comment 6

11 years ago
Posted 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)
Assignee

Comment 7

11 years ago
Posted 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+
Assignee

Updated

11 years ago
Flags: in-testsuite?
Assignee

Updated

11 years ago
OS: Windows Vista → All
Hardware: PC → All
Version: 1.9.0 Branch → Trunk
Assignee

Comment 9

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

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Posted 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.
Assignee

Comment 11

11 years ago
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.
Assignee

Comment 12

11 years ago
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.
Assignee

Comment 14

11 years ago
The test still passes for me when it's removed (i.e., I do "addLoadEvent(focusarea);").
Component: Event Handling → User events and focus handling
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.