Closed Bug 124209 Opened 23 years ago Closed 23 years ago

Deleting selection in plain text reply compose causes hang

Categories

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

Product:
SeaMonkey
Component:
MailNews: Message Display
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.9

People

(Reporter: endico, Assigned: kinmoz)

References

Details

(Keywords: hang)

Attachments

(3 files)

Patch Rev 1 (diff -uw)
919 bytes, patch
Details | Diff | Splinter Review
Patch Rev 1.1 (diff -uw Changed point offset to zero and modified comment.)
954 bytes, patch
glazou
: review+
Details | Diff | Splinter Review
Patch Rev 2 (diff -uw Handle zero-length text nodes in start case too)
1.54 KB, patch
glazou
: review+
shaver
: superreview+
Details | Diff | Splinter Review 
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.
Keywords: hang
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?
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.


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

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.
similar on Windows, but causes a crash. Bug 124222, bug 124239.
Can look like bug 116022 surfaced again?
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()
Assignee: sspitzer → kin
Severity: normal → critical
Priority: -- → P1
Target Milestone: --- → mozilla0.9.9
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.
Status: NEW → ASSIGNED
Here's the diff -uw patch I used to get rid of the infinite loop.
*** Bug 124222 has been marked as a duplicate of this bug. ***
Whiteboard: FIX IN HAND, need r= and sr=
*** Bug 124202 has been marked as a duplicate of this bug. ***
built with the patch: i don't get hangs anymore. (linux)
*** Bug 124184 has been marked as a duplicate of this bug. ***
I'm happy to sr if you can get an r=.
*** Bug 124284 has been marked as a duplicate of this bug. ***
Marking nsbeta1+
Keywords: nsbeta1+
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 on attachment 68452 [details] [diff] [review]
Patch Rev 1.1 (diff -uw Changed point offset to zero and modified comment.)

r/sr=sfraser
*** Bug 124300 has been marked as a duplicate of this bug. ***
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.
QA Contact: esther → sujay
Comment on attachment 68452 [details] [diff] [review]
Patch Rev 1.1 (diff -uw Changed point offset to zero and modified comment.)

r=glazman
Attachment #68452 - Flags: review+
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 on attachment 68539 [details] [diff] [review]
Patch Rev 2 (diff -uw Handle zero-length text nodes in start case too)

r=glazman
Attachment #68539 - Flags: review+
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.)
Attachment #68539 - Flags: superreview+
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.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
OS: Linux → All
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: FIX IN HAND, need r= and sr=
*** Bug 124491 has been marked as a duplicate of this bug. ***
*** Bug 124489 has been marked as a duplicate of this bug. ***
*** Bug 124656 has been marked as a duplicate of this bug. ***
*** Bug 124597 has been marked as a duplicate of this bug. ***
*** Bug 124672 has been marked as a duplicate of this bug. ***
*** Bug 124867 has been marked as a duplicate of this bug. ***
this is working for me now on linux. thanks!
verified.
Status: RESOLVED → VERIFIED
*** Bug 124826 has been marked as a duplicate of this bug. ***
*** Bug 124872 has been marked as a duplicate of this bug. ***
*** Bug 124853 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: