Bug 6644 – TinyMCE: Undo operation crashes Safari

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Justin Garcia]

Goto: http://tinymce.moxiecode.com/example.php?example=true
In the first editable area, select some text and click the Bold button.
Click the Undo button.

Safari crashes.
As an aside, our own Undo works fine (Command-Z).

Here's the trace:
#0	0x01e69830 in WebCore::maxDeepOffset at htmlediting.cpp:102
#1	0x01f70350 in Frame::selectContentsOfNode at Frame.cpp:2433
#2	0x01f7050c in Frame::selectAll at Frame.cpp:2427
#3	0x01e75b78 in WebCore::(anonymous namespace)::execSelectAll at jsediting.cpp:326
#4	0x01e77158 in WebCore::JSEditor::execCommand at jsediting.cpp:77
#5	0x01f9e010 in WebCore::DocumentImpl::execCommand at DocumentImpl.cpp:2611
#6	0x01d135d4 in KJS::DOMDocumentProtoFunc::callAsFunction at kjs_dom.cpp:1062

[Justin Garcia]

Created attachment 5757 [details]
patch

During the undo operation, TinyMCE does a selectAll (seems weird, but OK). 
Frame::selectAll tries to get the rootEditableElement for the node associated
with the start of the selection.  This patch fixes a bug in
rootEditableElement:

The isEditableBlock check is incorrect (the node is already checked to ensure
isContentEditable, and the node doesn't need to be a block, it can be a span, a
text node, or whatever).

[Justin Garcia]

The patch is kind of unclear without the surrounding code, here's the whole function:

ElementImpl *NodeImpl::rootEditableElement() const
{
    if (!isContentEditable())
        return 0;

    NodeImpl *n = const_cast<NodeImpl *>(this);
    if (n->hasTagName(bodyTag))
        return static_cast<ElementImpl *>(n);

    NodeImpl *result = n;
    while (1) {
        n = n->parentNode();
        if (!n || !n->isContentEditable())
            break;
        if (n->hasTagName(bodyTag)) {
            result = n;
            break;
        }
        if (n->isBlockFlow())
            result = n;
    }
    return static_cast<ElementImpl *>(result);
}

[Justin Garcia]

Please disregard the r?

[Justin Garcia]

Comment on attachment 5757 [details]
patch

This patch is wrong, unsetting the review flag

[Justin Garcia]

Created attachment 5790 [details]
patch

Makes a Frame's SelectionController a ref'able object.	
SelectionController listens for node removal events and nulls out its Selection
if necessary.
Todo: 
More sophisticated adjustment of the selection when its endpoints are removed
Also listen for node insertions
We shouldn't have to go through Frame::setSelection to notify delegates of
selection changes.  This is filed as
<http://bugzilla.opendarwin.org/show_bug.cgi?id=6498> SelectionController's
setters should notify delegates of selection change

[Justin Garcia]

Created attachment 5791 [details]
patch

I made a mistake in Frame::khtmlMouseReleaseEvent and removed some code that
cleared the selection if the mouse hadn't moved since the last mouse press. 
Fixed.

[Justin Garcia]

I don't know why I turned m_mark back into a SelectionController, I'm fixing that.

[Darin Adler]

Comment on attachment 5791 [details]
patch

You shouldn't need to call the EventListener constructor explicitly in the
SelectionController constructor initialization list. Should just work by
default.

The SelectionController::handleEvent method doesn't really need to check the
event type, since you only register for the one event.

SelectionController::handleEvent doesn't seem to arrange for a repaint of the
selection. Doesn't it need to do that?

What causes the selection controller to be removed as an event listener when
the Frame is destroyed?

Where is the code to set up the listener for the mark and drag caret?

I'd prefer to see a function in the selection controller itself to
register/deregister the event handler; this doesn't seem to be a good division
of responsibilities -- the fact that the selection controller itself is an
event listener should be its own private business.

In fact (late to make this suggestion) the listener could just be an object
allocated inside the selection controller with a back pointer to the selection
controller. That would eliminate the need to change all the uses of
SelectionController to use pointers.

None of these comments is really "mandatory", but please consider them.

[Darin Adler]

Comment on attachment 5791 [details]
patch

Marking review- based on my comments above. If you consider all my comments and
decide nothing needs to change, feel free to set this patch to review? again
without changing it.

[Justin Garcia]

Created attachment 5804 [details]
patch

Instead of making Frame's SC a ref'able object, I did what darin suggested and gave SC an object that listens for mutations.

[Justin Garcia]

Created attachment 5891 [details]
patch

A few tweaks.

[Maciej Stachowiak]

+    m_mutationListener = new MutationListener(this);

Why not use a C++-style initializer?

Also, there's a specific optimization for most types of mutation events that prevents dispatching them at all when there are no listeners, as a performance optimization. For instance, see this chunk in ContainerNodeImpl.cpp:

    // dispatch pre-removal mutation events
    if (getDocument()->hasListenerType(DocumentImpl::DOMNODEREMOVED_LISTENER)) {
        child->dispatchEvent(new MutationEventImpl(DOMNodeRemovedEvent, true, false,
            this, DOMString(), DOMString(), DOMString(), 0), ec, true);


 It looks like this patch will cause there to always be a listener for DOMNodeRemovedEvent and so will defeat the attempted optimization. You should verify that this change does not cause performance regressions for page load time (PLT, iBench) or basic DOM operations when no selection is set. If it does, one possibility is to register the listener only when the selection is either a range or caret, rather than None.

Here are some DOM benchmarks for comparisons:

http://www.hixie.ch/tests/adhoc/perf/dom/artificial/core/001.html
http://idanso.dyndns.org/maps/jsbench/

Oh wait, I wrote all that and then I noticed your code already avoids adding a mutation listener when there is no selection. So nevermind, but you may find it useful to do some performance testing anyway just to make sure this is working properly.

Also I see you did not take darin's advice on leaving out the event type check or arranging for a repaint. Is arranging for the repaint definitely unnecessary in all cases? What is the code that does it?

Despite all this, r=me.

[Justin Garcia]

I changed the initializers for m_mutationLister as per maciej's suggestion, and addressed the rest of his comments over IRC.  I'm going to land this + layout tests once I track down 6753.