In some instances, the startNode might not have any siblings.
<rdar://problem/86567729>
Created attachment 448336 [details] Patch
Created attachment 448350 [details] Patch
Created attachment 448351 [details] Patch
Created attachment 448355 [details] Patch
Removed dependence on the webkitSpeechRecognition API in the test that was added, since it is not needed to reproduce the issue.
Created attachment 448507 [details] Patch
Comment on attachment 448507 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448507&action=review > Source/WebCore/editing/InsertListCommand.cpp:-203 > + auto range = endingSelection().firstRange(); > + doApplyForSingleParagraph(false, listTag, *range); > } > - > - auto range = endingSelection().firstRange(); > - doApplyForSingleParagraph(false, listTag, *range); Looks like a good fix. > Source/WebCore/editing/ModifySelectionListLevel.cpp:97 > while (1) { > + ASSERT(node); Here I would do something other than adding this assertion. I see no harm in having these functions stop iterating if the node is null. Does not seem critical to crash rather than returning in that case. It’s fine to assert so we can catch mistakes, but also just might want to change this to be: while (node) { instead of while (1). I don’t see how we have a guarantee that we’ll always reach endNode before null, especially if any mutation is happening during the loops. I also think these functions should be using RefPtr for the nodes, not raw pointers. This code is written as if there was a guarantee that removeNode and insertNodeAfter have no side effects, and there’s no reason to have such a risky approach to object lifetimes. Like this: RefPtr node = startNode; RefPtr next = node->nextSibling(); We could also use "return" rather than "break" to end the loop so we can put an "assert not reached" after the loop. All of this applies to the other functions too.
(In reply to Darin Adler from comment #8) > Comment on attachment 448507 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448507&action=review > > > Source/WebCore/editing/InsertListCommand.cpp:-203 > > + auto range = endingSelection().firstRange(); > > + doApplyForSingleParagraph(false, listTag, *range); > > } > > - > > - auto range = endingSelection().firstRange(); > > - doApplyForSingleParagraph(false, listTag, *range); > > Looks like a good fix. > > > Source/WebCore/editing/ModifySelectionListLevel.cpp:97 > > while (1) { > > + ASSERT(node); > > Here I would do something other than adding this assertion. > > I see no harm in having these functions stop iterating if the node is null. > Does not seem critical to crash rather than returning in that case. It’s > fine to assert so we can catch mistakes, but also just might want to change > this to be: > > while (node) { > > instead of while (1). I don’t see how we have a guarantee that we’ll always > reach endNode before null, especially if any mutation is happening during > the loops. > > I also think these functions should be using RefPtr for the nodes, not raw > pointers. This code is written as if there was a guarantee that removeNode > and insertNodeAfter have no side effects, and there’s no reason to have such > a risky approach to object lifetimes. Like this: > > RefPtr node = startNode; > RefPtr next = node->nextSibling(); > > We could also use "return" rather than "break" to end the loop so we can put > an "assert not reached" after the loop. > > All of this applies to the other functions too. Thank you for the feedback. I have applied all of these changes locally and will include them in the updated patch. I am investigating EWS failures related to the proposed changes in InsertListCommand::doApply().
Created attachment 448551 [details] Patch
Created attachment 448558 [details] Patch
Comment on attachment 448558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448558&action=review > Source/WebCore/editing/VisibleUnits.cpp:1199 > + if (r->isBR() || is<HTMLBRElement>(n) || isBlock(n)) Is there a correctness implication here? Can a <br> element be styled so it is inline and not a block? Just want to be cautious about testing only the crashing aspect of this.
Committed r287812 (245864@main): <https://commits.webkit.org/245864@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 448558 [details].
(In reply to Darin Adler from comment #12) > Comment on attachment 448558 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448558&action=review > > > Source/WebCore/editing/VisibleUnits.cpp:1199 > > + if (r->isBR() || is<HTMLBRElement>(n) || isBlock(n)) > > Is there a correctness implication here? Can a <br> element be styled so it > is inline and not a block? Just want to be cautious about testing only the > crashing aspect of this. Thank you for pointing this out. Upon calling 'CompositeEditCommand::moveParagraph' I noticed the node's RenderObject bitfield m_isLineBreak changed from a 1 to a 0, as well as other flags, hence the need for is<HTMLBRElement>(n), as r->isBR() will return false. It appears there is some style state change going on here which might also impact isBlock(n). I will check whether this state change is an invalid state.
(In reply to Darin Adler from comment #12) > Comment on attachment 448558 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448558&action=review > > > Source/WebCore/editing/VisibleUnits.cpp:1199 > > + if (r->isBR() || is<HTMLBRElement>(n) || isBlock(n)) > > Is there a correctness implication here? Can a <br> element be styled so it > is inline and not a block? Just want to be cautious about testing only the > crashing aspect of this. I received some further clarification: <br> element is by default inline.