Bug 131982 - REGRESSION (r167652): Broke fast/regions/cssom/region-range-for-box-crash.html in debug mode
Summary: REGRESSION (r167652): Broke fast/regions/cssom/region-range-for-box-crash.htm...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-22 04:55 PDT by Manuel Rego Casasnovas
Modified: 2014-05-20 10:11 PDT (History)
9 users (show)

See Also:


Attachments
Patch (3.14 KB, patch)
2014-04-22 09:30 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (3.16 KB, patch)
2014-05-07 03:05 PDT, Manuel Rego Casasnovas
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (537.79 KB, application/zip)
2014-05-07 04:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (554.35 KB, application/zip)
2014-05-07 04:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (581.94 KB, application/zip)
2014-05-07 05:35 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2014-04-22 04:55:51 PDT
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()
Comment 1 Manuel Rego Casasnovas 2014-04-22 05:01:33 PDT
Updated test expectations in r167655.
Comment 2 Manuel Rego Casasnovas 2014-04-22 09:30:16 PDT
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.
Comment 3 Manuel Rego Casasnovas 2014-04-22 09:30:50 PDT
Created attachment 229888 [details]
Patch
Comment 4 Dave Hyatt 2014-04-22 12:41:22 PDT
Comment on attachment 229888 [details]
Patch

r=me
Comment 5 Manuel Rego Casasnovas 2014-04-22 12:48:59 PDT
Comment on attachment 229888 [details]
Patch

Clearing flags on attachment: 229888

Committed r167675: <http://trac.webkit.org/changeset/167675>
Comment 6 Manuel Rego Casasnovas 2014-04-22 12:49:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Manuel Rego Casasnovas 2014-05-07 02:45:59 PDT
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().
Comment 8 Manuel Rego Casasnovas 2014-05-07 03:05:40 PDT
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.
Comment 9 Build Bot 2014-05-07 04:05:44 PDT
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
Comment 10 Build Bot 2014-05-07 04:05:47 PDT
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
Comment 11 Build Bot 2014-05-07 04:27:33 PDT
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
Comment 12 Build Bot 2014-05-07 04:27:37 PDT
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
Comment 13 Manuel Rego Casasnovas 2014-05-07 04:35:04 PDT
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.
Comment 14 Build Bot 2014-05-07 05:35:28 PDT
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
Comment 15 Build Bot 2014-05-07 05:35:31 PDT
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
Comment 16 Manuel Rego Casasnovas 2014-05-20 04:03:23 PDT
This has been fixed in r169105 (bug #132720).
Comment 17 Alexey Proskuryakov 2014-05-20 10:11:45 PDT
The test still crashes, filed bug 133124.