Deleting selection in plain text reply compose causes hang

VERIFIED FIXED in mozilla0.9.9

Status

P1
critical
VERIFIED FIXED
17 years ago
14 years ago

People

(Reporter: endico, Assigned: kinmoz)

Tracking

({hang})

Trunk
mozilla0.9.9

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Updated

17 years ago
Keywords: hang
(Assignee)

Comment 1

17 years ago
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?
(Reporter)

Comment 2

17 years ago
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

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

Comment 4

17 years ago
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

17 years ago
similar on Windows, but causes a crash. Bug 124222, bug 124239.
Can look like bug 116022 surfaced again?
(Assignee)

Comment 6

17 years ago
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
(Assignee)

Comment 7

17 years ago
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
(Assignee)

Comment 8

17 years ago
Created attachment 68404 [details] [diff] [review]
Patch Rev 1 (diff -uw)

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

Comment 9

17 years ago
*** Bug 124222 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

17 years ago
Whiteboard: FIX IN HAND, need r= and sr=

Comment 10

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

Comment 11

17 years ago
built with the patch: i don't get hangs anymore. (linux)
*** Bug 124184 has been marked as a duplicate of this bug. ***

Comment 13

17 years ago
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+
(Assignee)

Comment 16

17 years ago
Created attachment 68452 [details] [diff] [review]
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

17 years ago
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. ***

Comment 19

17 years ago
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+
(Assignee)

Comment 21

17 years ago
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.
(Assignee)

Comment 22

17 years ago
Created attachment 68539 [details] [diff] [review]
Patch Rev 2 (diff -uw Handle zero-length text nodes in start case too)
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+
(Assignee)

Comment 25

17 years ago
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
Last Resolved: 17 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. ***

Comment 27

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

Comment 28

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

Comment 29

17 years ago
*** Bug 124597 has been marked as a duplicate of this bug. ***
*** Bug 124672 has been marked as a duplicate of this bug. ***

Comment 31

17 years ago
*** Bug 124867 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 32

17 years ago
this is working for me now on linux. thanks!

Comment 33

17 years ago
verified.
Status: RESOLVED → VERIFIED

Comment 34

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

Comment 35

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

Comment 36

17 years ago
*** 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.