Bug 131982 – REGRESSION (r167652): Broke fast/regions/cssom/region-range-for-box-crash.html in debug mode

• Component: CSS
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

Patch for bug #131511 which landed in r167652 broke fast/regions/cssom/region-range-for-box-crash.html in debug mode.

04:21:37.557 30890   ASSERTION FAILED: !m_code || m_code == defaultExceptionCode
04:21:37.557 30890   /Volumes/Data/slave/mavericks-debug/build/Source/WebCore/dom/Range.h(81) : 
04:21:37.557 30890   1   0x1015fd0a0 WTFCrash
04:21:37.557 30890   2   0x1034f8393 WebCore::NoExceptionAssertionChecker::~NoExceptionAssertionChecker()
04:21:37.557 30890   3   0x1034f8335 WebCore::NoExceptionAssertionChecker::~NoExceptionAssertionChecker()
04:21:37.557 30890   4   0x1044a6d5b WebCore::Range::Range(WebCore::Document&, WTF::PassRefPtr<WebCore::Node>, int, WTF::PassRefPtr<WebCore::Node>, int)
04:21:37.557 30890   5   0x1044a541b WebCore::Range::Range(WebCore::Document&, WTF::PassRefPtr<WebCore::Node>, int, WTF::PassRefPtr<WebCore::Node>, int)
04:21:37.557 30890   6   0x10449c6f3 WebCore::Range::create(WebCore::Document&, WTF::PassRefPtr<WebCore::Node>, int, WTF::PassRefPtr<WebCore::Node>, int)
04:21:37.557 30890   7   0x1047d64a8 WebCore::RenderView::splitSelectionBetweenSubtrees(WebCore::RenderObject*, int, WebCore::RenderObject*, int, WebCore::RenderView::SelectionRepaintMode)
04:21:37.557 30890   8   0x1047d4ce1 WebCore::RenderView::setSelection(WebCore::RenderObject*, int, WebCore::RenderObject*, int, WebCore::RenderView::SelectionRepaintMode)
04:21:37.557 30890   9   0x103675bb4 WebCore::FrameSelection::updateAppearance()
04:21:37.557 30890   10  0x103675437 WebCore::FrameSelection::updateAndRevealSelection()
04:21:37.557 30890   11  0x103673aa4 WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&, unsigned int, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)
04:21:37.557 30890   12  0x10367cf4c WebCore::FrameSelection::selectAll()
04:21:37.557 30890   13  0x10348ae78 WebCore::executeSelectAll(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)
04:21:37.557 30890   14  0x103486cf5 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const
04:21:37.558 30890   15  0x10331ea3e WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)
04:21:37.558 30890   16  0x103c2cd0a WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*)
04:21:37.558 30890   17  0x52068c001034
04:21:37.558 30890   18  0x10140b648 llint_entry
04:21:37.558 30890   19  0x101404da4 callToJavaScript
04:21:37.558 30890   20  0x10129efbd JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
04:21:37.558 30890   21  0x101283929 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
04:21:37.558 30890   22  0x100f5820e JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
04:21:37.558 30890   23  0x100f58273 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, JSC::JSValue*)
04:21:37.558 30890   24  0x103b8ae7b WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, JSC::JSValue*)
04:21:37.558 30890   25  0x103d249a8 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*)
04:21:37.558 30890   26  0x1034f61df WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow>&)
04:21:37.558 30890   27  0x1034f5aae WebCore::EventTarget::fireEventListeners(WebCore::Event*)
04:21:37.558 30890   28  0x103422d0b WebCore::DOMWindow::dispatchEvent(WTF::PassRefPtr<WebCore::Event>, WTF::PassRefPtr<WebCore::EventTarget>)
04:21:37.558 30890   29  0x103429db8 WebCore::DOMWindow::dispatchLoadEvent()
04:21:37.558 30890   30  0x1033166ad WebCore::Document::dispatchWindowLoadEvent()
04:21:37.558 30890   31  0x103313ad3 WebCore::Document::implicitClose()

[Manuel Rego Casasnovas]

Updated test expectations in r167655.

[Manuel Rego Casasnovas]

After debugging this for a while the problem is in RenderView::splitSelectionBetweenSubtrees() when we try to create the |intialRange|:
        RefPtr<Range> initialRange = Range::create(document(), start->node(), startPos, end->node(), endPos);

Basically, Range::create(), calls Range::setEnd() which ends up calling Range::checkNodeWOffset() which returns an error because of the element has not children:
        case Node::ELEMENT_NODE:
        case Node::ENTITY_REFERENCE_NODE:
        case Node::XPATH_NAMESPACE_NODE: {
            if (!offset)
                return 0;
            Node* childBefore = n->childNode(offset - 1);
            if (!childBefore)
                ec = INDEX_SIZE_ERR;
            return childBefore;
        }

This is because of in FrameSelection we have:
        view->setSelection(startRenderer, startPos.deprecatedEditingOffset(), endRenderer, endPos.deprecatedEditingOffset());

And Position::deprecatedEditingOffset() is calling lastOffsetForEditing() which is returning 1 for the <input> as explained in the following comment:
// This method is used to create positions in the DOM. It returns the maximum valid offset
// in a node. It returns 1 for some elements even though they do not have children, which
// creates technically invalid DOM Positions. Be sure to call parentAnchoredEquivalent
// on a Position before using it to create a DOM Range, or an exception will be thrown.
int lastOffsetForEditing(const Node* node)

As we're only interested in the nodes for the |initialRange| we can avoid the positions and the issue seems to be fixed.

[Manuel Rego Casasnovas]

Created attachment 229888 [details]
Patch

[Dave Hyatt]

Comment on attachment 229888 [details]
Patch

r=me

[Manuel Rego Casasnovas]

Comment on attachment 229888 [details]
Patch

Clearing flags on attachment: 229888

Committed r167675: <http://trac.webkit.org/changeset/167675>

[Manuel Rego Casasnovas]

All reviewed patches have been landed.  Closing bug.

[Manuel Rego Casasnovas]

Reopen bug, as the fix was not right. It was just hiding the issue due to the wrong !renderer->canBeSelectionLeaf() a few lines below, which has been fixed in bug #132493.

We need to find a proper solution for this issue. The problem here is that we're not able to create valid DOM ranges from the positions we receive in RenderView::setSelection().

As explained in comment #2, this is because of we're getting 1 as |endPos| for elements that don't have children. We have to be able to create a DOM range from the positions we receive in RenderView::setSelection() from FrameSelection::updateAppearance().

[Manuel Rego Casasnovas]

Created attachment 230982 [details]
Patch

This could be a possible solution but it's introducing regressions in other tests in debug mode. Maybe we should try to create a valid Position just before creating the DOM range, but I'm still not sure if that would fix or not the problem.

[Build Bot]

Comment on attachment 230982 [details]
Patch

Attachment 230982 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5698576785080320

New failing tests:
svg/text/non-bmp-positioning-lists.svg
fast/repaint/selection-gap-transformed-fixed-child.html
editing/selection/contains-node-crash.html
editing/execCommand/format-block-at-root.html
fast/repaint/selection-gap-flipped-absolute-child.html
fast/repaint/selection-gap-absolute-child.html
fast/repaint/selection-gap-transformed-absolute-child.html

[Build Bot]

Created attachment 230987 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 230982 [details]
Patch

Attachment 230982 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5400720803102720

New failing tests:
svg/text/non-bmp-positioning-lists.svg
fast/repaint/selection-gap-transformed-fixed-child.html
editing/selection/contains-node-crash.html
editing/execCommand/format-block-at-root.html
fast/repaint/selection-gap-flipped-absolute-child.html
fast/repaint/selection-gap-flipped-fixed-child.html
fast/repaint/selection-gap-fixed-child.html
fast/repaint/selection-gap-absolute-child.html
fast/repaint/selection-gap-transformed-absolute-child.html

[Build Bot]

Created attachment 230988 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Manuel Rego Casasnovas]

Just as a wild idea, we could probably try to avoid create DOM ranges in RenderView. Instead of some kind of structure with the data we need for the selection:
{
    RenderObject* start;
    int startPos;
    RenderObject* end;
    int endPost;
}

This would probably fix the kind of problems we're having here.

[Build Bot]

Comment on attachment 230982 [details]
Patch

Attachment 230982 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6208198277070848

New failing tests:
fast/repaint/selection-gap-transformed-fixed-child.html
editing/selection/contains-node-crash.html
editing/execCommand/format-block-at-root.html
fast/repaint/selection-gap-flipped-absolute-child.html
fast/repaint/selection-gap-flipped-fixed-child.html
fast/repaint/selection-gap-fixed-child.html
fast/repaint/selection-gap-absolute-child.html
fast/repaint/selection-gap-transformed-absolute-child.html

[Build Bot]

Created attachment 230995 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Manuel Rego Casasnovas]

This has been fixed in r169105 (bug #132720).

[Alexey Proskuryakov]

The test still crashes, filed bug 133124.