236270 - Hang when drag-selecting text over a <textarea> inside a <fieldset>

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Mats Palmgren (inactive)]

Hang when drag-selecting text over a <textarea> inside a <fieldset>

STEPS TO REPRODUCE:
1. Load the attached testcase
2. Left-click the word "Start" and hold the mouse button down.
3. Move the mouse up and down over the text area.

REPRODUCABLE: Always

ACTUAL RESULTS:
Mozilla consumes 100% CPU. Running a debug build fills the console with:

###!!! ASSERTION: Null current node in an iterator that's not done!: 'mCurNode',
file nsContentIterator.cpp, line 1183
Break: at file nsContentIterator.cpp, line 1183

EXPECTED RESULTS:
Mozilla consumes ~0% CPU selecting/deselecting the text ;-)

PLATFORMS & BUILDS TESTED:
Bug occurs in Mozilla 2004-03-02-08 trunk on Linux.
Bug occurs in Mozilla 1.7a on Linux.
Bug does NOT occur in Mozilla 1.6, 1.5, 1.4.1 and 1.2.1 on Linux.

ADDITIONAL INFORMATION:
It does not seem to occur if I change the testcase to:
  <textarea rows="20" cols="50"></textarea>
(i.e. empty - see Testcase #2)

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #1 (hang)

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase #2 (does not hang)

Comment 3

[Boris Zbarsky [:bzbarsky]]

This works in 2004-01-23-09 and is broken in 2004-01-24-09.  Relevant bonsai
window: 
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-01-23+08%3A00%3A00&maxdate=2004-01-24+09%3A00%3A00&cvsroot=%2Fcvsroot

Comment 4

[Boris Zbarsky [:bzbarsky]]

My money is on the checkin for bug 230840 causing this.....

Comment 5

[Mats Palmgren (inactive)]

Attachment: stack when assertion occurs

Comment 6

[Michael Lefevre]

happens in Windows also. OS->All.

Comment 7

[Johnny Stenback (:jst)]

I think I have a fix for this, testing...

Comment 8

[Johnny Stenback (:jst)]

Attachment: Make sure the iterator knows it's done when next gets to the end.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 142970 [details] [diff] [review]
Make sure the iterator knows it's done when next gets to the end.

Hmm... Why is this needed?  Shouldn't we bail out of the next call because
mCurNode is false?  Why did this use to work?

If this is needed, don't Prev() and nsContentIterator::Next and so forth need
similar changes?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 142970 [details] [diff] [review]
Make sure the iterator knows it's done when next gets to the end.

review- in an attemt to get some answers....

Comment 11

[Johnny Stenback (:jst)]

Attachment: Make nsContentSubtreeIterator work with broken selection code.

This makes the nsContentSubtreeIterator work with our broken selection code
again. While this hang was introduced by bug 230840, the selection code was
broken long before then, it just so happened that the iterators "worked" with
it by throwing errors due to the unexpected state, w/o alerting anyone about
it, of course.

This makes the code "work" again, and it also #if 0's out some code that's not
used (but I left it in to make it easier to optimize in the future), and it
also reverts an unintended unrelated change that went in with bug 230840.

Comment 12

[Johnny Stenback (:jst)]

So AFAIK this is only needed for nsContentSubtreeIterator since that's used when
dealing with selection code (and only then), so no need to clutter
nsContentIterator with this, IMO.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143436 [details] [diff] [review]
Make nsContentSubtreeIterator work with broken selection code.

>Index: content/base/src/nsContentIterator.cpp
>+  NS_ASSERTION(nextNode, "No next siblin!?! This could mean deadlock!");

"sibling"

>+  // This shouldn't be needed, but since our selection code can put is

"put us"

>+  // in a situation where mLast is in genrated content, we need this

"generated"

>+  // This shouldn't be needed, but since our selection code can put is

"put us"

>+  // in a situation where mFirst is in genrated content, we need this

"generated"

Please file a followup bug on selection to be fixed or something?

Comment 14

[Johnny Stenback (:jst)]

Fixed. Filed bug 236957 as followup.