Bug 90560 – SurroundingText should not advance character iterators if they are at end

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Leandro Graciá Gil]

Both the CharacterIterator and BackwardsCharacterIterator classes try to advance their internal iterator before actually checking if this iterator is at its end position. If this is the case TextIterator::advance might try to dereference a null m_node and crash. The character iterator classes should make sure they never try to advance when already at end.

[Leandro Graciá Gil]

Created attachment 150813 [details]
Patch

[Alexey Proskuryakov]

How did you measure performance impact?

[Ryosuke Niwa]

Comment on attachment 150813 [details]
Patch

I don't think we should be making this change. If anything, we should be asserting that !atEnd() in these two functions. What we should be fixing is the caller of these functions.

[Leandro Graciá Gil]

(In reply to comment #3)
> (From update of attachment 150813 [details])
> I don't think we should be making this change. If anything, we should be asserting that !atEnd() in these two functions. What we should be fixing is the caller of these functions.

Good point. I'll be updating the patch to do that.

[Leandro Graciá Gil]

Created attachment 150938 [details]
Patch

[Leandro Graciá Gil]

(In reply to comment #2)
> How did you measure performance impact?

No longer making the check in the character iterators for release builds. Checking in SurroundingText which is out of the critical path.

[Leandro Graciá Gil]

Comment on attachment 150938 [details]
Patch

Thanks for the review.

[WebKit Review Bot]

Comment on attachment 150938 [details]
Patch

Clearing flags on attachment: 150938

Committed r121921: <http://trac.webkit.org/changeset/121921>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 90648

[Ryosuke Niwa]

Comment on attachment 150938 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:1410
> +    ASSERT(!atEnd());
> +

We need to get rid of these assertions or fix callers.

[Leandro Graciá Gil]

Created attachment 151045 [details]
Patch

[Leandro Graciá Gil]

Comment on attachment 151045 [details]
Patch

Assertions moved out of character iterators to SurroundingText. Also adding a few more checks to properly handle any possible null ranges that might come from this case.

[Ryosuke Niwa]

(In reply to comment #13)
> (From update of attachment 151045 [details])
> Assertions moved out of character iterators to SurroundingText. Also adding a few more checks to properly handle any possible null ranges that might come from this case.

We need add tests for these new checks.

[Leandro Graciá Gil]

Created attachment 151283 [details]
Patch

[Leandro Graciá Gil]

Comment on attachment 151283 [details]
Patch

Moving the null visible position check to the WebCore side as discussed in IRC.

[WebKit Review Bot]

Comment on attachment 151283 [details]
Patch

Clearing flags on attachment: 151283

Committed r122139: <http://trac.webkit.org/changeset/122139>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.