Bug 75171 – NULL ptr in WebCore::TextIterator::rangeFromLocationAndLength

• Component: HTML Editing
• Status: RESOLVED FIXED

[Berend-Jan Wever]

Chromium: http://code.google.com/p/chromium/issues/detail?id=108547

- crash stack -
  WebCore::TextIterator::rangeFromLocationAndLength
  WebCore::visiblePositionForIndex
  WebCore::ApplyBlockElementCommand::doApply

- repro -
text
<script>
  document.execCommand("selectall");
  document.designMode="on";
  document.execCommand("Delete");
  document.execCommand("undo");
</script>
<style>
  *{-webkit-user-select:none}
</style>
<script>
  document.execCommand("Outdent")
</script>

[Pablo Flouret]

Created attachment 121168 [details]
proposed patch

[Ryosuke Niwa]

Comment on attachment 121168 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121168&action=review

> Source/WebCore/editing/ApplyBlockElementCommand.cpp:84
> +    // startOfSelection and endOfSelection can be null when
> +    // "-webkit-user-select: none" style attribute is specified.
> +    if (startOfSelection.isNull() || endOfSelection.isNull())

You should just call isNonOrphanedCaretOrRange on selection.

[Pablo Flouret]

(In reply to comment #2)
> (From update of attachment 121168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121168&action=review
> 
> > Source/WebCore/editing/ApplyBlockElementCommand.cpp:84
> > +    // startOfSelection and endOfSelection can be null when
> > +    // "-webkit-user-select: none" style attribute is specified.
> > +    if (startOfSelection.isNull() || endOfSelection.isNull())
> 
> You should just call isNonOrphanedCaretOrRange on selection.

That's called at the start of the function and it returns true. If i understand things correctly, it's not orphaned, it's just that there's a single element but it's not selectable due to the -webkit-user-select css declaration.

A similar bug is https://bugs.webkit.org/show_bug.cgi?id=26214

[Ryosuke Niwa]

Comment on attachment 121168 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121168&action=review

>>> Source/WebCore/editing/ApplyBlockElementCommand.cpp:84
>>> +    if (startOfSelection.isNull() || endOfSelection.isNull())
>> 
>> You should just call isNonOrphanedCaretOrRange on selection.
> 
> That's called at the start of the function and it returns true. If i understand things correctly, it's not orphaned, it's just that there's a single element but it's not selectable due to the -webkit-user-select css declaration.
> 
> A similar bug is https://bugs.webkit.org/show_bug.cgi?id=26214

selectionForParagraphIteration is probably returning a null selection. isNonOrphanedCaretOrRange checks nullness as well as orphanedness.

[Pablo Flouret]

(In reply to comment #4)
> (From update of attachment 121168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121168&action=review
> 
> >>> Source/WebCore/editing/ApplyBlockElementCommand.cpp:84
> >>> +    if (startOfSelection.isNull() || endOfSelection.isNull())
> >> 
> >> You should just call isNonOrphanedCaretOrRange on selection.
> > 
> > That's called at the start of the function and it returns true. If i understand things correctly, it's not orphaned, it's just that there's a single element but it's not selectable due to the -webkit-user-select css declaration.
> > 
> > A similar bug is https://bugs.webkit.org/show_bug.cgi?id=26214
> 
> selectionForParagraphIteration is probably returning a null selection. isNonOrphanedCaretOrRange checks nullness as well as orphanedness.

selectionForParagraphIteration seems to deal with tables, and there are none here, i checked the value of selection.isNonOrphanedCaretOrRange() in the debugger anyway and it's true at that point also.

The selection is not null itself, selection.visible{Start,End} are the ones returning null positions due to no available selectable elements (from my guess given above).

[Ryosuke Niwa]

(In reply to comment #5)
> selectionForParagraphIteration seems to deal with tables, and there are none here, i checked the value of selection.isNonOrphanedCaretOrRange() in the debugger anyway and it's true at that point also.
> 
> The selection is not null itself, selection.visible{Start,End} are the ones returning null positions due to no available selectable elements (from my guess given above).

That seems like a bug to me. visibleStart / visibleEnd shouldn't be returning null when VisibleSelection isn't null.

[Pablo Flouret]

(In reply to comment #6)
> (In reply to comment #5)
> > selectionForParagraphIteration seems to deal with tables, and there are none here, i checked the value of selection.isNonOrphanedCaretOrRange() in the debugger anyway and it's true at that point also.
> > 
> > The selection is not null itself, selection.visible{Start,End} are the ones returning null positions due to no available selectable elements (from my guess given above).
> 
> That seems like a bug to me. visibleStart / visibleEnd shouldn't be returning null when VisibleSelection isn't null.

Hmm, maybe the issue is that when -webkit-user-select is changed the selection should be re-validated or something?

[Berend-Jan Wever]

Here's a completely different repro for the same crash (AFAIK):

<script>
  window.onload = function() {
    document.execCommand("selectall");
    document.designMode="on";
    document.execCommand("indent");
    document.execCommand("outdent");
  };
</script>
<frameset>
  <frame></frame>
</frameset>

[Ryosuke Niwa]

Comment on attachment 121168 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121168&action=review

>>>>> Source/WebCore/editing/ApplyBlockElementCommand.cpp:84
>>>>> +    if (startOfSelection.isNull() || endOfSelection.isNull())
>>>> 
>>>> You should just call isNonOrphanedCaretOrRange on selection.
>>> 
>>> That's called at the start of the function and it returns true. If i understand things correctly, it's not orphaned, it's just that there's a single element but it's not selectable due to the -webkit-user-select css declaration.
>>> 
>>> A similar bug is https://bugs.webkit.org/show_bug.cgi?id=26214
>> 
>> selectionForParagraphIteration is probably returning a null selection. isNonOrphanedCaretOrRange checks nullness as well as orphanedness.
> 
> selectionForParagraphIteration seems to deal with tables, and there are none here, i checked the value of selection.isNonOrphanedCaretOrRange() in the debugger anyway and it's true at that point also.
> 
> The selection is not null itself, selection.visible{Start,End} are the ones returning null positions due to no available selectable elements (from my guess given above).

That seems to indicate we have a bug. visibleStart/visibleEnd should null iff selection is null. r- because this isn't the right fix.

[Ryosuke Niwa]

This bug has been fixed by http://trac.webkit.org/changeset/115461. I've added a detailed explanation as to how this bug reproduces and why the change was needed.