586662 - Window locks up when textarea keypress event handler sets overflow style to hidden

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[cplarosa]

User-Agent:       Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3) Gecko/20100805 Firefox/4.0b3
Build Identifier: Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3) Gecko/20100805 Firefox/4.0b3

Browser window locks up when textarea overflow style is set to hidden in a keypress event handler.

Reproducible: Always

Steps to Reproduce:
1.  Create a page with the following HTML code:
    <TEXTAREA onkeypress="this.style.overflow = 'hidden'"></TEXTAREA>
2.  Open the web page, click in the text box, and type a single character
3.  The cursor stops blinking, and the character is not entered in the box
Actual Results:  
The cursor stops blinking and the browser page stops accepting input.  The page will also not repaint if the browser in minimized and restored, so it appears that the window has locked up.  If the page contains multiple input fields, you cannot select or enter data into any of them.  The only thing you can do is close the window.

Expected Results:  
The window continues to accept input and behave normally.

This problem occurs in Firefox 4 beta 1, 2, and 3.  It does not occur in Firefox 3.x or other browsers.

Comment 1

[cplarosa]

Attachment: File containing the test case described in the bug report

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

This regressed within:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b219912edfec&tochange=ac1ed3f6b2e7
Bug 534785?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Seems plausible, yes.  Focus goes into lala-land, perhaps?

Comment 4

[Timothy Nikkel (:tnikkel)]

I have a feeling this is a similar issue to bug 592663.

Comment 5

[Timothy Nikkel (:tnikkel)]

I get these assertions, similar to bug 592663. The unbalanced update view batch count would explain most of the problems here.

###!!! ASSERTION: bad action nesting!: 'mActionNesting>0', file ../../../../editor/libeditor/text/nsTextEditRules.cpp, line 250
###!!! ASSERTION: bad state: 'mUpdateCount > 0', file ../../../../editor/libeditor/base/nsEditor.cpp, line 4080
###!!! ASSERTION: Someone forgot to call EndUpdateViewBatch!: '!mRootVM', file ../../dist/include/nsIViewManager.h, line 297

Comment 6

[Neil Deakin]

I don't get the last assertion on Mac, but definitely looks like painting/updating just stops happening. Focus and other behaviour is still happening properly, but there just isn't any visible effect.

Comment 7

[Timothy Nikkel (:tnikkel)]

Bug 240933 would probably fix most of the problems here.

Comment 8

[(mostly gone) XtC4UaLL [:xtc4uall]]

(In reply to comment #7)
> Bug 240933 would probably fix most of the problems here.

You're right. I got following Progression Range on MC where the Issue is fixed.
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=886665dec3cb&tochange=f2f217290bd0
So when Bug 240933 relands this should be okay. Does this make Bug 240933 a Blocker now? :)

Comment 9

[(no longer active)]

Bug 240933 fixes this in the sense that the browser doesn't lock up, but still the first character typed is lost.

And I get a lot of "ASSERTION: Bad mBatching: 'mBatching >=0'" assertions.

Comment 10

[(no longer active)]

So, the reason this happens is that we flush here: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#4209>, which causes the textarea to be reframed, which will reset the selection object, initializing mBatching to 0, and the next EndBatchChanges call will try to decrement it to -1.

This flush is coming from bug 393723.  Boris, do you think that this is still necessary?

A dirty hack which might work here is to put a script blocker in nsPlaintextEditor::TypedText...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Or to always do the scroll async in ScrollSelectionIntoView, right?

I think that as long as we keep doing sync scrolls we need the flush there, though you could try taking it out and seeing whether bug 393723 reappears.

Comment 12

[(no longer active)]

Attachment: Part 1: fix the assertions

(In reply to comment #11)
> I think that as long as we keep doing sync scrolls we need the flush there,
> though you could try taking it out and seeing whether bug 393723 reappears.

I tried it, and it seems that backing out bug 393723 doesn't cause the problem to reappear.  I added a test case for that bug to make sure that we don't regress this in the future.

With this patch, I don't get any assertions, but still the first character typed into the field gets eaten up.  I'm still investigating that.

Comment 13

[(no longer active)]

Attachment: Part 2: Always do async scroll

In fact, it seems that we can always do an async scroll.  This patch fixes the key entry problem, and with this, the bug can be considered fully fixed.

Comment 14

[(no longer active)]

Attachment: Part 3: Don't attempt to compare unrelated points

These patches caused the assertion count in <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/crashtests/448329-3.html?force=1> to jump up on Mac.  So, I thought to go ahead and fix the assertions instead of just increasing them in the manifest.

Comment 15

[(no longer active)]

This also depends on bug 174823, because if we refresh immediately, we might end up flushing as well, which is what happens on Windows and Linux, according to the try server.

I'll post a patch to that bug shortly.

Comment 16

[(no longer active)]

Attachment: Part 3: Don't attempt to compare unrelated points

With one other assertion count adjusted.

Comment 17

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

Comment on attachment 477818 [details] [diff] [review]
Part 3: Don't attempt to compare unrelated points

+  if (!aCompareNode->IsInDoc() || !end->IsInDoc() ||
+      aCompareNode->GetOwnerDoc() != end->GetOwnerDoc()) {

Just check aCompareNode->GetCurrentDoc() != end->GetCurrentDoc() || !end->GetCurrentDoc().

Comment 18

[(no longer active)]

Attachment: Part 3: Don't attempt to compare unrelated points

With comments addressed.

Comment 20

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/b138e7c0fdf2
http://hg.mozilla.org/mozilla-central/rev/eb27a2b76c40
http://hg.mozilla.org/mozilla-central/rev/ed8fed2ca7af