Bug 209086 - Color Picker crashes on touch
Summary: Color Picker crashes on touch
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: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-13 15:22 PDT by Megan Gardner
Modified: 2020-04-25 21:06 PDT (History)
11 users (show)

See Also:


Attachments
Patch (3.66 KB, patch)
2020-03-16 10:49 PDT, Megan Gardner
wenson_hsieh: review+
Details | Formatted Diff | Diff
Patch (3.68 KB, patch)
2020-03-16 10:57 PDT, Megan Gardner
wenson_hsieh: review+
Details | Formatted Diff | Diff
Patch (3.69 KB, patch)
2020-03-16 11:00 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2020-03-13 15:22:24 PDT
Color Picker crashes on touch
Comment 1 Megan Gardner 2020-03-16 10:49:26 PDT
Created attachment 393662 [details]
Patch
Comment 2 Wenson Hsieh 2020-03-16 10:54:06 PDT
Comment on attachment 393662 [details]
Patch

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

> LayoutTests/fast/forms/color/color-input-activate-crash.html:13
> +		if (!testRunner.runUIScript)

Nit - spaces instead of tabs.

> LayoutTests/fast/forms/color/color-input-activate-crash.html:16
> +	    var input = document.getElementById('colorInput');

Nit - it looks like there are a mix of spaces and tabs here.

> LayoutTests/fast/forms/color/color-input-activate-crash.html:19
> +		await UIHelper.activateElementAndWaitForInputSession(input);
> +	    document.getElementById('result').innerHTML = 'PASS: Test did not crash';
> +	    testRunner.notifyDone();

Ditto with these lines.

> LayoutTests/fast/forms/color/color-input-activate-crash.html:22
> +	window.addEventListener('load', runTest, false);

Ditto.
Comment 3 Megan Gardner 2020-03-16 10:57:06 PDT
Created attachment 393664 [details]
Patch
Comment 4 Wenson Hsieh 2020-03-16 10:58:14 PDT
Comment on attachment 393664 [details]
Patch

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

> LayoutTests/fast/forms/color/color-input-activate-crash.html:17
> +        wait UIHelper.activateElementAndWaitForInputSession(input);

wait => await?
Comment 5 Megan Gardner 2020-03-16 11:00:24 PDT
Created attachment 393666 [details]
Patch
Comment 6 Darin Adler 2020-03-16 12:50:00 PDT
Comment on attachment 393666 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Vector sizing lost in refactor. Not perfomance sensitive code, so just expanding vector as needed.

Thanks for fixing my mistake!

There are really two questions about using append. One you have answered here. The other is whether the vector lives a long time -- if it did we would want to make sure it doesn’t use a lot of space. I think it does not live a long time.

If the vector did live a long time, there are two ways we could deal with that:

1) Less effective: Add a call to suggestions.shrinkToFit() after the loop.

2) More effective: Leave this using uncheckedAppend and add this line of code before the loop:

    suggestions.reserveInitialCapacity(std::distance(dataList->suggestions().begin(), { }));

I’m going to assume it does not live a long time.
Comment 7 WebKit Commit Bot 2020-03-16 13:21:08 PDT
Comment on attachment 393666 [details]
Patch

Clearing flags on attachment: 393666

Committed r258516: <https://trac.webkit.org/changeset/258516>
Comment 8 Radar WebKit Bug Importer 2020-03-16 15:17:39 PDT
<rdar://problem/60512626>