123033 - mozilla crashes after selecting text input field and pressing a key

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect)

Description

[Christoph Studer]

Go to the URL. On the top of the page you see an input field with the value "test". 
1 . Now point with the cursor at the upper border of the text field between "te"
and "st".
2. triple click to select the underlying paragraph. 
3. if mozilla only selects the
text field's content, move the cursor pixel by pixel vertically&horizontally an
goto 2
until the text before and the space after the text field is selected.
3. type any key

Result: mozilla crashes
Expected: nothing. maybe a "bing"-sound to indicate that the nothing happens
though the user pressed a key.

note that this does not happen by selecting the text as you would normally by
clicking and dragging over it.

I actually encountered this on in a slightly different kind:
1. go to https://wwws.nic.ch/reg/domain/searchdomain.cfm
2. enter "test" into the text field.
3. select the text in the same way as described above.
4. repeat 3 until the content plus the surrounding text is selected.
5. type key

Result: mouzilla replaces field's content and deletes surrounding text
Expected: nothing. maybe a "bing"-sound to indicate that the nothing happens
though the user pressed a key.

Sorry for choosing "Browser-General". Please tell me if I sould split this bug
into two.

Comment 1

[Matthias Versen [:Matti]]

Reporter:
Please always add the build ID in a bug report.
for the crash:
Plase use a talkback enabled build. After you crashed, talkback should pop up.
After talkback submitted the crash to the server run
mozilla/components/talkback.exe and add the Talkback ID# from your crash in this
bug.

Thanks

Comment 2

[Christoph Studer]

sorry for the lack of this information.

Build ID:    2002020103
Incident ID: TB2425922Y

Comment 3

[[e-mail] (gone - use [e-mail] instead)]

0x00000000
nsTypedSelection::EndBatchChanges
[d:\builds\seamonkey\mozilla\content\base\src\nsSelection.cpp, line 7520]
nsEditor::Do [d:\builds\seamonkey\mozilla\editor\libeditor\base\nsEditor.cpp,
line 502] 

Comment 4

[Matthias Versen [:Matti]]

-> selection

Comment 5

[mjudge]

i can reproduce this. i am working on it. I think this is EDITORBASE.

Comment 6

[mjudge]

ok i fixed it.  the problem was that the tripple click actually.  The code was 
written such that it didnt go through nsISelection. Instead it went directly to 
the nsITypedSelection and called collapse and Extend.  collapse and extend 
never before checked the "limiter" of their container nsISelection.  AND since 
the text fields are technically in the same document it was perfectly legal in 
a dom sense to set the selection to before or extend to after the text field.  
Thus when a key was typed it tried to delete and reflow content outside the 
text field and booom!

I fixed this by calling up to the containing frameselection from the 
nsTypedSelection (if a frame selection exists)  and I ask for the Limiter.  The 
limiter is a div that surrounds the anonymous content of the text field/area.  
This method was allready in place.   IF such a limiter exists we check it 
against the passed in content and offsets.  If the passed in content nor the 
parent of that content(aka a text node check the parent) is the limiter then we 
have a problem and some frame is trying to set its nsTypedSelection to a bogus 
location and we simply fail out as if no extend nor a collapse was called.  

Patch to be attached.

Comment 7

[mjudge]

Attachment: fix to check the limiter of the parent nsSeleciton

fix for crasher

Comment 8

[mjudge]

talked to syd he has said this is +'d. looking for r= and sr=

Comment 9

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 10

[kinmoz]

Comment on attachment 71999 [details] [diff] [review]
fix to check the limiter of the parent nsSeleciton

r/sr=[e-mail] (whichever you need) with the following changes:


-- Since the 2 pieces of code you are adding are identical,
   perhaps a utility method would be good. I'd also add
   comments to that method about why you can make the
   assumptions you make about tLimiter, and the fact that
   you only have to match the container's parent and grandparent
   against it.



+  nsCOMPtr<nsIContent> tLimiter;
+  if (mFrameSelection)
+  {


-- Since tLimiter isn't used outside of the |if| block, I'd
   move it's declaration into it.


+    result = mFrameSelection->GetLimiter(getter_AddRefs(tLimiter));
+    if (NS_FAILED(result))
+      return result;


-- Does GetLimiter() guarantee a non-null tLimiter if result is not
   a FAILED code? if not perhaps we should check tLimiter for null.

Comment 11

[mjudge]

tlimiter WAS being checked for null.  It is perfectly valid not to have a 
limiter in the case of topmost webbrowser or composer. 

I have factored out the method and had the original check for the limiter also 
call the factored out function.  I put in a big comment on what the heck this 
is all used for as well. new patch

Comment 12

[mjudge]

Attachment: patch for nsSelection.cpp in content/base/src.

factored out function, better comments

Comment 13

[kinmoz]

Comment on attachment 73905 [details] [diff] [review]
patch for nsSelection.cpp in content/base/src.

sr=[e-mail]

Perhaps we should also mention (with an XXX comment?) that should the need ever
arise for a limiter on a non-text-widget, that IsValidSelectionPoint() would
have to be modified to traverse the container's parent hierarchy with a loop
looking for tLimiter.
Or if you're feeling frisky you could just add that loop. :-)

Comment 14

[Garrett Arch Blythe]

Comment on attachment 73905 [details] [diff] [review]
patch for nsSelection.cpp in content/base/src.

r=blythe

Comment 15

[mjudge]

ok I put in a comment about the while loop. I dont want to put it in because i 
think its unecessary. r=blythe sr=kin i need approval too.

I have had Sujay Desai pound on the build with this change on it for about 45 
min.  He reported no obvious problems if this helps get the a=

Comment 16

[mjudge]

i also ran the precheckin tests, sent mail, open files.

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 73905 [details] [diff] [review]
patch for nsSelection.cpp in content/base/src.

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 18

[mjudge]

checked in didnt comment on it

Comment 19

[sairuh (rarely reading bugmail)]

no longer crashes --tested with 2003.02.27.09 comm trunk, win2k.