Bug 53564 – Make canHaveChildrenForEditing more efficient

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

I profiled editing/selection/extend-selection because they take more than 5s to run on the debug build.  The blame is on editingIgnoresContent, which consumes more than 5% of the run time according to Shark.  We should make function more efficient.

	5.5%	5.5%	WebCore	WebCore::Node::getFlag(WebCore::Node::NodeFlags) const
	0.0%	4.1%	WebCore	 WebCore::Node::isElementNode() const
	0.0%	2.7%	WebCore	  WebCore::Node::hasTagName(WebCore::QualifiedName const&) const
	0.0%	2.5%	WebCore	   WebCore::canHaveChildrenForEditing(WebCore::Node const*)
	0.0%	2.5%	WebCore	    WebCore::editingIgnoresContent(WebCore::Node const*)
	0.0%	0.1%	WebCore	   WebCore::endsOfNodeAreVisuallyDistinctPositions(WebCore::Node*)
	0.0%	0.1%	WebCore	   WebCore::Position::downstream(WebCore::EditingBoundaryCrossingRule) const
	0.0%	0.0%	WebCore	   WebCore::Node::rootEditableElement() const
	0.0%	0.0%	WebCore	   WebCore::highestEditableRoot(WebCore::Position const&)
	0.0%	1.1%	WebCore	  WebCore::toElement(WebCore::Node const*)
	0.0%	1.1%	WebCore	   WebCore::Node::hasTagName(WebCore::QualifiedName const&) const
	0.0%	1.0%	WebCore	    WebCore::canHaveChildrenForEditing(WebCore::Node const*)
	0.0%	0.1%	WebCore	    WebCore::Position::downstream(WebCore::EditingBoundaryCrossingRule) const
	0.0%	0.0%	WebCore	    WebCore::Node::rootEditableElement() const
	0.0%	0.0%	WebCore	    WebCore::Position::isCandidate() const
	0.0%	0.0%	WebCore	    WebCore::highestEditableRoot(WebCore::Position const&)
	0.0%	0.0%	WebCore	    WebCore::endsOfNodeAreVisuallyDistinctPositions(WebCore::Node*)
	0.0%	0.1%	WebCore	  WebCore::isTableElement(WebCore::Node*)
	0.0%	0.0%	WebCore	  WebCore::Node::rootEditableElement() const
	0.0%	0.0%	WebCore	  WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
	0.0%	0.6%	WebCore	 WebCore::Node::parentNode() const
	0.0%	0.4%	WebCore	 WebCore::Node::isContainerNode() const
	0.0%	0.3%	WebCore	 WebCore::Node::isTextNode() const
	0.0%	0.0%	WebCore	 WebCore::Node::isShadowRoot() const
	0.0%	0.0%	WebCore	 WebCore::Node::parentNodeGuaranteedHostFree() const
	0.0%	0.0%	WebCore	 WebCore::Node::isSVGElement() const
	0.0%	0.0%	WebCore	 WebCore::Node::rootEditableElement() const
	0.0%	0.0%	WebCore	 WebCore::Node::inDocument() const
	0.0%	0.0%	WebCore	 WebCore::Node::hasTagName(WebCore::QualifiedName const&) const
	0.0%	0.0%	WebCore	 WebCore::isDeletableElement(WebCore::Node const*)
	2.1%	2.1%	WebCore	WebCore::Node::hasTagName(WebCore::QualifiedName const&) const
	1.9%	1.9%	WebCore	WebCore::Node::isElementNode() const
	1.6%	1.6%	WebCore	WTF::RefPtr<WTF::StringImpl>::get() const
	1.6%	1.6%	JavaScriptCore	WTF::isMainThread()
	1.5%	1.5%	WebCore	WebCore::QualifiedName::matches(WebCore::QualifiedName const&) const
	1.3%	1.3%	libSystem.B.dylib	__spin_lock
	1.2%	1.2%	libobjc.A.dylib	objc_msgSend
	1.1%	1.1%	libicucore.A.dylib	icu::RuleBasedBreakIterator::handleNext(icu::RBBIStateTable const*)

[Ryosuke Niwa]

Per Justin's suggestion, I put the check for the text node first.  While this improved the situation a lot, we need to do better:

	3.8%	3.8%	WebCore	WebCore::Node::getFlag(WebCore::Node::NodeFlags) const
	0.0%	2.4%	WebCore	 WebCore::Node::isElementNode() const
	0.0%	1.3%	WebCore	  WebCore::Node::hasTagName(WebCore::QualifiedName const&) const
	0.0%	1.0%	WebCore	  WebCore::toElement(WebCore::Node const*)
	0.0%	0.1%	WebCore	  WebCore::isTableElement(WebCore::Node*)
	0.0%	0.0%	WebCore	  WebCore::Node::rootEditableElement() const
	0.0%	0.0%	WebCore	  WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
	0.0%	0.5%	WebCore	 WebCore::Node::parentNode() const
	0.0%	0.3%	WebCore	 WebCore::Node::isContainerNode() const
	0.0%	0.3%	WebCore	 WebCore::Node::isTextNode() const
	0.0%	0.1%	WebCore	 WebCore::Node::isShadowRoot() const
	0.0%	0.1%	WebCore	 WebCore::Node::parentNodeGuaranteedHostFree() const
	0.0%	0.0%	WebCore	 WebCore::Node::hasRareData() const
	0.0%	0.0%	WebCore	 WebCore::toElement(WebCore::Node const*)
	0.0%	0.0%	WebCore	 WebCore::Range::compareBoundaryPoints(WebCore::Node*, int, WebCore::Node*, int)
	0.0%	0.0%	WebCore	 WebCore::Node::isSVGElement() const
	0.0%	0.0%	WebCore	 WebCore::Node::hasTagName(WebCore::QualifiedName const&) const
	0.0%	0.0%	WebCore	 WebCore::Node::childNode(unsigned int) const
	0.0%	0.0%	WebCore	 WebCore::Node::childNeedsStyleRecalc() const
	0.0%	0.0%	WebCore	 WebCore::Node::appendChild(WTF::PassRefPtr<WebCore::Node>, int&, bool)
	0.0%	0.0%	WebCore	 WebCore::isTableElement(WebCore::Node*)
	0.0%	0.0%	WebCore	 WebCore::Document::updateStyleIfNeeded()
	1.9%	1.9%	WebCore	WebCore::QualifiedName::matches(WebCore::QualifiedName const&) const
	0.0%	1.9%	WebCore	 WebCore::Element::hasTagName(WebCore::QualifiedName const&) const
	0.0%	1.9%	WebCore	  WebCore::Node::hasTagName(WebCore::QualifiedName const&) const
	0.0%	1.8%	WebCore	   WebCore::canHaveChildrenForEditing(WebCore::Node const*)
	0.0%	0.1%	WebCore	   WebCore::highestEditableRoot(WebCore::Position const&)
	0.0%	0.0%	WebCore	   WebCore::Position::downstream(WebCore::EditingBoundaryCrossingRule) const
	0.0%	0.0%	WebCore	   WebCore::Node::rootEditableElement() const
	0.0%	0.0%	WebCore	   WebCore::RenderObject::isBody() const
	0.0%	0.0%	WebCore	   WebCore::Position::isCandidate() const
	0.0%	0.0%	WebCore	 WebCore::Node::hasTagName(WebCore::QualifiedName const&) const
	1.6%	1.6%	WebCore	WebCore::Node::hasTagName(WebCore::QualifiedName const&) const
	0.0%	1.2%	WebCore	 WebCore::canHaveChildrenForEditing(WebCore::Node const*)
	0.0%	0.1%	WebCore	 WebCore::endsOfNodeAreVisuallyDistinctPositions(WebCore::Node*)
	0.0%	0.1%	WebCore	 WebCore::Position::downstream(WebCore::EditingBoundaryCrossingRule) const
	0.0%	0.0%	WebCore	 WebCore::RenderObject::isBody() const
	0.0%	0.0%	WebCore	 WebCore::RenderBlock::isSelectionRoot() const
	0.0%	0.0%	WebCore	 WebCore::Node::rootEditableElement() const
	0.0%	0.0%	WebCore	 WebCore::enclosingVisualBoundary(WebCore::Node*)
	0.0%	0.0%	WebCore	 WebCore::RenderObject::isLegend() const
	0.0%	0.0%	WebCore	 WebCore::RenderObject::isHR() const
	0.0%	0.0%	WebCore	 WebCore::RenderBox::avoidsFloats() const
	0.0%	0.0%	WebCore	 WebCore::previousVisuallyDistinctCandidate(WebCore::Position const&)
	0.0%	0.0%	WebCore	 WebCore::Position::atEditingBoundary() const
	0.0%	0.0%	WebCore	 WebCore::highestEditableRoot(WebCore::Position const&)
	0.0%	0.0%	WebCore	 WebCore::enclosingNodeOfType(WebCore::Position const&, bool (*)(WebCore::Node const*), bool)
	0.0%	0.0%	WebCore	 WebCore::Document::body() const

[Ryosuke Niwa]

It seems like using hashset slows down the function.  I tried the following implementation:

bool canHaveChildrenForEditing(const Node* node)
{
    if (node->isTextNode() || !node->isElementNode())
        return false;

    DEFINE_STATIC_LOCAL(HashSet<QualifiedName>, tagNames, ());
    if (tagNames.isEmpty()) {
        tagNames.add(hrTag);
        tagNames.add(brTag);
        tagNames.add(imgTag);
        tagNames.add(buttonTag);
        tagNames.add(inputTag);
        tagNames.add(textareaTag);
        tagNames.add(objectTag);
        tagNames.add(iframeTag);
        tagNames.add(appletTag);
        tagNames.add(datagridTag);
#if ENABLE(WML)
        tagNames.add(WMLNames::doTag);
#endif
    }

    return !tagNames.contains(static_cast<const Element*>(node)->tagQName());
}

but this implementation was consistently slower 14-5s vs. 16-17s to run LayoutTests/editing/selection/extend-selection-*.html with --repeat 5.

[Darin Adler]

Qualified name comparison is just pointer comparison so we have to make sure we have code that takes advantage of that.

Since the set is small, a binary search of a vector might be faster than a hash table.

I am not sure that HashSet<QualifiedName> is sufficiently specialized to do pointer hashing. If it was we might see faster performance. A way to work around that and test that assumption would be to use a HashSet<QualifiedNameImpl*>.

[Ryosuke Niwa]

(In reply to comment #3)
> Qualified name comparison is just pointer comparison so we have to make sure we have code that takes advantage of that.
>
> I am not sure that HashSet<QualifiedName> is sufficiently specialized to do pointer hashing. If it was we might see faster performance. A way to work around that and test that assumption would be to use a HashSet<QualifiedNameImpl*>.

Ah, QualifiedNameImpl* seems to be considerably faster.  It's now 12s-13s, which is comparable to the performance obtained by making it a virtual function of Node.  But I found a number of bugs in this function (e.g. returns true for applet & embed elements) which could have been avoided if this function was virtual so I'm more inclined to make it a Node's virtual member function.

[Ryosuke Niwa]

Created attachment 80996 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 80996 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80996&action=review

It kinda sucks to polute Node.h like this.  I mean, certainly we follow this pattern for other areas of the code, but I wonder if in this case your original hashset isn't better?   Are there more general rules about what types of tags can/can't have editing kids?  should we move the false up to some PluginElement superclass instead of <object>, <embed> directly?

> Source/WebCore/dom/Node.h:234
> +    // FIXME: We should return false here and return true in ContainerNode
> +    virtual bool canHaveChildrenForEditing() const { return true; }

Why not just fix this?

[Eric Seidel (no email)]

I'm OK with the virtual function.  But see my above commetns.

[Ryosuke Niwa]

(In reply to comment #6)
> (From update of attachment 80996 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80996&action=review
> 
> It kinda sucks to polute Node.h like this.  I mean, certainly we follow this pattern for other areas of the code, but I wonder if in this case your original hashset isn't better?

I think it's okay to add a member function to Node in this case because the concept of not being able to have children is also related to Position's behavior.  For example, positions or range boundary points like [br, 0] or [img, 0] doesn't make sense, and this is not necessarily editing-specific knowledge.

Maybe we should rename it to something more general like canHaveChildrenForPosition.   I was also thinking that we can have canHaveOffsetInside to replace editingIgnoresContent, and have canHaveChildrenForEditing in htmlediting.h instead because there are very few places where we call canHaveChildrenForPosition but there are many places where we call editingIgnoresContent.

>Are there more general rules about what types of tags can/can't have editing kids?  should we move the false up to some PluginElement superclass instead of <object>, <embed> directly?

I do return false in HTMLPlugInElement.h.  I listed all elements in ChangeLog just for the documentation purpose.

> > Source/WebCore/dom/Node.h:234
> > +    // FIXME: We should return false here and return true in ContainerNode
> > +    virtual bool canHaveChildrenForEditing() const { return true; }
> 
> Why not just fix this?

It seemed a risky change at the moment but I could do that.

[Ryosuke Niwa]

After my patch, the time profile looks much better although getFlag is still very hot.

	2.2%	2.2%	WebCore	WebCore::Node::getFlag(WebCore::Node::NodeFlags) const
	0.0%	0.8%	WebCore	 WebCore::Node::parentNode() const
	0.0%	0.1%	WebCore	  WebCore::PositionIterator::atEnd() const
	0.0%	0.1%	WebCore	  WebCore::Range::compareBoundaryPoints(WebCore::Node*, int, WebCore::Node*, int)
	0.0%	0.1%	WebCore	  WebCore::PositionIterator::atStart() const
	0.0%	0.1%	WebCore	  WebCore::Range::setStart(WTF::PassRefPtr<WebCore::Node>, int, int&)
	0.0%	0.1%	WebCore	  WebCore::Range::setEnd(WTF::PassRefPtr<WebCore::Node>, int, int&)
	0.0%	0.1%	WebCore	  WebCore::positionInParentBeforeNode(WebCore::Node const*)
	0.0%	0.1%	WebCore	  WebCore::Position::downstream(WebCore::EditingBoundaryCrossingRule) const
	0.0%	0.1%	WebCore	  WebCore::Position::atStartOfTree() const
	0.0%	0.1%	WebCore	  WebCore::enclosingVisualBoundary(WebCore::Node*)
	0.0%	0.0%	WebCore	  WebCore::PositionIterator::increment()
	0.0%	0.0%	WebCore	  WebCore::Node::rootEditableElement() const
	0.0%	0.0%	WebCore	  WebCore::Range::commonAncestorContainer(WebCore::Node*, WebCore::Node*)
	0.0%	0.0%	WebCore	  WebCore::PositionIterator::operator WebCore::Position() const
	0.0%	0.0%	WebCore	  WebCore::Position::parentAnchoredEquivalent() const
	0.0%	0.0%	WebCore	  WebCore::highestEditableRoot(WebCore::Position const&)
	0.0%	0.5%	WebCore	 WebCore::Node::isElementNode() const
	0.0%	0.3%	WebCore	  WebCore::Node::hasTagName(WebCore::QualifiedName const&) const
	0.0%	0.1%	WebCore	   WebCore::endsOfNodeAreVisuallyDistinctPositions(WebCore::Node*)
	0.0%	0.1%	WebCore	    WebCore::enclosingVisualBoundary(WebCore::Node*)
	0.0%	0.0%	WebCore	    WebCore::Position::upstream(WebCore::EditingBoundaryCrossingRule) const
	0.0%	0.1%	WebCore	   WebCore::highestEditableRoot(WebCore::Position const&)
	0.0%	0.0%	WebCore	   WebCore::RenderObject::isHR() const
	0.0%	0.0%	WebCore	   WebCore::Position::downstream(WebCore::EditingBoundaryCrossingRule) const
	0.0%	0.0%	WebCore	   WebCore::Node::rootEditableElement() const
	0.0%	0.0%	WebCore	   WebCore::Document::body() const
	0.0%	0.1%	WebCore	  WebCore::toElement(WebCore::Node const*)
	0.0%	0.1%	WebCore	  WebCore::isTableElement(WebCore::Node*)
	0.0%	0.0%	WebCore	  WebCore::Node::rootEditableElement() const
	0.0%	0.0%	WebCore	  WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
	0.0%	0.4%	WebCore	 WebCore::Node::isContainerNode() const
	0.0%	0.3%	WebCore	 WebCore::Node::isTextNode() const
	0.0%	0.0%	WebCore	 WebCore::Node::parentNodeGuaranteedHostFree() const
	0.0%	0.0%	WebCore	 WebCore::Node::isShadowRoot() const
	0.0%	0.0%	WebCore	 WebCore::Node::hasRareData() const
	0.0%	0.0%	WebCore	 WebCore::Node::childNeedsStyleRecalc() const
	0.0%	0.0%	WebCore	 WebCore::Node::appendChild(WTF::PassRefPtr<WebCore::Node>, int&, bool)
	0.0%	0.0%	WebCore	 WebCore::editingIgnoresContent(WebCore::Node const*)
	1.5%	1.5%	libicucore.A.dylib	icu::RuleBasedBreakIterator::handleNext(icu::RBBIStateTable const*)
	1.3%	1.3%	JavaScriptCore	WTF::isMainThread()
	1.3%	1.3%	libSystem.B.dylib	__spin_lock
	1.0%	1.0%	libobjc.A.dylib	objc_msgSend
	1.0%	1.0%	WebCore	WebCore::Node::renderer() const
	0.7%	0.7%	WebCore	WebCore::Node::parentNode() const
	0.7%	0.7%	WebCore	WTF::RefPtr<WebCore::RenderStyle>::get() const
	0.7%	0.7%	WebCore	WebCore::Node::document() const
	0.6%	0.6%	WebCore	WebCore::TreeShared<WebCore::ContainerNode>::deref()

[James Robinson]

What's the performance delta here in a release build?

[Ryosuke Niwa]

(In reply to comment #10)
> What's the performance delta here in a release build?

Testing times for "Tools/Scripts/run-webkit-tests --repeat 50 LayoutTests/editing/selection/extend-selection-*.html" on my MacPro (Snow Leopard):

r77451:
30.38s
30.34s
30.39s
avg: 30.4s

r77451 + patch:
29.63s
29.89s
29.80s
avg: 29.8s

So it's about 2% improvement.

[Ryosuke Niwa]

Results for "Tools/Scripts/run-webkit-tests --repeat 50 LayoutTests/editing/selection/move-left-right.html"

r77451:
52.31s
52.05s
51.99s
52.14s
avg: 52.1s

r77451 + patch:
51.82s
51.18s
51.89s
51.32s
avg: 51.6s

The average improvement: 1.1%

[Ryosuke Niwa]

But note that we have 20% improvement on debug builds.  Since these each test takes 2-3s to finish, I think this is a valuable change.

[Ryosuke Niwa]

Created attachment 81041 [details]
added canContainPosition instead of canHaveChildrenForEditing

[Ryosuke Niwa]

(In reply to comment #10)
> What's the performance delta here in a release build?

By the way, release build performance should be addressed by the bug 53649.  This bug is all about improving debug build's performance because it has been wasting my time.  It also has a nice side-effect of making test less flaky on debug bots: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=extend-selection-character.html&master=ChromiumWebkit

[Ryosuke Niwa]

Eric, could you review this patch?

[Eric Seidel (no email)]

Comment on attachment 81041 [details]
added canContainPosition instead of canHaveChildrenForEditing

View in context: https://bugs.webkit.org/attachment.cgi?id=81041&action=review

> Source/WebCore/wml/WMLBRElement.h:44
> +    bool canContainPosition() const { return false; }

Does WML support editing at all?

[Ryosuke Niwa]

(In reply to comment #17)
> (From update of attachment 81041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81041&action=review
> 
> > Source/WebCore/wml/WMLBRElement.h:44
> > +    bool canContainPosition() const { return false; }
> 
> Does WML support editing at all?

The old function specifically supported doTag so I had assumed that we support it.

[Ryosuke Niwa]

Could someone review this patch?

[Eric Seidel (no email)]

Comment on attachment 81041 [details]
added canContainPosition instead of canHaveChildrenForEditing

View in context: https://bugs.webkit.org/attachment.cgi?id=81041&action=review

It also seems like this "can contain" concept is very similar to being a replaced element in the renderer.  Are we sure they're not identical?

> Source/WebCore/dom/Document.h:440
> +    bool canContainPosition() const { return true; }

Since position isn't a DOM concept (yet), I think we should rename this to talk about range end points.  canContainRangeEndPoint would be fine.  You had some other suggestions over IRC which sounded fine too.

> Source/WebCore/editing/htmlediting.h:87
> +inline bool canHaveChildrenForEditing(const Node* node)

Should have a FIXME about renmaing this function, since the name makes no sense to me.  SEems to have nothing to do with children.

[Ryosuke Niwa]

Created attachment 81923 [details]
Fixed per Eric's comment

[Ryosuke Niwa]

Created attachment 81931 [details]
Added back HTMLMeterElement change

[Eric Seidel (no email)]

Comment on attachment 81931 [details]
Added back HTMLMeterElement change

View in context: https://bugs.webkit.org/attachment.cgi?id=81931&action=review

Otherwise looks ok.

> Source/WebCore/html/HTMLElement.cpp:-658
> -    if (document()->frame() && document()->frame()->isContentEditable())

huh?

[Ryosuke Niwa]

Thanks for the review!

(In reply to comment #23)
> (From update of attachment 81931 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81931&action=review
> 
> Otherwise looks ok.
> 
> > Source/WebCore/html/HTMLElement.cpp:-658
> > -    if (document()->frame() && document()->frame()->isContentEditable())
> 
> huh?

Oops, I merged the patch I was reviewing for the bug 53031!  I'll fix before landing it.

[Ryosuke Niwa]

Committed r78219: <http://trac.webkit.org/changeset/78219>

[Philippe Normand]

Seems like this patch broke editing/style/iframe-onload-crash.html on GTK 64-bit Debug:

http://webkit-bots.igalia.com/amd64/svn_78223.core-when_1297353842-_-who_DumpRenderTree-_-why_11.trace.html


#0  0x00007fb5124a92e4 in WebCore::TextIterator::rangeFromLocationAndLength (scope=0x7fb500f31af0, rangeLocation=2, rangeLength=0, forSelectionPreservation=true) at ../../Source/WebCore/editing/TextIterator.cpp:2350
2350	                        ASSERT(!ec);

#0  0x00007fb5124a92e4 in WebCore::TextIterator::rangeFromLocationAndLength (scope=0x7fb500f31af0, rangeLocation=2, rangeLength=0, forSelectionPreservation=true) at ../../Source/WebCore/editing/TextIterator.cpp:2350
#1  0x00007fb51242d568 in WebCore::ApplyStyleCommand::applyBlockStyle (this=0x197bee0, style=0x194db50) at ../../Source/WebCore/editing/ApplyStyleCommand.cpp:619
#2  0x00007fb51242cab7 in WebCore::ApplyStyleCommand::doApply (this=0x197bee0) at ../../Source/WebCore/editing/ApplyStyleCommand.cpp:539
#3  0x00007fb512450ae5 in WebCore::EditCommand::apply (this=0x197bee0) at ../../Source/WebCore/editing/EditCommand.cpp:92
#4  0x00007fb51243b7b7 in WebCore::CompositeEditCommand::applyCommandToComposite (this=0x1be8b20, cmd=...) at ../../Source/WebCore/editing/CompositeEditCommand.cpp:102
#5  0x00007fb51248a61e in WebCore::RemoveFormatCommand::doApply (this=0x1be8b20) at ../../Source/WebCore/editing/RemoveFormatCommand.cpp:90
#6  0x00007fb512450ae5 in WebCore::EditCommand::apply (this=0x1be8b20) at ../../Source/WebCore/editing/EditCommand.cpp:92
#7  0x00007fb512451146 in WebCore::applyCommand (command=...) at ../../Source/WebCore/editing/EditCommand.cpp:214
#8  0x00007fb51245df3a in WebCore::Editor::removeFormattingAndStyle (this=0x12bbdd8) at ../../Source/WebCore/editing/Editor.cpp:843
#9  0x00007fb51245658d in WebCore::executeRemoveFormat (frame=0x12bb800) at ../../Source/WebCore/editing/EditorCommand.cpp:929
#10 0x00007fb51245818e in WebCore::Editor::Command::execute (this=0x7fff9cc978b0, parameter=..., triggeringEvent=0x0) at ../../Source/WebCore/editing/EditorCommand.cpp:1640
#11 0x00007fb5123746b5 in WebCore::Document::execCommand (this=0x7fb500f31af0, commandName=..., userInterface=false, value=...) at ../../Source/WebCore/dom/Document.cpp:3974
#12 0x00007fb512bf470c in WebCore::jsDocumentPrototypeFunctionExecCommand (exec=0x7fb504cdf1a8) at DerivedSources/WebCore/JSDocument.cpp:2208

[Philippe Normand]

Reopening this as the patch has been rolled out

[Ryosuke Niwa]

(In reply to comment #27)
> Reopening this as the patch has been rolled out

Did a similar crash happened on other platforms?  This stack trace doesn't tell me anything about the cause of this failure.  As I don't have access to GTK port, it's impossible for me to diagnose the issue.  Could someone tell me what kind of exception we're getting?  I bet it's index error or something in which case, the only viable option here is to remove the assertion...

[Ryosuke Niwa]

Looking at the test (http://trac.webkit.org/browser/trunk/LayoutTests/editing/style/iframe-onload-crash.html), I don't see how this change can cause a crash only on GTK.  It's got to be some bug on GTK port.

[Philippe Normand]

(In reply to comment #29)
> Looking at the test (http://trac.webkit.org/browser/trunk/LayoutTests/editing/style/iframe-onload-crash.html), I don't see how this change can cause a crash only on GTK.  It's got to be some bug on GTK port.

Shouldn't runEnd.node()->canContainRangeEndPoint() be checked before attempting  textRunRange->setEnd() ?

[Ryosuke Niwa]

(In reply to comment #30)
> (In reply to comment #29)
> > Looking at the test (http://trac.webkit.org/browser/trunk/LayoutTests/editing/style/iframe-onload-crash.html), I don't see how this change can cause a crash only on GTK.  It's got to be some bug on GTK port.
> 
> Shouldn't runEnd.node()->canContainRangeEndPoint() be checked before attempting  textRunRange->setEnd() ?

It doesn't need to.  Even though the function name suggests that we should be doing that, Range code doesn't currently depend on canContainRangeEndPoint at all.  One thing you can try is to apply my patch but keep the original editingIgnoresContents and assert that the results are the same.  If that assert hits before the other one, then we'll be able to see what node is causing a problem.  If not, it's a bug in GTK DRT.

[Philippe Normand]

Can you join IRC #webkit ? i am philn-tp. This will go much faster on irc vs bugzilla :)

[Philippe Normand]

I did what you asked (I think):

ASSERTION FAILED: ret == notContainRangeEndPoint
../../Source/WebCore/editing/htmlediting.cpp(73) : bool WebCore::editingIgnoresContent(const WebCore::Node*)

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff576d322 in WebCore::editingIgnoresContent (node=0xd5dfa0)
    at ../../Source/WebCore/editing/htmlediting.cpp:73
73	    ASSERT(ret == notContainRangeEndPoint);
(gdb) ret
Make WebCore::editingIgnoresContent(WebCore::Node const*) return now? (y or n) n
Not confirmed
(gdb) p ret
$1 = true
(gdb) p notContainRangeEndPoint
$2 = false
(gdb) p node
$3 = (const WebCore::Node *) 0xd5dfa0
(gdb) p node->nodeName()
$4 = "DATAGRID"
(gdb) p node->nodeValue()
$5 = "(null)"

[Ryosuke Niwa]

(In reply to comment #32)
> Can you join IRC #webkit ? i am philn-tp. This will go much faster on irc vs bugzilla :)

I'm in JST (UTC+0900) right now so I don't think we can talk on IRC unfortunately :(

(In reply to comment #33)
> I did what you asked (I think):

Thanks!
> (gdb) ret
> Make WebCore::editingIgnoresContent(WebCore::Node const*) return now? (y or n) n
> Not confirmed
> (gdb) p ret
> $1 = true

Does this mean that new canContainRangeEndPoint returned true?  That's odd given my changes to http://trac.webkit.org/changeset/78219/trunk/Source/WebCore/html/HTMLDataGridElement.h, right?  Unless... GTK port doesn't support datagrid.  Is that so?

I guess I can manually disable datagrid support and see what happens.  In fact, the same crash might reproduce if I just renamed the element name datagrid1 or something.  Let me try that ASAP.

Thanks for your help.  Really appreciated!

[Martin Robinson]

(In reply to comment #34)
> Does this mean that new canContainRangeEndPoint returned true?  That's odd given my changes to http://trac.webkit.org/changeset/78219/trunk/Source/WebCore/html/HTMLDataGridElement.h, right?  Unless... GTK port doesn't support datagrid.  Is that so?

DataGrid looks to be disabled completely on GTK+.

[Ryosuke Niwa]

I manually disabled datagrid element but I still could not reproduce the crash.  I also renamed datagrid tag in the test to datagrid1 and others but that didn't work either.  Could someone from GTK port debug this?

[Ryosuke Niwa]

Created attachment 82420 [details]
patch to land

[Philippe Normand]

Created attachment 82434 [details]
updated patch (works on GTK)

[Ryosuke Niwa]

(In reply to comment #38)
> Created an attachment (id=82434) [details]
> updated patch (works on GTK)

Thanks for the patch!  I still don't understand why this crash only reproduces on GTK.  I guess I'll have to get a Linux box to investigate it.

[Philippe Normand]

My understanding is that since the datagrid support is disabled on GTK there's no HTMLDataGridElement class and the default implementation of the added virtual method is called, which makes the editingIgnoresContents() method return wrong result in that specific case.

But maybe I'm just wrong ;)

[Ryosuke Niwa]

(In reply to comment #40)
> My understanding is that since the datagrid support is disabled on GTK there's no HTMLDataGridElement class and the default implementation of the added virtual method is called, which makes the editingIgnoresContents() method return wrong result in that specific case.

If that was the case, then I should have been able to reproduce the crash when I disabled datagrid element on my Mac.

[Ryosuke Niwa]

We were able to track the root cause of this failure and fix it in http://trac.webkit.org/changeset/81266.

[Darin Adler]

Comment on attachment 82434 [details]
updated patch (works on GTK)

View in context: https://bugs.webkit.org/attachment.cgi?id=82434&action=review

Since endpoint is a single word, it should be RangeEndpoint, not RangeEndPoint.

What does this make 1-2% faster?

> Source/WebCore/ChangeLog:9
> +        canContainRangeEndPoint to Node. It returns true whenever the associated node can have be a container

can have be

[Ryosuke Niwa]

(In reply to comment #43)
> (From update of attachment 82434 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82434&action=review
> 
> Since endpoint is a single word, it should be RangeEndpoint, not RangeEndPoint.
so It'
Good point.

> What does this make 1-2% faster?

"Tools/Scripts/run-webkit-tests --repeat 50 LayoutTests/editing/selection/extend-selection-*.html" on release build.  It'll also make debug builds 40% faster so as to boost editing folks' productivity :D

[Eric Seidel (no email)]

Comment on attachment 82434 [details]
updated patch (works on GTK)

View in context: https://bugs.webkit.org/attachment.cgi?id=82434&action=review

OK.  I feel like this kinda makes it harder to tell what elements we're white listing... I'm also wondering how people adding new elements are going to get this right.

> Source/WebCore/editing/htmlediting.cpp:76
> +#if !ENABLE(DATAGRID)
> +    if (node->hasTagName(datagridTag))
> +        ignoreContent = true;
>  #endif

I'm confused why this can't use the canContainRangeEndPoint stuff.

[Ryosuke Niwa]

Comment on attachment 82434 [details]
updated patch (works on GTK)

I think this patch is out of date as datagrid has been removed in http://trac.webkit.org/changeset/84991.

[Ryosuke Niwa]

Created attachment 92649 [details]
updated

[Ryosuke Niwa]

Comment on attachment 92649 [details]
updated

oops, some test crashes.

[Ryosuke Niwa]

Created attachment 92655 [details]
Added back forgotten change to Document.h

[Ryosuke Niwa]

Eric, could you review the new patch again?  The one you r+ was totally out of date due to a bug being fixed and datagrid being removed.

[Eric Seidel (no email)]

Comment on attachment 92655 [details]
Added back forgotten change to Document.h

OK.  Seems this is very similar to the question of if it uses a replaced renderer.

[Ryosuke Niwa]

Comment on attachment 92655 [details]
Added back forgotten change to Document.h

Thanks for the review, Eric!

[WebKit Commit Bot]

Comment on attachment 92655 [details]
Added back forgotten change to Document.h

Clearing flags on attachment: 92655

Committed r86491: <http://trac.webkit.org/changeset/86491>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.