53564 – Make canHaveChildrenForEditing more efficient

RESOLVED FIXED Bug 53564

Make canHaveChildrenForEditing more efficient

https://bugs.webkit.org/show_bug.cgi?id=53564

Summary Make canHaveChildrenForEditing more efficient

Ryosuke Niwa

Reported 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*)

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
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 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

Ryosuke Niwa

Comment 2 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.

Darin Adler

Comment 3 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*>.

Ryosuke Niwa

Comment 4 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.

Ryosuke Niwa

Comment 5 2011-02-02 16:13:17 PST

Created attachment 80996 [details] Patch

Eric Seidel (no email)

Comment 6 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?

Eric Seidel (no email)

Comment 7 2011-02-02 16:18:02 PST

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

Ryosuke Niwa

Comment 8 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.

Ryosuke Niwa

Comment 9 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()

James Robinson

Comment 10 2011-02-02 21:39:50 PST

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

Ryosuke Niwa

Comment 11 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.

Ryosuke Niwa

Comment 12 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%

Ryosuke Niwa

Comment 13 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.

Ryosuke Niwa

Comment 14 2011-02-03 01:25:44 PST

Created attachment 81041 [details] added canContainPosition instead of canHaveChildrenForEditing

Ryosuke Niwa

Comment 15 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

Ryosuke Niwa

Comment 16 2011-02-04 20:56:19 PST

Eric, could you review this patch?

Eric Seidel (no email)

Comment 17 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?

Ryosuke Niwa

Comment 18 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.

Ryosuke Niwa

Comment 19 2011-02-08 01:24:55 PST

Could someone review this patch?

Eric Seidel (no email)

Comment 20 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.

Ryosuke Niwa

Comment 21 2011-02-09 22:06:10 PST

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

Ryosuke Niwa

Comment 22 2011-02-10 00:05:23 PST

Created attachment 81931 [details] Added back HTMLMeterElement change

Eric Seidel (no email)

Comment 23 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?

Ryosuke Niwa

Comment 24 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.

Ryosuke Niwa

Comment 25 2011-02-10 06:47:42 PST

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

Philippe Normand

Comment 26 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

Philippe Normand

Comment 27 2011-02-10 09:56:48 PST

Reopening this as the patch has been rolled out

Ryosuke Niwa

Comment 28 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...

Ryosuke Niwa

Comment 29 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.

Philippe Normand

Comment 30 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() ?

Ryosuke Niwa

Comment 31 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.

Philippe Normand

Comment 32 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 :)

Philippe Normand

Comment 33 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)"

Ryosuke Niwa

Comment 34 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!

Martin Robinson

Comment 35 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+.

Ryosuke Niwa

Comment 36 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?

Ryosuke Niwa

Comment 37 2011-02-14 23:18:49 PST

Created attachment 82420 [details] patch to land

Philippe Normand

Comment 38 2011-02-15 03:05:04 PST

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

Ryosuke Niwa

Comment 39 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.

Philippe Normand

Comment 40 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 ;)

Ryosuke Niwa

Comment 41 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.

Ryosuke Niwa

Comment 42 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.

Darin Adler

Comment 43 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

Ryosuke Niwa

Comment 44 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

Eric Seidel (no email)

Comment 45 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.

Ryosuke Niwa

Comment 46 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.

Ryosuke Niwa

Comment 47 2011-05-06 15:26:33 PDT

Created attachment 92649 [details] updated

Ryosuke Niwa

Comment 48 2011-05-06 15:33:10 PDT

Comment on attachment 92649 [details] updated oops, some test crashes.

Ryosuke Niwa

Comment 49 2011-05-06 16:10:19 PDT

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

Ryosuke Niwa

Comment 50 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.

Eric Seidel (no email)

Comment 51 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.

Ryosuke Niwa

Comment 52 2011-05-14 07:01:38 PDT

Comment on attachment 92655 [details] Added back forgotten change to Document.h Thanks for the review, Eric!

WebKit Commit Bot

Comment 53 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>

WebKit Commit Bot

Comment 54 2011-05-14 11:10:40 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Enhancement

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component HTML Editing

Assignee

Ryosuke Niwa

Reported

2011-02-01 17:58 PST

Modified

2011-05-14 11:10 PDT History

CC List

12 users Show

URL

Keywords

Depends on

54215 56505

Blocks

Dependencies

tree graph