136165 - Composer crashes after deleting styled text

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Németh István]

Start Composer
Switch to Show-all-tags
Enter some text like "123 456 789"
Selected some words between, like "456"
Changed its color to red
Try to delete it. A font symbol will remain there.
Deleting the font symbol the mozilla crashes.
Tried twice, crashed twice.

Comment 1

[Matthias Versen [:Matti]]

build ID ?
Wo you have a tlkabck ID for that crash ?

Comment 2

[Németh István]

Just the plain 0.9.9 (Gecko/20020311) If you mean this.

Comment 3

[Matthias Versen [:Matti]]

wfm with win2k build 20020407.

Is it possible that you try it again with a nightly build 
(install it in a different directory) ?
or a talkback ID from a build with talkback

Comment 4

[R.K.Aa.]

WFM. 2002040803 XP

Comment 5

[Németh István]

I have no firewall, so I can't download the nightly.
The exact steps are:
start Composer, switch to Show All Tags
[1],[2],[3],[left],[shift+left],[ctrl+i],[left],[shift+right],[delete]
the [i] symbol remained there, I can't go right pressing the [right]
after [end],[left],[backspace] the Mozilla crashes.
I am using W2KSP2

Comment 6

[Adam Hauner]

Nemeth, night builda are available via HTTP and FTP:
http://ftp.mozilla.org/pub/mozilla/nightly/latest/
ftp://ftp.mozilla.org/pub/mozilla/nightly/latest/

Comment 7

[Németh István]

I downloaded it and tried it. The Mozilla crashes. My version was:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.9+) Gecko/20020408

>>or a talkback ID from a build with talkback

Where can I get this ID from?

István (Németh is my family name, in Hungary we are using it backwards)

Comment 8

[Kathleen :Brade]

Joe--does your patch fix this bug?

Comment 9

[Chris Weber]

While using build 2002041617 on a Win98 machine, I could not confirm this bug. 
However the font tag was left over after deleting the text in Show all tags
mode.  I'm unsure if this is not a feature, since the remnant font tag could be
used to be applied to arbitrary text instead of the intended text.

Comment 10

[kinmoz]

--> jfrancis

Comment 11

[Joe Francis]

show all tags mode is not needed to reproduce this.  confirming bug.

Comment 12

[Joe Francis]

*** Bug 143331 has been marked as a duplicate of this bug. ***

Comment 13

[Joe Francis]

*** Bug 145608 has been marked as a duplicate of this bug. ***

Comment 14

[Joe Francis]

Attachment: patch to editor/libeditor/html/nsWSRunObject.cpp

This prevents the crash.  It does not address the underlying problem, but
rather just prevents it from causing a crash.  I'm posting this patch because I
think we should use it for the moz1.0 branch.  It is a very safe patch.  Fixing
the "real" bug will be more risky.

Comment 15

[Joe Francis]

nominating with keyword soup.  at least three different people have hit this
crash, so we need to fix it.

Comment 16

[Joe Francis]

Comment on attachment 84532 [details] [diff] [review]
patch to editor/libeditor/html/nsWSRunObject.cpp

r=glazman

Comment 17

[Kevin McCluskey (gone)]

nsbeta1+

Comment 18

[Kevin McCluskey (gone)]

per jfrancis: This is the 23rd most common crash in the talkback for rc2

Comment 19

[Joe Francis]

still need sr.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 84532 [details] [diff] [review]
patch to editor/libeditor/html/nsWSRunObject.cpp

>       for (pos=mOffset-1; pos>=0; pos--)
>       {
>+        PRInt32 fragLen = textFrag->GetLength();
>+        if ((pos<0) || (pos>=fragLen))

Could pos really be < 0? I don't see how, if you're worried about
it anyway but don't think it should be right that's a perfect place
for NS_ASSERTION(pos >= 0, "Illegal value for pos!");

I also don't see the reason for the temp variable, it's not adding
a whole lot of clarity.

	 if ( pos >= textFrag->GetLength() )

ought to do fine.

>+        {
>+          NS_NOTREACHED("looking beyond end of text fragment");
>+          continue;

Is NS_NOTREACHED appropriate (as opposed to NS_ERROR)? The fact that
we're crashing seems to indicate something happens regularly that the
code wasn't expecting. Is this a real fix or just preventing the crash?
What's the real problem? If we're putting off the real fix (because we
don't know why it happens) this bug should stay open and get changed to
something like "Composer asserts after deleting <etc>"

Now that I see what the continue is doing (decrementing pos) I'm even
less happy you can get into this block if pos < 0, although at the moment
the top for loop should catch it (until someone changes it).

>     textNode->GetTextLength(&len);
>     if (mOffset<len)
>     {
>       PRInt32 pos;
>       for (pos=mOffset; pos<len; pos++)
>       {
>+        PRInt32 fragLen = textFrag->GetLength();
>+        if ((pos<0) || (pos>=fragLen))
>+        {
>+          NS_NOTREACHED("looking beyond end of text fragment");
>+          continue;
>+        }

What's the point of the continue, anyway? You're already past the
length of your fragment so you'll just spew out warnings for each
charater until the for loop stops -- why not just break; instead?

It's not just these two loops, the same comments apply to all the loops.

>@@ -1888,15 +1912,13 @@
>   
>-  PRInt32 len;
>-  res = aTextNode->GetTextLength(&len);
>-  NS_ENSURE_SUCCESS(res, 0);
>+  PRInt32 len = textFrag->GetLength();

What's the difference between GetTextLength and GetLength?

>   if (!len) 
>     return 0;

len is a physical length, right? (len != 0) would seem clearer for this case

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 84532 [details] [diff] [review]
patch to editor/libeditor/html/nsWSRunObject.cpp

sr=jst

Comment 22

[Joe Francis]

to respond to deveditz:

0) you are right about pos being non-negative.  i was being anal.  
1) you are right about the temp being unneeded - i'll get rid of it
2) you are right that this will not close the real bug.  my earlier comment #14 
already indicated that.  I want the safest possible fix to maximize branch 
landing odds.  This crash is being hit a lot and I don't want moz1.0 or future 
ns's to ship with it.
3) the point of the continue is that it is probably correct to do the processing 
in this loop for the characters that are not beyond the end of the range.  break 
would skip that.
4) I used GetLength to make sure I was talking to the actual fragment.  It was 
just cleanup+paranoia.  why get the length from a different interface than I was 
getting the data from?

Comment 23

[Joe Francis]

Attachment: patch to nsWSRunObject.cpp

This addresses dveditz's comments.  I have confirmed this still fixes bug.

Comment 24

[Joe Francis]

seeking driver approval

Comment 25

[Joe Francis]

adding adt1.0.0, topcrash

Comment 26

[Joe Francis]

*** Bug 126026 has been marked as a duplicate of this bug. ***

Comment 27

[scottputterman]

adding adt1.0.0+ for checkin to 1.0 branch, pending reviews of the new patch and
drivers approval.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 84689 [details] [diff] [review]
patch to nsWSRunObject.cpp

thanks, sr=dveditz

The continue question was more about the two loops which increment -- you will
never come back into range. No big deal, maybe the assertion spew will motivate
someone to fix the real problem later

Comment 29

[sujay]

changing qa_contact.

Comment 30

[Charles Manske]

Comment on attachment 84689 [details] [diff] [review]
patch to nsWSRunObject.cpp

r=cmanske

Comment 31

[Joe Francis]

fix landed on trunk.  still awaiting drivers for branch landing.

Comment 32

[sujay]

verified on 5/24 trunk build.

Comment 33

[Dawn Endico]

Comment on attachment 84689 [details] [diff] [review]
patch to nsWSRunObject.cpp

a=shaver,scc,tor

please check this in today

Comment 34

[Joe Francis]

Sujay:
fix landed on branch - please verify branch

Comment 35

[sujay]

verified in 5/28 branch build.