232837 – nullptr deref in CompositeEditCommand::insertNodeAt

RESOLVED FIXED 232837

nullptr deref in CompositeEditCommand::insertNodeAt

https://bugs.webkit.org/show_bug.cgi?id=232837

Summary nullptr deref in CompositeEditCommand::insertNodeAt

Gabriel Nava Marino

Reported 2021-11-08 12:46:53 PST

This seems to occur when p.deprecatedNode() is nullptr.

Attachments
Patch (3.92 KB, patch) 2021-11-08 12:59 PST, Gabriel Nava Marino	no flags	Details Formatted Diff Diff
Patch (3.82 KB, patch) 2021-11-08 14:03 PST, Gabriel Nava Marino	no flags	Details Formatted Diff Diff
Patch (3.75 KB, patch) 2021-11-08 15:49 PST, Gabriel Nava Marino	no flags	Details Formatted Diff Diff
Patch (4.15 KB, patch) 2021-11-10 10:21 PST, Gabriel Nava Marino	no flags	Details Formatted Diff Diff
Patch (4.02 KB, patch) 2021-11-10 16:41 PST, Gabriel Nava Marino	no flags	Details Formatted Diff Diff
Patch (3.91 KB, patch) 2021-11-11 10:31 PST, Gabriel Nava Marino	no flags	Details Formatted Diff Diff
Patch (3.65 KB, patch) 2021-11-11 17:46 PST, Gabriel Nava Marino	no flags	Details Formatted Diff Diff
Patch (3.69 KB, patch) 2021-11-11 17:50 PST, Gabriel Nava Marino	ews-feeder: commit-queue-	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Gabriel Nava Marino

Comment 1 2021-11-08 12:59:13 PST

Created attachment 443593 [details] Patch

Gabriel Nava Marino

Comment 2 2021-11-08 14:03:30 PST

Created attachment 443600 [details] Patch

Wenson Hsieh

Comment 3 2021-11-08 15:08:13 PST

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.

Gabriel Nava Marino

Comment 4 2021-11-08 15:49:25 PST

Created attachment 443625 [details] Patch

Gabriel Nava Marino

Comment 5 2021-11-10 10:21:36 PST

Created attachment 443830 [details] Patch

Wenson Hsieh

Comment 6 2021-11-10 13:16:15 PST

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)

Gabriel Nava Marino

Comment 7 2021-11-10 13:38:44 PST

(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.

Gabriel Nava Marino

Comment 8 2021-11-10 16:41:47 PST

Created attachment 443877 [details] Patch

Gabriel Nava Marino

Comment 9 2021-11-11 10:31:01 PST

Created attachment 443971 [details] Patch

Gabriel Nava Marino

Comment 10 2021-11-11 10:33:06 PST

<rdar://problem/82958441>

Radar WebKit Bug Importer

Comment 11 2021-11-11 10:33:40 PST

<rdar://problem/85305520>

Darin Adler

Comment 12 2021-11-11 17:01:23 PST

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?

Darin Adler

Comment 13 2021-11-11 17:29:34 PST

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?

Darin Adler

Comment 14 2021-11-11 17:30:51 PST

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.

Gabriel Nava Marino

Comment 15 2021-11-11 17:35:45 PST

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.

Gabriel Nava Marino

Comment 16 2021-11-11 17:46:42 PST

Created attachment 444030 [details] Patch

Gabriel Nava Marino

Comment 17 2021-11-11 17:50:39 PST

Created attachment 444031 [details] Patch

Gabriel Nava Marino

Comment 18 2021-11-15 10:49:21 PST

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?

Wenson Hsieh

Comment 19 2021-11-15 10:52:39 PST

Comment on attachment 444031 [details] Patch Indeed, those geolocation tests have been flaky somewhat recently.

EWS

Comment 20 2021-11-15 10:57:06 PST

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].

Rob Buis

Comment 21 2022-06-15 22:55:09 PDT

*** Bug 227389 has been marked as a duplicate of this bug. ***

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component HTML Editing

Assignee

Gabriel Nava Marino

Reported

2021-11-08 12:46 PST

Modified

2022-06-15 22:55 PDT History

CC List

6 users Show

URL

Keywords InRadar

Duplicates (1)

227389 View as bug list

Depends on

Blocks