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*)
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
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.
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*>.
(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.
Created attachment 80996 [details] Patch
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?
I'm OK with the virtual function. But see my above commetns.
(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.
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()
What's the performance delta here in a release build?
(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.
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%
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.
Created attachment 81041 [details] added canContainPosition instead of canHaveChildrenForEditing
(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
Eric, could you review this patch?
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?
(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.
Could someone review this patch?
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.
Created attachment 81923 [details] Fixed per Eric's comment
Created attachment 81931 [details] Added back HTMLMeterElement change
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?
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.
Committed r78219: <http://trac.webkit.org/changeset/78219>
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
Reopening this as the patch has been rolled out
(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...
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.
(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() ?
(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.
Can you join IRC #webkit ? i am philn-tp. This will go much faster on irc vs bugzilla :)
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)"
(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!
(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+.
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?
Created attachment 82420 [details] patch to land
Created attachment 82434 [details] updated patch (works on GTK)
(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.
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 ;)
(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.
We were able to track the root cause of this failure and fix it in http://trac.webkit.org/changeset/81266.
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
(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 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 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.
Created attachment 92649 [details] updated
Comment on attachment 92649 [details] updated oops, some test crashes.
Created attachment 92655 [details] Added back forgotten change to Document.h
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 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 on attachment 92655 [details] Added back forgotten change to Document.h Thanks for the review, Eric!
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>
All reviewed patches have been landed. Closing bug.