REGRESSION (r165385): Crash when applying autocorrection exceeds maximum text area length.

[Gavin Sherlock]

Created attachment 240162 [details]
crash log

Note - this is Safari 8, on OS X 10.10, but I can't actually select those from the drop downs.

Steps to reproduce:

1.  Go to Apple's feedback page
2.  Write a rant about how you hate the new behavior of the zoom buttons on Finder windows, where the last word is intended to be "awful", but only "awfu" fits.
3.  When Safari suggests "awful" as the completion, select it

Expected results:

Either "awful" is added, or it leaves it as "awfu"

Actual results:

Boom - tab crashes, losing you everything you just typed in. :-/


Crash report attached

[Alexey Proskuryakov]

Tracked internally as rdar://problem/18568864 and related bugs.

[Enrica Casucci]

Created attachment 243466 [details]
Patch

[Darin Adler]

Comment on attachment 243466 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:1098
> -    ASSERT(!atEnd());
> +    if (atEnd())
> +        return nullptr;

It’s the caller’s responsibility to check atEnd(). This is a design change. I don’t think we should change the design of TextIterator. Instead we should fix the caller that is not checking atEnd(). In fact, I think this function should return Ref<Range> and not PassRefPtr<Range> in the future.

> Source/WebCore/editing/TextIterator.cpp:1389
> -    ASSERT(!atEnd());
> +    if (atEnd())
> +        return nullptr;

Ditto.

> Source/WebCore/editing/TextIterator.cpp:1473
> +    if (!start)
> +        return nullptr;

This should be if (it.atEnd()) return nullptr; *before* calling it.range().

> Source/WebCore/editing/TextIterator.cpp:1479
> +    if (!end)
> +        return nullptr;

Ditto.

[Darin Adler]

Comment on attachment 243466 [details]
Patch

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

>> Source/WebCore/editing/TextIterator.cpp:1473
>> +        return nullptr;
> 
> This should be if (it.atEnd()) return nullptr; *before* calling it.range().

I think we might also need to check if (it.atEnd()) before calling it.advance()?

[Darin Adler]

Comment on attachment 243466 [details]
Patch

I’d also like to see a regression test. Any reason we can’t make one?

[Enrica Casucci]

(In reply to comment #3)
> Comment on attachment 243466 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243466&action=review
> 
> > Source/WebCore/editing/TextIterator.cpp:1098
> > -    ASSERT(!atEnd());
> > +    if (atEnd())
> > +        return nullptr;
> 
> It’s the caller’s responsibility to check atEnd(). This is a design change.
> I don’t think we should change the design of TextIterator. Instead we should
> fix the caller that is not checking atEnd(). In fact, I think this function
> should return Ref<Range> and not PassRefPtr<Range> in the future.

Ok, I understand now. I'll change it.
> 
> > Source/WebCore/editing/TextIterator.cpp:1389
> > -    ASSERT(!atEnd());
> > +    if (atEnd())
> > +        return nullptr;
> 
> Ditto.
> 
> > Source/WebCore/editing/TextIterator.cpp:1473
> > +    if (!start)
> > +        return nullptr;
> 
> This should be if (it.atEnd()) return nullptr; *before* calling it.range().
> 
> > Source/WebCore/editing/TextIterator.cpp:1479
> > +    if (!end)
> > +        return nullptr;
> 
> Ditto.

[Enrica Casucci]

(In reply to comment #5)
> Comment on attachment 243466 [details]
> Patch
> 
> I’d also like to see a regression test. Any reason we can’t make one?

I think it is possible, testing the autocorrection case at the end of the field.

[Enrica Casucci]

Created attachment 244475 [details]
New patch

[WebKit Commit Bot]

Attachment 244475 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/Internals.h:161:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 244475 [details]
New patch

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

> Source/WebCore/editing/TextIterator.cpp:2391
> +    return characterSubrange(entireRange->startContainer()->document(), entireRangeIterator, characterOffset, characterCount);

What guarantees startContainer is non-null? If callers passed a detached range, then that might be null. I suggest using entireRange->ownerDocument() instead.

> Source/WebCore/editing/TextIterator.cpp:2611
> +    return characterSubrange(range.startContainer()->document(), computeRangeIterator, matchStart, matchLength);

Ditto.

[Enrica Casucci]

(In reply to comment #10)
> What guarantees startContainer is non-null? If callers passed a detached
> range, then that might be null. I suggest using entireRange->ownerDocument()
> instead.

That is a valid point. I'll change it before landing the patch. Thanks for the review.

[Enrica Casucci]

Committed revision 178462.

[Brent Fulgham]

This broke the Windows build, because it started using 'TextIterator::subrange', which is not exported by WebKitExports.def.

I'll land a fix shortly.

[Brent Fulgham]

Windows build fix committed in r178470.