Bug 53564 - Make canHaveChildrenForEditing more efficient
Summary: Make canHaveChildrenForEditing more efficient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 54215 56505
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-01 17:58 PST by Ryosuke Niwa
Modified: 2011-05-14 11:10 PDT (History)
12 users (show)

See Also:


Attachments
Patch (20.29 KB, patch)
2011-02-02 16:13 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
added canContainPosition instead of canHaveChildrenForEditing (16.17 KB, patch)
2011-02-03 01:25 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed per Eric's comment (16.06 KB, patch)
2011-02-09 22:06 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Added back HTMLMeterElement change (18.67 KB, patch)
2011-02-10 00:05 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
patch to land (16.49 KB, patch)
2011-02-14 23:18 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
updated patch (works on GTK) (17.21 KB, patch)
2011-02-15 03:05 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
updated (13.02 KB, patch)
2011-05-06 15:26 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Added back forgotten change to Document.h (13.47 KB, patch)
2011-05-06 16:10 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-02-01 17:58:13 PST
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*)
Comment 1 Ryosuke Niwa 2011-02-01 18:25:56 PST
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
Comment 2 Ryosuke Niwa 2011-02-02 11:54:11 PST
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.
Comment 3 Darin Adler 2011-02-02 12:00:13 PST
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*>.
Comment 4 Ryosuke Niwa 2011-02-02 12:49:07 PST
(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.
Comment 5 Ryosuke Niwa 2011-02-02 16:13:17 PST
Created attachment 80996 [details]
Patch
Comment 6 Eric Seidel (no email) 2011-02-02 16:17:39 PST
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?
Comment 7 Eric Seidel (no email) 2011-02-02 16:18:02 PST
I'm OK with the virtual function.  But see my above commetns.
Comment 8 Ryosuke Niwa 2011-02-02 16:34:38 PST
(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.
Comment 9 Ryosuke Niwa 2011-02-02 16:37:48 PST
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()
Comment 10 James Robinson 2011-02-02 21:39:50 PST
What's the performance delta here in a release build?
Comment 11 Ryosuke Niwa 2011-02-03 00:35:38 PST
(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.
Comment 12 Ryosuke Niwa 2011-02-03 01:04:33 PST
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%
Comment 13 Ryosuke Niwa 2011-02-03 01:05:32 PST
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.
Comment 14 Ryosuke Niwa 2011-02-03 01:25:44 PST
Created attachment 81041 [details]
added canContainPosition instead of canHaveChildrenForEditing
Comment 15 Ryosuke Niwa 2011-02-03 01:43:11 PST
(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
Comment 16 Ryosuke Niwa 2011-02-04 20:56:19 PST
Eric, could you review this patch?
Comment 17 Eric Seidel (no email) 2011-02-05 00:58:49 PST
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?
Comment 18 Ryosuke Niwa 2011-02-05 06:50:49 PST
(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.
Comment 19 Ryosuke Niwa 2011-02-08 01:24:55 PST
Could someone review this patch?
Comment 20 Eric Seidel (no email) 2011-02-09 20:19:22 PST
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.
Comment 21 Ryosuke Niwa 2011-02-09 22:06:10 PST
Created attachment 81923 [details]
Fixed per Eric's comment
Comment 22 Ryosuke Niwa 2011-02-10 00:05:23 PST
Created attachment 81931 [details]
Added back HTMLMeterElement change
Comment 23 Eric Seidel (no email) 2011-02-10 00:59:31 PST
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?
Comment 24 Ryosuke Niwa 2011-02-10 03:46:30 PST
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.
Comment 25 Ryosuke Niwa 2011-02-10 06:47:42 PST
Committed r78219: <http://trac.webkit.org/changeset/78219>
Comment 26 Philippe Normand 2011-02-10 08:27:48 PST
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
Comment 27 Philippe Normand 2011-02-10 09:56:48 PST
Reopening this as the patch has been rolled out
Comment 28 Ryosuke Niwa 2011-02-10 16:26:52 PST
(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...
Comment 29 Ryosuke Niwa 2011-02-10 16:33:59 PST
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.
Comment 30 Philippe Normand 2011-02-11 06:30:49 PST
(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() ?
Comment 31 Ryosuke Niwa 2011-02-11 06:40:22 PST
(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.
Comment 32 Philippe Normand 2011-02-11 06:55:47 PST
Can you join IRC #webkit ? i am philn-tp. This will go much faster on irc vs bugzilla :)
Comment 33 Philippe Normand 2011-02-11 07:02:34 PST
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)"
Comment 34 Ryosuke Niwa 2011-02-11 13:51:33 PST
(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!
Comment 35 Martin Robinson 2011-02-11 14:04:14 PST
(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+.
Comment 36 Ryosuke Niwa 2011-02-14 23:18:03 PST
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?
Comment 37 Ryosuke Niwa 2011-02-14 23:18:49 PST
Created attachment 82420 [details]
patch to land
Comment 38 Philippe Normand 2011-02-15 03:05:04 PST
Created attachment 82434 [details]
updated patch (works on GTK)
Comment 39 Ryosuke Niwa 2011-02-15 06:10:05 PST
(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.
Comment 40 Philippe Normand 2011-02-15 06:17:26 PST
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 ;)
Comment 41 Ryosuke Niwa 2011-02-15 07:20:22 PST
(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.
Comment 42 Ryosuke Niwa 2011-03-16 14:14:42 PDT
We were able to track the root cause of this failure and fix it in http://trac.webkit.org/changeset/81266.
Comment 43 Darin Adler 2011-03-16 16:25:14 PDT
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
Comment 44 Ryosuke Niwa 2011-03-16 16:28:27 PDT
(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
Comment 45 Eric Seidel (no email) 2011-05-06 13:34:52 PDT
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.
Comment 46 Ryosuke Niwa 2011-05-06 13:45:35 PDT
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.
Comment 47 Ryosuke Niwa 2011-05-06 15:26:33 PDT
Created attachment 92649 [details]
updated
Comment 48 Ryosuke Niwa 2011-05-06 15:33:10 PDT
Comment on attachment 92649 [details]
updated

oops, some test crashes.
Comment 49 Ryosuke Niwa 2011-05-06 16:10:19 PDT
Created attachment 92655 [details]
Added back forgotten change to Document.h
Comment 50 Ryosuke Niwa 2011-05-13 17:23:12 PDT
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.
Comment 51 Eric Seidel (no email) 2011-05-14 00:16:25 PDT
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.
Comment 52 Ryosuke Niwa 2011-05-14 07:01:38 PDT
Comment on attachment 92655 [details]
Added back forgotten change to Document.h

Thanks for the review, Eric!
Comment 53 WebKit Commit Bot 2011-05-14 11:10:31 PDT
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>
Comment 54 WebKit Commit Bot 2011-05-14 11:10:40 PDT
All reviewed patches have been landed.  Closing bug.