Bug 133592 - Use unsigneds instead of ints for indexes into a string in text layout code
Summary: Use unsigneds instead of ints for indexes into a string in text layout code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 133594 (view as bug list)
Depends on: 134174 134250
Blocks:
  Show dependency treegraph
 
Reported: 2014-06-06 17:01 PDT by Myles C. Maxfield
Modified: 2014-06-24 14:05 PDT (History)
14 users (show)

See Also:


Attachments
Patch (61.60 KB, patch)
2014-06-16 20:13 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (89.10 KB, patch)
2014-06-17 10:21 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (89.56 KB, patch)
2014-06-17 12:48 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (89.59 KB, patch)
2014-06-20 16:12 PDT, Myles C. Maxfield
zalan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2014-06-06 17:01:01 PDT
It would make more sense for it to return an unsigned.
Comment 1 Myles C. Maxfield 2014-06-16 20:13:34 PDT
Created attachment 233210 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2014-06-16 20:14:10 PDT
<rdar://problem/17337617>
Comment 3 Myles C. Maxfield 2014-06-17 10:21:40 PDT
Created attachment 233240 [details]
Patch
Comment 4 WebKit Commit Bot 2014-06-17 10:24:10 PDT
Attachment 233240 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:106:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Myles C. Maxfield 2014-06-17 10:40:29 PDT
*** Bug 133594 has been marked as a duplicate of this bug. ***
Comment 6 Brent Fulgham 2014-06-17 12:13:38 PDT
Comment on attachment 233240 [details]
Patch

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

> Source/WebCore/platform/graphics/Font.cpp:340
> +    unsigned to = (initialTo == -1 ? run.length() : initialTo);

Can initialTo ever be -2 (or lower)? Should this be "initialTo < 0 ?", as it is in 'drawEmphasisMarks'?

> Source/WebCore/platform/graphics/Font.cpp:511
> +    unsigned to = (initialTo == -1 ? run.length() : initialTo);

Ditto above question.

> Source/WebCore/platform/graphics/Font.cpp:1068
> +    unsigned offsetInString = static_cast<unsigned>(initialOffsetInString);

What if initialOffsetInString is negative?
Comment 7 Myles C. Maxfield 2014-06-17 12:36:21 PDT
Comment on attachment 233240 [details]
Patch

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

>> Source/WebCore/platform/graphics/Font.cpp:340
>> +    unsigned to = (initialTo == -1 ? run.length() : initialTo);
> 
> Can initialTo ever be -2 (or lower)? Should this be "initialTo < 0 ?", as it is in 'drawEmphasisMarks'?

It shouldn't be, but it's simple enough to change the conditional.

>> Source/WebCore/platform/graphics/Font.cpp:511
>> +    unsigned to = (initialTo == -1 ? run.length() : initialTo);
> 
> Ditto above question.

Same answer.

>> Source/WebCore/platform/graphics/Font.cpp:1068
>> +    unsigned offsetInString = static_cast<unsigned>(initialOffsetInString);
> 
> What if initialOffsetInString is negative?

It should never be - the only negative value that it should ever have is -1 (kNoOffset). I'll add an assert for this.
Comment 8 Myles C. Maxfield 2014-06-17 12:48:22 PDT
Created attachment 233253 [details]
Patch
Comment 9 WebKit Commit Bot 2014-06-17 12:50:26 PDT
Attachment 233253 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:106:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Myles C. Maxfield 2014-06-17 18:45:15 PDT
The style red bubble is a false negative, and it looks like the EFL bot was killed during compilation
Comment 11 zalan 2014-06-20 11:12:42 PDT
Comment on attachment 233253 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:202
> +    if (endPos < m_start)

if (endPos <= m_start)

> Source/WebCore/rendering/InlineTextBox.cpp:204
> +    unsigned ePos = std::min(endPos - m_start, static_cast<unsigned>(m_len));

Could you change m_len's type to unsigned too?
I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.

Its setter looks wrong too and can result in unsigned overflow:
void setLen(unsigned len) { m_len = len; }
So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.

It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.

> Source/WebCore/rendering/InlineTextBox.cpp:278
> +    if (endPos < m_start)

same here. though now that I am seeing it the second time, it might be better to keep the old structure, though I don't feel too strongly about it.  (shaving off a std::min() call vs. better code structuring (better code reading, less error prone))
Comment 12 zalan 2014-06-20 11:12:43 PDT
Comment on attachment 233253 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:202
> +    if (endPos < m_start)

if (endPos <= m_start)

> Source/WebCore/rendering/InlineTextBox.cpp:204
> +    unsigned ePos = std::min(endPos - m_start, static_cast<unsigned>(m_len));

Could you change m_len's type to unsigned too?
I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.

Its setter looks wrong too and can result in unsigned overflow:
void setLen(unsigned len) { m_len = len; }
So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.

It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.

> Source/WebCore/rendering/InlineTextBox.cpp:278
> +    if (endPos < m_start)

same here. though now that I am seeing it the second time, it might be better to keep the old structure, though I don't feel too strongly about it.  (shaving off a std::min() call vs. better code structuring (better code reading, less error prone))
Comment 13 Myles C. Maxfield 2014-06-20 15:53:48 PDT
Comment on attachment 233253 [details]
Patch

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

>>> Source/WebCore/rendering/InlineTextBox.cpp:204
>>> +    unsigned ePos = std::min(endPos - m_start, static_cast<unsigned>(m_len));
>> 
>> Could you change m_len's type to unsigned too?
>> I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.
>> 
>> Its setter looks wrong too and can result in unsigned overflow:
>> void setLen(unsigned len) { m_len = len; }
>> So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.
>> 
>> It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.
> 
> Could you change m_len's type to unsigned too?
> I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.
> 
> Its setter looks wrong too and can result in unsigned overflow:
> void setLen(unsigned len) { m_len = len; }
> So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.
> 
> It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.

That's a good idea, but I do thing that that would be better to do in another patch. I designed this one to have no changed runtime characteristics.
Comment 14 zalan 2014-06-20 16:04:19 PDT
(In reply to comment #13)
> (From update of attachment 233253 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233253&action=review
> 
> >>> Source/WebCore/rendering/InlineTextBox.cpp:204
> >>> +    unsigned ePos = std::min(endPos - m_start, static_cast<unsigned>(m_len));
> >> 
> >> Could you change m_len's type to unsigned too?
> >> I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.
> >> 
> >> Its setter looks wrong too and can result in unsigned overflow:
> >> void setLen(unsigned len) { m_len = len; }
> >> So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.
> >> 
> >> It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.
> > 
> > Could you change m_len's type to unsigned too?
> > I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.
> > 
> > Its setter looks wrong too and can result in unsigned overflow:
> > void setLen(unsigned len) { m_len = len; }
> > So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.
> > 
> > It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.
> 
> That's a good idea, but I do thing that that would be better to do in another patch. I designed this one to have no changed runtime characteristics.
On certain hw, going from signed to unsigned does change runtime characteristic, but I agree, it's better to do it in a separate patch.
Comment 15 Myles C. Maxfield 2014-06-20 16:12:11 PDT
Created attachment 233476 [details]
Patch
Comment 16 Myles C. Maxfield 2014-06-20 16:14:11 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 233253 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=233253&action=review
> > 
> > >>> Source/WebCore/rendering/InlineTextBox.cpp:204
> > >>> +    unsigned ePos = std::min(endPos - m_start, static_cast<unsigned>(m_len));
> > >> 
> > >> Could you change m_len's type to unsigned too?
> > >> I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.
> > >> 
> > >> Its setter looks wrong too and can result in unsigned overflow:
> > >> void setLen(unsigned len) { m_len = len; }
> > >> So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.
> > >> 
> > >> It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.
> > > 
> > > Could you change m_len's type to unsigned too?
> > > I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.
> > > 
> > > Its setter looks wrong too and can result in unsigned overflow:
> > > void setLen(unsigned len) { m_len = len; }
> > > So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.
> > > 
> > > It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.
> > 
> > That's a good idea, but I do thing that that would be better to do in another patch. I designed this one to have no changed runtime characteristics.
> On certain hw, going from signed to unsigned does change runtime characteristic, but I agree, it's better to do it in a separate patch.

Really? Signed math isn't always the same speed as unsigned math on all hardware? I had no idea.
Comment 17 WebKit Commit Bot 2014-06-20 16:14:43 PDT
Attachment 233476 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:106:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Andreas Kling 2014-06-20 16:17:30 PDT
(In reply to comment #16)
> > > That's a good idea, but I do thing that that would be better to do in another patch. I designed this one to have no changed runtime characteristics.
> > On certain hw, going from signed to unsigned does change runtime characteristic, but I agree, it's better to do it in a separate patch.
> 
> Really? Signed math isn't always the same speed as unsigned math on all hardware? I had no idea.

ARMv7 doesn't have an instruction for signed modulo, for instance.
Comment 19 zalan 2014-06-20 16:25:59 PDT
Comment on attachment 233476 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:278
> +    if (endPos < m_start)

endPos <= m_start
Comment 20 Myles C. Maxfield 2014-06-22 10:18:27 PDT
http://trac.webkit.org/changeset/170265
Comment 21 WebKit Commit Bot 2014-06-22 11:05:55 PDT
Re-opened since this is blocked by bug 134174
Comment 22 Myles C. Maxfield 2014-06-23 17:31:01 PDT
The problem was a "<" vs a "<=". Re-landed in http://trac.webkit.org/changeset/170337.
Comment 23 Csaba Osztrogonác 2014-06-24 04:25:08 PDT
(In reply to comment #22)
> The problem was a "<" vs a "<=". Re-landed in http://trac.webkit.org/changeset/170337.

It made 3 tests crash on Apple Mac debug bots.
Comment 24 WebKit Commit Bot 2014-06-24 07:19:51 PDT
Re-opened since this is blocked by bug 134250
Comment 25 Myles C. Maxfield 2014-06-24 12:00:49 PDT
Having trouble reproducing locally :(
Comment 26 Myles C. Maxfield 2014-06-24 14:02:13 PDT
Was able to reproduce these, and the fix is trivial. I'll land again shortly. Hopefully this time it sticks.
Comment 27 Myles C. Maxfield 2014-06-24 14:05:04 PDT
http://trac.webkit.org/changeset/170390