| 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: | P2 | Keywords: | InRadar | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||
| Bug Blocks: | 116980 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
Created attachment 267083 [details]
Test
Replacing the old test case with a new one that still works.
The attached test case still reproduces on ToT. New radar for tracking this issue: <rdar://59535009> Created attachment 396572 [details]
Patch
Comment on attachment 396572 [details]
Patch
Temporary patch to check if any test fails.
Created attachment 396593 [details]
Patch
The failed test "insert-list-in-table-cell-07.html" was added in https://bugs.webkit.org/show_bug.cgi?id=174593. 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.) Created attachment 396605 [details]
Test result of insert-list-in-table-cell-07.html without patch.
Created attachment 396606 [details]
Test result of insert-list-in-table-cell-07.html with patch.
The test results are visually the same. 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());
Created attachment 396610 [details]
Patch
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.) 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.) Created attachment 396640 [details]
Patch
> 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.
Comment on attachment 396640 [details]
Patch
r=me
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. Committed r260207: <https://trac.webkit.org/changeset/260207> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396640 [details]. |
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 ?? ()