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_list with a single item that is a text-node containing a single space character.
- 11.2 runs once with empty
sublist and node_list with the single item as mentioned above.
- 11.2.3.2 moves the text-node from
node_list to nodes_to_wrap.
- 11.2.3.3 calls
wrap with the single text-node and returns null because it is not visible.
- 11.2 runs again. At this point both
sublist and node_list are empty. As such, the condition holds and another iteration is executed.
- 11.2.1 tries to get the first member of
node_list which 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())) {
Consider the following testcase:
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:
node_listwith a single item that is a text-node containing a single space character.sublistandnode_listwith the single item as mentioned above.node_listtonodes_to_wrap.wrapwith the single text-node and returnsnullbecause it is not visible.sublistandnode_listare empty. As such, the condition holds and another iteration is executed.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):
I believe the condition in 11.2 should be changed from
to
And indeed, the corresponding code change fixes the crash in Ladybird: