Bug 160735 - Migrate from ints to unsigneds when referring to indices into strings
Summary: Migrate from ints to unsigneds when referring to indices into strings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-10 02:43 PDT by Myles C. Maxfield
Modified: 2017-10-11 09:47 PDT (History)
6 users (show)

See Also:


Attachments
WIP (58.27 KB, patch)
2016-08-10 02:47 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (129.43 KB, patch)
2016-08-10 18:32 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (133.37 KB, patch)
2016-08-11 13:25 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (137.02 KB, patch)
2016-08-11 13:47 PDT, Myles C. Maxfield
simon.fraser: review+
Details | Formatted Diff | Diff
Patch for committing (140.88 KB, patch)
2016-08-11 16:30 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for committing (141.91 KB, patch)
2016-08-11 16:56 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for committing (142.76 KB, patch)
2016-08-11 17:10 PDT, Myles C. Maxfield
no flags 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 2016-08-10 02:43:51 PDT
Migrate from ints to unsigneds when referring to indices into strings
Comment 1 Myles C. Maxfield 2016-08-10 02:47:24 PDT
Created attachment 285726 [details]
WIP
Comment 2 Myles C. Maxfield 2016-08-10 18:32:02 PDT
Created attachment 285798 [details]
WIP
Comment 3 WebKit Commit Bot 2016-08-10 19:32:39 PDT
Attachment 285798 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Myles C. Maxfield 2016-08-11 13:25:37 PDT
Created attachment 285848 [details]
Patch
Comment 5 Myles C. Maxfield 2016-08-11 13:47:07 PDT
Created attachment 285851 [details]
Patch
Comment 6 Simon Fraser (smfr) 2016-08-11 14:20:06 PDT
Comment on attachment 285851 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:2094
> +        ASSERT(startOffset >= 0 && endOffset >= 0);

The autos hide whether these are ints or unsigned so I have no idea whether the assertion is useful.

> Source/WebCore/platform/DragImage.cpp:179
> +    auto startOffset = start.deprecatedEditingOffset();
> +    auto endOffset = end.deprecatedEditingOffset();
> +    ASSERT(startOffset >= 0 && endOffset >= 0);

Ditto.

> Source/WebCore/platform/graphics/Latin1TextIterator.h:62
> +    unsigned m_currentCharacter;
> +    unsigned m_lastCharacter;

These could be renamed to current/lastIndex perhaps?

> Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.h:65
> +    unsigned m_currentCharacter;
> +    unsigned m_lastCharacter;
> +    unsigned m_endCharacter;

Ditto

> Source/WebCore/platform/graphics/WidthIterator.cpp:111
> +    ASSERT(glyphBufferSize >= lastGlyphCount);

I would flip this: ASSERT(lastGlyphCount <= glyphBufferSize);

> Source/WebCore/rendering/InlineTextBox.cpp:139
> +    // FIXME: We should only be checking if sPos >= ePos here, because those are the
> +    // indices used to actually generate the selection rect. Allowing us past this guard
> +    // on any other condition creates zero-width selection rects.

But what's the proposed solution? Should this reference a bug?

> Source/WebCore/rendering/InlineTextBox.cpp:175
> +            ASSERT(static_cast<int>(selectionStart) >= 0 && static_cast<int>(selectionEnd) >= 0);

This will never fire, right?

> Source/WebCore/rendering/InlineTextBox.cpp:206
> +    // FIXME: We should only be checking if sPos >= ePos here, because those are the
> +    // indices used to actually generate the selection rect. Allowing us past this guard
> +    // on any other condition creates zero-width selection rects.

But what's the proposed solution? Should this reference a bug?

> Source/WebCore/rendering/InlineTextBox.cpp:597
> +unsigned InlineTextBox::clampedOffset(unsigned x) const

Blank line above pls.

> Source/WebCore/rendering/RenderView.cpp:917
> +                auto newEndPos = node->offsetInCharacters() ? node->maxCharacterOffset() : node->countChildNodes();
> +                ASSERT(newEndPos >= 0);

Can't tell whether the assertion is useful.

> Source/WebCore/rendering/RenderView.h:321
>      RenderObject* m_selectionUnsplitStart;
>      RenderObject* m_selectionUnsplitEnd;

Maybe move this class to initializers?

> Source/WebCore/rendering/svg/RenderSVGInlineText.cpp:135
> +    ASSERT(caretOffset > 0);

Nope.
Comment 7 Myles C. Maxfield 2016-08-11 14:44:52 PDT
Comment on attachment 285851 [details]
Patch

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

>> Source/WebCore/rendering/RenderView.h:321
>>      RenderObject* m_selectionUnsplitEnd;
> 
> Maybe move this class to initializers?

I don't understand what you mean by this. Could you explain it?
Comment 8 Myles C. Maxfield 2016-08-11 16:30:08 PDT
Created attachment 285863 [details]
Patch for committing
Comment 9 Myles C. Maxfield 2016-08-11 16:56:55 PDT
Created attachment 285868 [details]
Patch for committing
Comment 10 Myles C. Maxfield 2016-08-11 17:10:18 PDT
Created attachment 285870 [details]
Patch for committing
Comment 11 WebKit Commit Bot 2016-08-11 18:24:25 PDT
Comment on attachment 285870 [details]
Patch for committing

Clearing flags on attachment: 285870

Committed r204400: <http://trac.webkit.org/changeset/204400>
Comment 12 Philippe Normand 2016-08-12 04:02:30 PDT
Looks like this patch broke the GTK Debug tests bot.

For instance fast/forms/input-text-paste-maxlength.html now crashes the WebProcess:

https://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug%20(Tests)/r204408%20(10632)/fast/forms/input-text-paste-maxlength-crash-log.txt

Thread 1 (Thread 0x7f4976d6a940 (LWP 45002)):
#0  0x00007f49868ac334 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:323
#1  0x00007f498eb7800d in (anonymous namespace)::HarfBuzzShaper::selectionRect (this=0x7fffa92bfe70, point=..., height=0, from=0, to=5) at ../../Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:661
#2  0x00007f498eb57b8c in (anonymous namespace)::FontCascade::adjustSelectionRectForComplexText (this=0x7f4976257018, run=..., selectionRect=..., from=0, to=7) at ../../Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp:93
#3  0x00007f498e385b62 in (anonymous namespace)::FontCascade::adjustSelectionRectForText (this=0x7f4976257018, run=..., selectionRect=..., from=0, to=...) at ../../Source/WebCore/platform/graphics/FontCascade.cpp:505
#4  0x00007f498e4f66ba in (anonymous namespace)::InlineTextBox::positionForOffset (this=0x7f4976259a80, offset=7) at ../../Source/WebCore/rendering/InlineTextBox.cpp:1016
#5  0x00007f498e6ff30b in (anonymous namespace)::RenderText::localCaretRect (this=0x7f497625aae0, inlineBox=0x7f4976259a80, caretOffset=7, extraWidthToEndOfLine=0x0) at ../../Source/WebCore/rendering/RenderText.cpp:451
#6  0x00007f498dd5a36d in (anonymous namespace)::VisiblePosition::localCaretRect (this=0x7fffa92c0350, renderer=@0x7fffa92c0288: 0x7f497625aae0) at ../../Source/WebCore/editing/VisiblePosition.cpp:660
#7  0x00007f498dd72726 in (anonymous namespace)::localCaretRectInRendererForCaretPainting (caretPosition=..., caretPainter=@0x7fffa92c0318: 0x7f498d0a0352 <(anonymous namespace)::Frame::document() const+30>) at ../../Source/WebCore/editing/htmlediting.cpp:1269
#8  0x00007f498dd1053d in (anonymous namespace)::CaretBase::updateCaretRect (this=0x7f49763bf230, document=0x7f49762ac000, caretPosition=...) at ../../Source/WebCore/editing/FrameSelection.cpp:1559
#9  0x00007f498dd10917 in (anonymous namespace)::FrameSelection::recomputeCaretRect (this=0x7f49763bf230) at ../../Source/WebCore/editing/FrameSelection.cpp:1613
#10 0x00007f498dd12fa4 in (anonymous namespace)::FrameSelection::updateAppearance (this=0x7f49763bf230) at ../../Source/WebCore/editing/FrameSelection.cpp:2033
#11 0x00007f498dd09889 in (anonymous namespace)::FrameSelection::updateAndRevealSelection (this=0x7f49763bf230, intent=...) at ../../Source/WebCore/editing/FrameSelection.cpp:380
#12 0x00007f498dd14c69 in (anonymous namespace)::FrameSelection::updateAppearanceAfterLayout (this=0x7f49763bf230) at ../../Source/WebCore/editing/FrameSelection.cpp:2386
#13 0x00007f498e1f7cd5 in (anonymous namespace)::FrameView::performPostLayoutTasks (this=0x7f49345fdb00) at ../../Source/WebCore/page/FrameView.cpp:3206
#14 0x00007f498e1f1f01 in (anonymous namespace)::FrameView::layout (this=0x7f49345fdb00, allowSubtree=true) at ../../Source/WebCore/page/FrameView.cpp:1523
#15 0x00007f498db5e712 in (anonymous namespace)::Document::updateLayout (this=0x7f49762ac000) at ../../Source/WebCore/dom/Document.cpp:2009
#16 0x00007f498db5e814 in (anonymous namespace)::Document::updateLayoutIgnorePendingStylesheets (this=0x7f49762ac000, runPostLayoutTasks=(anonymous namespace)::Document::RunPostLayoutTasks::Asynchronously) at ../../Source/WebCore/dom/Document.cpp:2041
#17 0x00007f498dd03ce6 in (anonymous namespace)::Editor::Command::execute (this=0x7fffa92c0a30, parameter="(null)", triggeringEvent=0x0) at ../../Source/WebCore/editing/EditorCommand.cpp:1778
#18 0x00007f498db6a452 in (anonymous namespace)::Document::execCommand (this=0x7f49762ac000, commandName="SelectAll", userInterface=false, value="(null)") at ../../Source/WebCore/dom/Document.cpp:4964
#19 0x00007f498ee3b17e in (anonymous namespace)::jsDocumentPrototypeFunctionExecCommand (state=0x7fffa92c0b20) at DerivedSources/WebCore/JSDocument.cpp:4633
#20 0x00007f4935ffe028 in ?? ()
#21 0x00007fffa92c0ba0 in ?? ()
#22 0x00007f49864dc8e8 in llint_entry () at ../../Source/WTF/wtf/RefPtr.h:79
Comment 13 Myles C. Maxfield 2016-08-12 14:01:25 PDT
(In reply to comment #12)
> Looks like this patch broke the GTK Debug tests bot.
I'm working on this in https://bugs.webkit.org/show_bug.cgi?id=160818