[execCommand] Incorrect condition in 9.9 toggle-lists #521
Description
Consider the following testcase:
<!DOCTYPE html>
<div contenteditable=""> </div>
<script>
const div = document.querySelector("[contenteditable]");
const range = document.createRange();
range.setStart(div, 0);
range.setEnd(div, 1);
document.getSelection().addRange(range);
document.execCommand("insertorderedlist", false, "");
</script>This scenario currently causes a crash in Ladybird (see LadybirdBrowser/ladybird#7291)
From what I can tell the following happens in toggle-lists for this case:
- Step 8 returns
node_listwith a single item that is a text-node containing a single space character. - 11.2 runs once with empty
sublistandnode_listwith the single item as mentioned above. - 11.2.3.2 moves the text-node from
node_listtonodes_to_wrap. - 11.2.3.3 calls
wrapwith the single text-node and returnsnullbecause it is not visible. - 11.2 runs again. At this point both
sublistandnode_listare empty. As such, the condition holds and another iteration is executed. - 11.2.1 tries to get the first member of
node_listwhich is not possible because the list is empty.
Ladybird implements this closely to the wording of the spec text and crashes on 11.2.1 in the second iteration (some debug prints + start of the stack trace):
Initial node list (1 items):
#text
Value: ' '
------------------------
Node list in iteration (1 items):
#text
Value: ' '
Sublist in iteration (0 items):
Not wrapping because not visible and no br
------------------------
Node list in iteration (0 items):
Sublist in iteration (0 items):
VERIFICATION FAILED: i < m_size at AK/Vector.h:172
#0 (inlined) in AK::Vector<GC::Ref<Web::DOM::Node>, 0ul, (AK::FastLastAccess)0>::at(unsigned long) at AK/Vector.h:172:9
#1 0x00007b138cff8f21 in AK::Vector<GC::Ref<Web::DOM::Node>, 0ul, (AK::FastLastAccess)0>::first() at AK/Vector.h:197:35
#2 0x00007b138d04acf9 in Web::Editing::toggle_lists(Web::DOM::Document&, AK::FlyString const&) at Libraries/LibWeb/Editing/Internal/Algorithms.cpp:4221:63
#3 0x00007b138cfe027f in Web::Editing::command_insert_ordered_list_action(Web::DOM::Document&, AK::Utf16String const&) at Libraries/LibWeb/Editing/Commands.cpp:1421:5
[...]
I believe the condition in 11.2 should be changed from
While either sublist is empty, or node list is not empty and its first member is the nextSibling of sublist's last member:
to
While node list is not empty and either sublist is empty, or node list's first member is the nextSibling of sublist's last member:
And indeed, the corresponding code change fixes the crash in Ladybird:
diff --git a/Libraries/LibWeb/Editing/Internal/Algorithms.cpp b/Libraries/LibWeb/Editing/Internal/Algorithms.cpp
index dcec6c19f7b..fd14c0af8f1 100644
--- a/Libraries/LibWeb/Editing/Internal/Algorithms.cpp
+++ b/Libraries/LibWeb/Editing/Internal/Algorithms.cpp
@@ -4201,7 +4201,7 @@ void toggle_lists(DOM::Document& document, FlyString const& tag_name)
// 2. While either sublist is empty, or node list is not empty and its first member is the nextSibling of
// sublist's last member:
- while (sublist.is_empty() || (!node_list.is_empty() && node_list.first().ptr() == sublist.last()->next_sibling())) {
+ while (!node_list.is_empty() && (sublist.is_empty() || node_list.first().ptr() == sublist.last()->next_sibling())) {
// 1. If node list's first member is a p or div, set the tag name of node list's first member to "li",
// and append the result to sublist. Remove the first member from node list.
if (is<HTML::HTMLParagraphElement>(*node_list.first()) || is<HTML::HTMLDivElement>(*node_list.first())) {