Bug 210246 – Use Node::length to replace Node::maxCharacterOffset and lastOffsetInNode; switch more offsets from int to unsigned

• Component: DOM
• Status: RESOLVED FIXED

[Darin Adler]

Use Node::length to replace Node::maxCharacterOffset and lastOffsetInNode; switch more offsets from int to unsigned

[Darin Adler]

Created attachment 395908 [details]
Patch

[Darin Adler]

While working on reducing the use of live ranges, discovered this improvement, so getting it reviewed and landed separately.

[Darin Adler]

Created attachment 395910 [details]
Patch

[Antti Koivisto]

Looks like there is a behavior change here. Tests show failures but also new PASSes in WPT.

[Darin Adler]

Looks like behavior change is in the text input selection part of this; I think it’s a change we will want, but I will separate that out into a much smaller patch.

[Darin Adler]

Created attachment 395982 [details]
Patch

[Darin Adler]

This newer patch should leave out the behavior change, which we can consider and do later/separately.

[Darin Adler]

Oops, looks like the behavior change is still there.

[Darin Adler]

Some behavior change.

[Darin Adler]

Created attachment 396008 [details]
Patch

[Darin Adler]

This last tiny layout change difference is a mystery.

[Darin Adler]

Created attachment 396136 [details]
Patch

[Darin Adler]

Mystery solved. Finally had a chance to debug and it was a refactoring error; fixed now.

[Darin Adler]

This patch is now both a simple refactoring, and all tests are passing, so should be a quick review for someone.

[Darin Adler]

Committed r259930: <https://trac.webkit.org/changeset/259930>

[Radar WebKit Bug Importer]

<rdar://problem/61638646>

[Daniel Bates]

Comment on attachment 396136 [details]
Patch

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

> Source/WebCore/dom/DocumentMarkerController.cpp:600
> +        if (static_cast<unsigned>(targetStartOffset) >= node.length() || targetEndOffset <= 0) {

This is a change in behavior that causes a regression: marker at the start of the text will **now be removed** if trailing whitespace was subsequently inserted and that whitespace is collapsed. When collapse happens, marker.startOffset() == 0, **delta** == -1. So, targetStartOffset == -1 and this blows up when casted to unsigned (i.e. it ALWAYS is >= node.length()).

Just for me: This iOS code was added to fix <rdar://problem/11478253>.

> Source/WebCore/dom/DocumentMarkerController.cpp:615
>                  list->remove(i);

Just for me: ^^^ line above (not part of this patch) is incorrect: performs **unsigned** comparison, but delta is signed so "+ delta" + implicit conversion to unsigned can blow up and make comparison meaningless. Also expression should reference targetEndOffset instead of re-computing <-- may have made the bug more obvious.

[Darin Adler]

Comment on attachment 396136 [details]
Patch

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

>> Source/WebCore/dom/DocumentMarkerController.cpp:600
>> +        if (static_cast<unsigned>(targetStartOffset) >= node.length() || targetEndOffset <= 0) {
> 
> This is a change in behavior that causes a regression: marker at the start of the text will **now be removed** if trailing whitespace was subsequently inserted and that whitespace is collapsed. When collapse happens, marker.startOffset() == 0, **delta** == -1. So, targetStartOffset == -1 and this blows up when casted to unsigned (i.e. it ALWAYS is >= node.length()).
> 
> Just for me: This iOS code was added to fix <rdar://problem/11478253>.

Are you fixing it, or would you like me to?

Do you have an idea how to add a test?

[Daniel Bates]

Comment on attachment 396136 [details]
Patch

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

>>> Source/WebCore/dom/DocumentMarkerController.cpp:600
>>> +        if (static_cast<unsigned>(targetStartOffset) >= node.length() || targetEndOffset <= 0) {
>> 
>> This is a change in behavior that causes a regression: marker at the start of the text will **now be removed** if trailing whitespace was subsequently inserted and that whitespace is collapsed. When collapse happens, marker.startOffset() == 0, **delta** == -1. So, targetStartOffset == -1 and this blows up when casted to unsigned (i.e. it ALWAYS is >= node.length()).
>> 
>> Just for me: This iOS code was added to fix <rdar://problem/11478253>.
> 
> Are you fixing it, or would you like me to?
> 
> Do you have an idea how to add a test?

You can fix it, but I will supply you with a bug + a test in a few minutes...

[Daniel Bates]

Comment on attachment 396136 [details]
Patch

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

>>>> Source/WebCore/dom/DocumentMarkerController.cpp:600
>>>> +        if (static_cast<unsigned>(targetStartOffset) >= node.length() || targetEndOffset <= 0) {
>>> 
>>> This is a change in behavior that causes a regression: marker at the start of the text will **now be removed** if trailing whitespace was subsequently inserted and that whitespace is collapsed. When collapse happens, marker.startOffset() == 0, **delta** == -1. So, targetStartOffset == -1 and this blows up when casted to unsigned (i.e. it ALWAYS is >= node.length()).
>>> 
>>> Just for me: This iOS code was added to fix <rdar://problem/11478253>.
>> 
>> Are you fixing it, or would you like me to?
>> 
>> Do you have an idea how to add a test?
> 
> You can fix it, but I will supply you with a bug + a test in a few minutes...

Bug + test at posted at bug #212093.

[Darin Adler]

Comment on attachment 396136 [details]
Patch

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

>> Source/WebCore/dom/DocumentMarkerController.cpp:615
>>                  list->remove(i);
> 
> Just for me: ^^^ line above (not part of this patch) is incorrect: performs **unsigned** comparison, but delta is signed so "+ delta" + implicit conversion to unsigned can blow up and make comparison meaningless. Also expression should reference targetEndOffset instead of re-computing <-- may have made the bug more obvious.

That’s incorrect.

If targetEndOffset is <= 0 we won’t get here; checked for above.