Bug 130844

Summary:

ASSERTION FAILED: candidate.isCandidate() in WebCore::canonicalizeCandidate

Product:

WebKit

Reporter:

Renata Hodovan <rhodovan.u-szeged>

Component:

HTML Editing

Assignee:

Jack <shihchieh_lee>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, bfulgham, cdumez, darin, esprehn+autocc, ews-watchlist, ggaren, kangil.han, kling, rniwa, shihchieh_lee, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

116980

Attachments:

Description	Flags
Test case	none
Test	none
Patch	none
Patch	none
Test result of insert-list-in-table-cell-07.html without patch.	none
Test result of insert-list-in-table-cell-07.html with patch.	none
Patch	none
Patch	none

Renata Hodovan

Reported 2014-03-27 10:00:42 PDT

Created attachment 227952 [details] Test case The test hits the assertion: <body contenteditable="true"> <fieldset> <sup contenteditable="false"> <svg> <polyline onload='document.execCommand("selectall", true, null)'> The related backtrace: ASSERTION FAILED: candidate.isCandidate() /home/reni2/data/REPOS/webkit_sec/Source/WebCore/editing/VisiblePosition.cpp(496) : WebCore::Position WebCore::canonicalizeCandidate(const WebCore::Position&) 1 0x7ffff5ed9db5 WTFCrash 2 0x7ffff10e6977 3 0x7ffff10e6b8a WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) 4 0x7ffff10e4a8d WebCore::VisiblePosition::init(WebCore::Position const&, WebCore::EAffinity) 5 0x7ffff10e4a53 WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&, WebCore::EAffinity) 6 0x7ffff10e8adc WebCore::VisibleSelection::setBaseAndExtentToDeepEquivalents() 7 0x7ffff10e9b57 WebCore::VisibleSelection::validate(WebCore::TextGranularity) 8 0x7ffff10e7cac WebCore::VisibleSelection::VisibleSelection(WebCore::Position const&, WebCore::Position const&, WebCore::EAffinity, bool) 9 0x7ffff10e7f15 WebCore::VisibleSelection::selectionFromContentsOfNode(WebCore::Node*) 10 0x7ffff10a9f5c WebCore::FrameSelection::selectAll() 11 0x7ffff109d15e 12 0x7ffff109eb24 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const 13 0x7ffff0f58f4c WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) 14 0x7ffff1f340fb WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*) 15 0x7fff9b6ce0b4 Program received signal SIGSEGV, Segmentation fault. 0x00007ffff5ed9dba in WTFCrash () at /home/reni2/data/REPOS/webkit_sec/Source/WTF/wtf/Assertions.cpp:333 333 *(int *)(uintptr_t)0xbbadbeef = 0; (gdb) ERR<26821>:eet eet_lib.c:668 eet_shutdown() File '/home/reni2/.cache/efreet/icon_themes_reni2.eet' is still open ! bt #0 0x00007ffff5ed9dba in WTFCrash () at /home/reni2/data/REPOS/webkit_sec/Source/WTF/wtf/Assertions.cpp:333 #1 0x00007ffff10e6977 in WebCore::canonicalizeCandidate (candidate=...) at /home/reni2/data/REPOS/webkit_sec/Source/WebCore/editing/VisiblePosition.cpp:496 #2 0x00007ffff10e6b8a in WebCore::VisiblePosition::canonicalPosition (this=0x7fffffffb290, passedPosition=...) at /home/reni2/data/REPOS/webkit_sec/Source/WebCore/editing/VisiblePosition.cpp:533 #3 0x00007ffff10e4a8d in WebCore::VisiblePosition::init (this=0x7fffffffb290, position=..., affinity=WebCore::DOWNSTREAM) at /home/reni2/data/REPOS/webkit_sec/Source/WebCore/editing/VisiblePosition.cpp:58 #4 0x00007ffff10e4a53 in WebCore::VisiblePosition::VisiblePosition (this=0x7fffffffb290, pos=..., affinity=WebCore::DOWNSTREAM) at /home/reni2/data/REPOS/webkit_sec/Source/WebCore/editing/VisiblePosition.cpp:51 #5 0x00007ffff10e8adc in WebCore::VisibleSelection::setBaseAndExtentToDeepEquivalents (this=0x7fffffffb3b0) at /home/reni2/data/REPOS/webkit_sec/Source/WebCore/editing/VisibleSelection.cpp:257 #6 0x00007ffff10e9b57 in WebCore::VisibleSelection::validate (this=0x7fffffffb3b0, granularity=WebCore::CharacterGranularity) at /home/reni2/data/REPOS/webkit_sec/Source/WebCore/editing/VisibleSelection.cpp:427 #7 0x00007ffff10e7cac in WebCore::VisibleSelection::VisibleSelection (this=0x7fffffffb3b0, base=..., extent=..., affinity=WebCore::DOWNSTREAM, isDirectional=false) at /home/reni2/data/REPOS/webkit_sec/Source/WebCore/editing/VisibleSelection.cpp:66 #8 0x00007ffff10e7f15 in WebCore::VisibleSelection::selectionFromContentsOfNode (node=0x9b8ec0) at /home/reni2/data/REPOS/webkit_sec/Source/WebCore/editing/VisibleSelection.cpp:99 #9 0x00007ffff10a9f5c in WebCore::FrameSelection::selectAll (this=0x8a0b40) at /home/reni2/data/REPOS/webkit_sec/Source/WebCore/editing/FrameSelection.cpp:1690 #10 0x00007ffff109d15e in WebCore::executeSelectAll (frame=...) at /home/reni2/data/REPOS/webkit_sec/Source/WebCore/editing/EditorCommand.cpp:1021 #11 0x00007ffff109eb24 in WebCore::Editor::Command::execute (this=0x7fffffffb4b0, parameter=..., triggeringEvent= 0x0) at /home/reni2/data/REPOS/webkit_sec/Source/WebCore/editing/EditorCommand.cpp:1741 #12 0x00007ffff0f58f4c in WebCore::Document::execCommand (this=0x8cfd40, commandName=..., userInterface=true, value=...) at /home/reni2/data/REPOS/webkit_sec/Source/WebCore/dom/Document.cpp:4217 #13 0x00007ffff1f340fb in WebCore::jsDocumentPrototypeFunctionExecCommand (exec=0x7fffffffb5b0) at /home/reni2/data/REPOS/webkit_sec/WebKitBuild/Debug/DerivedSources/WebCore/JSDocument.cpp:4736 #14 0x00007fff9b6ce0b4 in ?? () #15 0x00007fffffffb610 in ?? () #16 0x00007ffff5ec4fb5 in llint_op_call () from /home/reni2/data/REPOS/webkit_sec/WebKitBuild/Debug/lib/libjavascriptcore_efl.so.0 #17 0x0000000000000000 in ?? ()

Attachments
Test case (148 bytes, text/html) 2014-03-27 10:00 PDT, Renata Hodovan	no flags	Details
Test (270 bytes, text/html) 2015-12-10 02:21 PST, Renata Hodovan	no flags	Details
Patch (2.46 KB, patch) 2020-04-15 14:14 PDT, Jack	no flags	Details Formatted Diff Diff
Patch (2.43 KB, patch) 2020-04-15 17:13 PDT, Jack	no flags	Details Formatted Diff Diff
Test result of insert-list-in-table-cell-07.html without patch. (157.32 KB, image/png) 2020-04-15 19:26 PDT, Jack	no flags	Details
Test result of insert-list-in-table-cell-07.html with patch. (146.34 KB, image/png) 2020-04-15 19:27 PDT, Jack	no flags	Details
Patch (6.04 KB, patch) 2020-04-15 20:17 PDT, Jack	no flags	Details Formatted Diff Diff
Patch (6.09 KB, patch) 2020-04-16 06:29 PDT, Jack	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Renata Hodovan

Comment 1 2015-12-10 02:21:46 PST

Created attachment 267083 [details] Test Replacing the old test case with a new one that still works.

Brent Fulgham

Comment 2 2016-08-03 14:07:54 PDT

This reproduces under r204037.

Radar WebKit Bug Importer

Comment 3 2016-08-03 14:10:39 PDT

<rdar://problem/27685444>

Jack

Comment 4 2020-04-15 13:44:58 PDT

The attached test case still reproduces on ToT.

Jack

Comment 5 2020-04-15 14:04:17 PDT

New radar for tracking this issue: <rdar://59535009>

Jack

Comment 6 2020-04-15 14:14:21 PDT

Created attachment 396572 [details] Patch

Jack

Comment 7 2020-04-15 14:16:20 PDT

Comment on attachment 396572 [details] Patch Temporary patch to check if any test fails.

Jack

Comment 8 2020-04-15 17:13:49 PDT

Created attachment 396593 [details] Patch

Jack

Comment 9 2020-04-15 18:51:04 PDT

The failed test "insert-list-in-table-cell-07.html" was added in https://bugs.webkit.org/show_bug.cgi?id=174593.

Geoffrey Garen

Comment 10 2020-04-15 19:10:40 PDT

I think the test difference may be harmless, or a slight improvement. Can you check whether the test looks visually the same if you open it in Safari? (The test content is the table at the top of the page.)

Jack

Comment 11 2020-04-15 19:26:57 PDT

Created attachment 396605 [details] Test result of insert-list-in-table-cell-07.html without patch.

Jack

Comment 12 2020-04-15 19:27:22 PDT

Created attachment 396606 [details] Test result of insert-list-in-table-cell-07.html with patch.

Jack

Comment 13 2020-04-15 19:29:20 PDT

The test results are visually the same.

Jack

Comment 14 2020-04-15 20:03:40 PDT

Root cause: Function PositionIterator::isCandidate() has different implementation comparing to Position::isCandidate(), when the candidate is a br element. The former is called to find candidate in function previousCandidate, and the assertion calls the later function and disqualify the br element as a candidate. Difference: 1. PositionIterator::isCandidate() if (renderer->isBR()) return !m_offsetInAnchor && !Position::nodeIsUserSelectNone(m_anchorNode->parentNode()); 2. Position::isCandidate() if (renderer->isBR()) return !m_offset && m_anchorType != PositionIsAfterAnchor && !nodeIsUserSelectNone(deprecatedNode()->parentNode());

Jack

Comment 15 2020-04-15 20:17:37 PDT

Created attachment 396610 [details] Patch

Geoffrey Garen

Comment 16 2020-04-15 21:54:53 PDT

Comment on attachment 396610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396610&action=review > Source/WebCore/dom/PositionIterator.cpp:152 > + Position candidate = *this; > + return candidate.isCandidate(); I think you can change this function into a one-liner: return Position(*this).isCandidate(); (You can remove the m_anchorNode check too.)

Jack

Comment 17 2020-04-16 06:24:55 PDT

Thanks Geoff. However, Position(*this) would crash if m_anchorNode is null. It the following change okay? return m_anchorNode ? Position(*this).isCandidate() : false; (In reply to Geoffrey Garen from comment #16) > Comment on attachment 396610 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396610&action=review > > > Source/WebCore/dom/PositionIterator.cpp:152 > > + Position candidate = *this; > > + return candidate.isCandidate(); > > I think you can change this function into a one-liner: > > return Position(*this).isCandidate(); > > (You can remove the m_anchorNode check too.)

Jack

Comment 18 2020-04-16 06:29:12 PDT

Created attachment 396640 [details] Patch

Geoffrey Garen

Comment 19 2020-04-16 10:26:37 PDT

> Thanks Geoff. However, Position(*this) would crash if m_anchorNode is null. I see. I misread that. My instinct is that PositionIterator::operator Position() should just check for null and return Position(); but perhaps I've gotten too far afield. This patch seems fine as is.

Geoffrey Garen

Comment 20 2020-04-16 10:26:47 PDT

Comment on attachment 396640 [details] Patch r=me

Jack

Comment 21 2020-04-16 11:38:57 PDT

Thanks. Yeah, I assumed that too. And surprisingly fuzzer hasn't found it. Probably m_anchorNode is always checked before Position() is called? (In reply to Geoffrey Garen from comment #19) > My instinct is that PositionIterator::operator Position() should just check > for null and return Position(); but perhaps I've gotten too far afield. This > patch seems fine as is.

EWS

Comment 22 2020-04-16 11:42:40 PDT

Committed r260207: <https://trac.webkit.org/changeset/260207> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396640 [details].

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