This seems to occur when p.deprecatedNode() is nullptr.
Created attachment 443593 [details] Patch
Created attachment 443600 [details] Patch
Comment on attachment 443600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443600&action=review > LayoutTests/fast/editing/editing-position-crash.html:1 > +<!DOCTYPE html> The rest of the change seems fine, but this test case looks like it could use some cleanup. A few confusing/odd details: - The html closing tag on line 2 - The use of try-catch in the event handlers (as well as the names of all the event handler functions) - The text node after the end of the body element. - Lack of explicit closing tags on a few of the elements below, such as the svg.
Created attachment 443625 [details] Patch
Created attachment 443830 [details] Patch
Comment on attachment 443830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443830&action=review It looks like the new test is crashing underneath: ``` ASSERTION FAILED: ancestor ./dom/Position.cpp(1586) : WebCore::Position WebCore::positionInParentBeforeNode(WebCore::Node *) 1 0x13a6f4928 WTFCrash 2 0x118de87a0 WebCore::CachedResourceHandleBase::operator!() const 3 0x119784e14 WebCore::positionInParentBeforeNode(WebCore::Node*) 4 0x1198b0e10 WebCore::CreateLinkCommand::doApply() 5 0x1198951d8 WebCore::CompositeEditCommand::apply() 6 0x119929a70 WebCore::executeCreateLink(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) 7 0x119900498 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const 8 0x1195b8bd4 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) 9 0x116b7e268 WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*) 10 0x116b7dd10 long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) ``` > LayoutTests/ChangeLog:6 > + Reviewed by Reviewed by Wenson Hsieh. Since I haven't reviewed (added r+ to) this patch, this should say `Reviewed by NOBODY (OOPS!)`. (But also, there is a redundant "Reviewed by" here)
(In reply to Wenson Hsieh from comment #6) > Comment on attachment 443830 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=443830&action=review > > It looks like the new test is crashing underneath: I think this is likely pointing at the root cause being upstream. I am taking a look. Thank you for pointing this out. > > ``` > ASSERTION FAILED: ancestor > ./dom/Position.cpp(1586) : WebCore::Position > WebCore::positionInParentBeforeNode(WebCore::Node *) > 1 0x13a6f4928 WTFCrash > 2 0x118de87a0 WebCore::CachedResourceHandleBase::operator!() const > 3 0x119784e14 WebCore::positionInParentBeforeNode(WebCore::Node*) > 4 0x1198b0e10 WebCore::CreateLinkCommand::doApply() > 5 0x1198951d8 WebCore::CompositeEditCommand::apply() > 6 0x119929a70 WebCore::executeCreateLink(WebCore::Frame&, WebCore::Event*, > WebCore::EditorCommandSource, WTF::String const&) > 7 0x119900498 WebCore::Editor::Command::execute(WTF::String const&, > WebCore::Event*) const > 8 0x1195b8bd4 WebCore::Document::execCommand(WTF::String const&, bool, > WTF::String const&) > 9 0x116b7e268 > WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, > JSC::CallFrame*, WebCore::JSDocument*) > 10 0x116b7dd10 long long > WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore:: > jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, > JSC::CallFrame*, WebCore::JSDocument*)), > (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, > char const*) > ``` > > > LayoutTests/ChangeLog:6 > > + Reviewed by Reviewed by Wenson Hsieh. > > Since I haven't reviewed (added r+ to) this patch, this should say `Reviewed > by NOBODY (OOPS!)`. Thank you for this clarification. I will add this Reviewed by after receiving an r+. > > (But also, there is a redundant "Reviewed by" here) Thank you for pointing this out.
Created attachment 443877 [details] Patch
Created attachment 443971 [details] Patch
<rdar://problem/82958441>
<rdar://problem/85305520>
Comment on attachment 443971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443971&action=review > LayoutTests/fast/editing/editing-position-crash-expected.txt:1 > +CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded. Is there a way to make this test not depend on our recursion limit, but still cover this crash?
Comment on attachment 443971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443971&action=review > Source/WebCore/editing/CreateLinkCommand.cpp:51 > + else if (!endingSelection().isOrphan()) { Shouldn’t we check if it’s an orphan up at the top of the function, rather than only after the isRange check?
Comment on attachment 443971 [details] Patch This patch is OK, but I think it could be made a little better by making the test not depend on a "call stack size exceeded" exception and by doing the orphan check earlier. But neither of those seems a reason to not land it ... just that each of those things would be a significant improvement.
Comment on attachment 443971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443971&action=review >> Source/WebCore/editing/CreateLinkCommand.cpp:51 >> + else if (!endingSelection().isOrphan()) { > > Shouldn’t we check if it’s an orphan up at the top of the function, rather than only after the isRange check? Thank you for the feedback! Yes, I will change the endingSelection().isNone check above to endingSelection().isNoneOrOrphaned(). >> LayoutTests/fast/editing/editing-position-crash-expected.txt:1 >> +CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded. > > Is there a way to make this test not depend on our recursion limit, but still cover this crash? Yes, I will pass an option so the event listener is only invoked at most once.
Created attachment 444030 [details] Patch
Created attachment 444031 [details] Patch
Comment on attachment 444031 [details] Patch It looks like the unrelated layout test failure it is hitting was addressed in Bug 233043 - Regression(r285639) fast/dom/Geolocation/cached-position-iframe.html is frequently crashing on Mac-wk1 However, the bot is not yet running with that revision. Moving to cq?
Comment on attachment 444031 [details] Patch Indeed, those geolocation tests have been flaky somewhat recently.
Committed r285813 (244257@main): <https://commits.webkit.org/244257@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 444031 [details].
*** Bug 227389 has been marked as a duplicate of this bug. ***