75305 - Doing Undo and Redo crashes browser

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[shrirang khanzode]

seen on windows 0409...very unstable today..

Steps:

Launch composer
Open the attachment 
Highlight (left to right) with mouse..the contents of a row and press CTRL + X
Observe that the row becomes empty
Now, do Edit|Undo and immediately after that,do Edit|Redo
BOOM ! Crashes...

Comment 1

[shrirang khanzode]

Attachment: testfile

Comment 2

[shrirang khanzode]

windoze(NT) only...linux and mac are fine

Comment 3

[shrirang khanzode]

cc: kin

Comment 4

[shrirang khanzode]

happens in my 04/05 build too

Comment 5

[kinmoz]

Attachment: Stack trace for crash.

Comment 6

[kinmoz]

I just attatched a stack trace that shows that it's crashing in layout while 
painting a text frame (nsTextFrame::PaintAsciiText()). Somehow 
the content node's text fragment and it's frame's mContentLength are out of 
sync. The text fragment contains no text (null pointer) and mContentLength is 
greater than zero so they are indexing into a null pointer. Here's the code 
snippet that's causing the crash:

    // See if the text ends in a newline
    if ((textLength > 0) && (text[textLength - 1] == '\n')) {

Comment 7

[kinmoz]

I'll debug this one.

Comment 8

[kinmoz]

Attachment: Smaller Testcase

Comment 9

[kinmoz]

Attachment: Some helpful stack traces.

Comment 10

[kinmoz]

I just attatched a file containing some stack traces that may be helpful in 
understanding why we crash.

The root of the crash is what I believe to be a race condition between the 
processing of Reflow and Paint events generated by layout methods.

The only real difference between the original "Cut" operation and the "Redo" of 
the cut operation is the fact that we call nsEditor::BeginUpdateViewBatch() and 
nsEditor::EndUpdateViewBatch() during the original "Cut".

Begin/EndUpdateViewBatch() are editor convenience routines that turn Selection, 
ViewManager, and Reflow batching on/off. The idea is that we want to prevent the 
user from seeing all the intermediate steps/edits neccessary to accomplish what 
they asked us to do.

It's also important to note that closing ViewManager update and Layout Reflow 
batches, force synchronous processing of pending events, so the editor uses this 
fact to ensure that reflows always happen before any view updates.

In the "Redo" case, we don't call BeginUpdateViewBatch(), so an async reflow 
request is posted as the result of our inserting/removing of nodes from the DOM 
tree, also, the manipulation of selection during these operations and the 
removal of text nodes from the DOM tree trigger frame Invalidations that send 
async Paint requests.

When we return to the event loop we will now either process the reflow request 
(PLEvent) or process the pending PAINT event sitting in the native event queue. 
If the PAINT event is processed first (as is for this bug) we crash because the 
frame tree is in a state that is out of sync with the content tree, because a 
reflow did not happen to sync things up.

Having said all this, it's pretty easy to fix this particular bug by wrapping 
both Undo() and Redo() calls in the editor with Begin/EndUpdateViewBatch(). My 
concern is that this bug could probably be reproduced by mimic'ing what the 
editor does with DOM calls from JS ... so I think it is still worth the time 
from some people in layout to look into this race condition.

Comment 11

[kinmoz]

Attachment: Fix for crash in editor.

Comment 12

[Joe Francis]

this patch looks good.  it smells good.  it trastes good.
mmmmmmmmmm, patch.
r=jfrancis

Comment 13

[Daniel Bratell]

The same symptoms are in bug 73291 (resolved WORKSFORME) and bug 66856 (marked 
dupe of 73291). Since they are not reproducible, it's difficult to say if it's 
the same, but maybe you can take a look and see if it's the same crash caused by 
a similar mistake somewhere else?

Comment 14

[kinmoz]

I'll bet money that both those bugs Daniel mentioned are due to the same race 
condition. Perhaps we should reopen those bugs and point them to the steps in 
this bug since it's quite easy to reproduce.

Comment 15

[kinmoz]

I just attatched an HTML file to bug 73291 that uses JS and DOM calls to 
recreate the crash we see here.

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=31505

Comment 16

[Simon Fraser [no longer active]]

So this is our Bandaid for the crash, right? Do we have open bugs to get the 
layout guys to fix this the real way?

nsAutoUpdateViewBatch: it might be better to assert that you have an editor in 
the ctor, rather than the 2 if checks, since, if you have one in the ctor, mEd 
will always be non-null in the dtor (tho it could be a bad pointer).

Comment 17

[kinmoz]

Bug 73291 covers the real problem. It also has a JS test case that I whipped up 
to demonstrate that this isn't an editor specific problem.

In regards to your ASSERTION comment, I can add one, but I still need to leave 
the 2 'if' statements in to ensure we don't try to call through a bad pointer. 
:-)

Comment 18

[Simon Fraser [no longer active]]

kin: i think having a non-null nsEditor needs to be guaranteed by the caller 
(who, it seems, often pass 'this'). This is not an interface class.

Comment 19

[kinmoz]

Fix checked into trunk:

    mozilla/editor/base/nsPlaintextEditor.cpp  revision 1.10
    mozilla/editor/base/nsEditorUtils.h        revision 1.17

Should appear in QA builds after 8am today.

Comment 20

[sujay]

Shrir, is this fixed for you now?

Comment 21

[shrirang khanzode]

Oh yeah, this is fixed on 0529 trunk. VERIFIED