124209 - Deleting selection in plain text reply compose causes hang

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P1)

Description

[Dawn Endico]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.8+) Gecko/20020207

Mozilla hangs for me when deleting selections of quoted text in replies
when using the the delete key. 

Steps to reproduce
1. Set pref to "compose messages in html format"
2. Set pref to "automatically quote when replying"
3. Reply to an email message.
4. Select some of the replied text in the compose window.
5. Press delete key.

Pasting in a bunch of text and then deleting parts of it
parts of it works fine. Deleting selected text by hitting
the enter key or typing text does not hang.

Comment 1

[kinmoz]

Hmmm, I'm not seeing this in my Win32 debug build from this morning.

The summary says plaintext mail compose, yet the steps say that we have to set
the pref to compose in html format. In either case, I'm not seeing the problem.

Is the message you are replying to plaintext? Or does it have HTML in it?

Any stack trace?

Comment 2

[Dawn Endico]

arg. i'm an eediot.  I meant that  "compose messages in html format" 
should be unset. This happens when replying to both plain text and
multipart text/html. I don't think i have any text/html that issn't
multipart.  Not sure how to get a stack trace. i'm using a nightly
build. 

Steps to reproduce
1. Unset pref to "compose messages in html format" (You want to compose
   in plain text)
2. Set pref to "automatically quote when replying"
3. Reply to an email message.
4. Select some of the replied text in the compose window.
5. Press delete key.

Comment 3

[Joe Francis]

Dawn, what are your settings for:
1) mailquote above or below your reply?
2) signature (yes/no)?

Comment 4

[Dawn Endico]

i just mailed my prefs file to jfrancis and kin. not sure what the 
mailquote pref is but i guess i'm using the default. I don't use a 
signature.

Comment 5

[R.K.Aa.]

similar on Windows, but causes a crash. Bug 124222, bug 124239.
Can look like bug 116022 surfaced again?

Comment 6

[kinmoz]

Ok, endico came to my cube and we successfully reproduced it. In a plaintext
mail compose, you have to select from the very beginning of one line and end the
selection past the right side of the line you stop on, then press delete. You
should then see a hang.

It looks like we get stuck in copy_string() in the string code. Here's the stack
trace I'm seeing:


OLE32! 77baeb4c()
copy_string(nsReadingIterator<unsigned short> & {...}, const
nsReadingIterator<unsigned short> & {...}, nsWritingIterator<unsigned short> &
{...}) line 92 + 13 bytes
nsAString::do_AppendFromReadable(const nsAString & {...}) line 362 + 55 bytes
nsAString::do_AppendFromElementPtr(const unsigned short * 0x0126fa7c) line 368 +
24 bytes
nsAString::Append(const unsigned short * 0x0126fa7c) line 284 + 28 bytes
nsNodeInfo::GetQualifiedName(const nsNodeInfo * const 0x08170970, nsAString &
{...}) line 115
nsGenericHTMLElement::GetNodeName(nsGenericHTMLElement * const 0x08170060,
nsAString & {...}) line 385 + 22 bytes
nsHTMLSpanElement::GetNodeName(nsHTMLSpanElement * const 0x08170060, nsAString &
{...}) line 59 + 16 bytes
nsEditor::IsContainer(nsIDOMNode * 0x08170088) line 3560 + 19 bytes
nsWSRunObject::GetNextWSNode(nsIDOMNode * 0x08170088, short 10, nsIDOMNode *
0x0810ead8, nsCOMPtr<nsIDOMNode> * 0x0012edac) line 1272 + 15 bytes
nsWSRunObject::GetNextWSNode(nsWSRunObject::DOMPoint {...}, nsIDOMNode *
0x0810ead8, nsCOMPtr<nsIDOMNode> * 0x0012edac) line 1256 + 30 bytes
nsWSRunObject::GetWSNodes() line 840 + 47 bytes
nsWSRunObject::nsWSRunObject(nsHTMLEditor * 0x08125e60, nsIDOMNode * 0x08170088,
int 10) line 96
nsHTMLEditRules::AdjustWhitespace(nsISelection * 0x0810b910) line 6377 + 28 bytes
nsHTMLEditRules::AfterEditInner(int 8, short 2) line 423 + 17 bytes
nsHTMLEditRules::AfterEdit(nsHTMLEditRules * const 0x081269e4, int 8, short 2)
line 333 + 20 bytes
nsHTMLEditor::EndOperation(nsHTMLEditor * const 0x08125e60) line 3858 + 62 bytes
nsAutoRules::~nsAutoRules() line 125
nsPlaintextEditor::DeleteSelection(nsPlaintextEditor * const 0x08125e60, short
2) line 949 + 22 bytes
nsHTMLEditorLog::DeleteSelection(nsHTMLEditorLog * const 0x08125e60, short 2)
line 158 + 14 bytes
nsTextEditorKeyListener::KeyPress(nsTextEditorKeyListener * const 0x08127af0,
nsIDOMEvent * 0x08175268) line 241
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x0810ca30,
nsIPresContext * 0x08108500, nsEvent * 0x0012f9a0, nsIDOMEvent * * 0x0012f6bc,
nsIDOMEventTarget * 0x081051e4, unsigned int 2, nsEventStatus * 0x0012f910) line
1636 + 41 bytes
nsDocument::HandleDOMEvent(nsDocument * const 0x081051b0, nsIPresContext *
0x08108500, nsEvent * 0x0012f9a0, nsIDOMEvent * * 0x0012f6bc, unsigned int 2,
nsEventStatus * 0x0012f910) line 3239
nsGenericElement::HandleDOMEvent(nsGenericElement * const 0x081071a0,
nsIPresContext * 0x08108500, nsEvent * 0x0012f9a0, nsIDOMEvent * * 0x0012f6bc,
unsigned int 1, nsEventStatus * 0x0012f910) line 1680 + 39 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f9a0, nsIView * 0x08123d90,
unsigned int 1, nsEventStatus * 0x0012f910) line 6000 + 44 bytes
PresShell::HandleEvent(PresShell * const 0x0810bbc4, nsIView * 0x08123d90,
nsGUIEvent * 0x0012f9a0, nsEventStatus * 0x0012f910, int 0, int & 1) line 5923 +
25 bytes
nsView::HandleEvent(nsView * const 0x08123d90, nsGUIEvent * 0x0012f9a0, unsigned
int 0, nsEventStatus * 0x0012f910, int 0, int & 1) line 390
nsView::HandleEvent(nsView * const 0x081220c0, nsGUIEvent * 0x0012f9a0, unsigned
int 0, nsEventStatus * 0x0012f910, int 0, int & 1) line 347
nsView::HandleEvent(nsView * const 0x08109c10, nsGUIEvent * 0x0012f9a0, unsigned
int 0, nsEventStatus * 0x0012f910, int 1, int & 1) line 347
nsViewManager::DispatchEvent(nsViewManager * const 0x08109cb0, nsGUIEvent *
0x0012f9a0, nsEventStatus * 0x0012f910) line 1900
HandleEvent(nsGUIEvent * 0x0012f9a0) line 83
nsWindow::DispatchEvent(nsWindow * const 0x08123ef4, nsGUIEvent * 0x0012f9a0,
nsEventStatus & nsEventStatus_eIgnore) line 850 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f9a0) line 871
nsWindow::DispatchKeyEvent(unsigned int 131, unsigned short 0, unsigned int 8,
long 0) line 2598 + 15 bytes
nsWindow::OnChar(unsigned int 8, unsigned int 8, unsigned char 1) line 2731
nsWindow::ProcessMessage(unsigned int 258, unsigned int 8, long 917505, long *
0x0012fda8) line 3280 + 51 bytes
nsWindow::WindowProc(HWND__ * 0x0ccc0866, unsigned int 258, unsigned int 8, long
917505) line 1115 + 27 bytes
USER32! 77e71820()

Comment 7

[kinmoz]

I think I know what's going on. If we run across a zero-length text node in
nsWSRunObject::GetWSNodes() when looping over the end point,
nsWSRunObject::GetNextWSNode() will keep retruning the same zero-length node
because we never fall into the loop that will set the end point past it.

I just modified my tree to handle this case. I'll let you know if it fixes the
problem.

Comment 8

[kinmoz]

Attachment: Patch Rev 1 (diff -uw)

Here's the diff -uw patch I used to get rid of the infinite loop.

Comment 9

[kinmoz]

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

Comment 10

[R.K.Aa.]

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

Comment 11

[R.K.Aa.]

built with the patch: i don't get hangs anymore. (linux)

Comment 12

[Jean-Francois Ducarroz]

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

Comment 13

[David :Bienvenu]

I'm happy to sr if you can get an r=.

Comment 14

[Jean-Francois Ducarroz]

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

Comment 15

[Kevin McCluskey (gone)]

Marking nsbeta1+

Comment 16

[kinmoz]

Attachment: Patch Rev 1.1 (diff -uw Changed point offset to zero and modified comment.)

I'll try to get jfrancis to look at this and r= so I can check it in tomorrow
morning before the build verifications kick-off.

This 2nd patch just changes the offset I used from 1 to 0.

Comment 17

[Simon Fraser [no longer active]]

Comment on attachment 68452 [details] [diff] [review]
Patch Rev 1.1 (diff -uw Changed point offset to zero and modified comment.)

r/sr=sfraser

Comment 18

[Jean-Francois Ducarroz]

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

Comment 19

[Sheela Ravindran]

Including comments from lisa chiang:  When I used today's 2-7 Win32
build, the system hung right after I pressed enter twice to start a new 
paragraph at the * point above in html compose.   If anyone has experienced 
this, I believe this really should be a blocker since it renders the compose 
window useless to send mail if the hangs happen frequent enough.

Reassigning qa contact since I also see this problem in composer too.  I was 
trying to edit my status report and selecting a block of text and hit delete key 
caused the whole application to hang.  

Comment 20

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 68452 [details] [diff] [review]
Patch Rev 1.1 (diff -uw Changed point offset to zero and modified comment.)

r=glazman

Comment 21

[kinmoz]

Ok, I was afraid the start point in the same GetWSNodes() had the same problem, 
but I couldn't prove it until glazman brought bug 124375 to my attention. It has 
a test case that can reproduce the same hang with the start point search.

New diff coming up which is the previous diff plus the same change for the start 
point.

Comment 22

[kinmoz]

Attachment: Patch Rev 2 (diff -uw Handle zero-length text nodes in start case too)

Comment 23

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 68539 [details] [diff] [review]
Patch Rev 2 (diff -uw Handle zero-length text nodes in start case too)

r=glazman

Comment 24

[Mike Shaver (:shaver emeritus)]

Comment on attachment 68539 [details] [diff] [review]
Patch Rev 2 (diff -uw Handle zero-length text nodes in start case too)

sr=shaver on the blocker fix, with some indication that the |< 1| check is also
meant to catch presumably-corrupt text nodes and remove them as well.

(Looks like a helper method might be useful here?  Looks like a fair bit of
pretty similar code in the two patched areas.)

Comment 25

[kinmoz]

Fix checked into TRUNK:

    mozilla/editor/libeditor/html/nsWSRunObject.cpp  revision 1.15

To answer shaver's question, len should really never be negative, but the dom 
method takes a signed int so I tried to err on the side of paranoia. If is ever 
negative, I suppose we'd want to remove it too.

Comment 26

[Jean-Francois Ducarroz]

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

Comment 27

[R.K.Aa.]

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

Comment 28

[R.K.Aa.]

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

Comment 29

[R.K.Aa.]

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

Comment 30

[Tuukka Tolvanen (sp3000)]

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

Comment 31

[laurel]

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

Comment 32

[Dawn Endico]

this is working for me now on linux. thanks!

Comment 33

[sujay]

verified.

Comment 34

[laurel]

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

Comment 35

[laurel]

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

Comment 36

[laurel]

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