Bug 177102 - Allow modern decoding of Vectors
Summary: Allow modern decoding of Vectors
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: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-18 14:22 PDT by Alex Christensen
Modified: 2017-09-27 12:24 PDT (History)
8 users (show)

See Also:


Attachments
Patch (72.96 KB, patch)
2017-09-18 14:23 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (72.96 KB, patch)
2017-09-18 14:29 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (73.88 KB, patch)
2017-09-18 14:36 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (76.93 KB, patch)
2017-09-18 14:52 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (77.59 KB, patch)
2017-09-18 14:59 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (124.21 KB, patch)
2017-09-19 10:26 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (127.89 KB, patch)
2017-09-19 10:40 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (129.51 KB, patch)
2017-09-19 10:57 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (131.91 KB, patch)
2017-09-19 11:17 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (140.81 KB, patch)
2017-09-19 11:54 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (2.35 MB, application/zip)
2017-09-19 14:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.15 MB, application/zip)
2017-09-19 15:43 PDT, Build Bot
no flags Details
Patch (145.77 KB, patch)
2017-09-19 15:51 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (145.75 KB, patch)
2017-09-19 16:16 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2017-09-18 14:22:26 PDT
Allow modern decoding of Vectors
Comment 1 Alex Christensen 2017-09-18 14:23:51 PDT
Created attachment 321129 [details]
Patch
Comment 2 Alex Christensen 2017-09-18 14:29:01 PDT
Created attachment 321130 [details]
Patch
Comment 3 Alex Christensen 2017-09-18 14:36:02 PDT
Created attachment 321132 [details]
Patch
Comment 4 Alex Christensen 2017-09-18 14:52:29 PDT
Created attachment 321135 [details]
Patch
Comment 5 Alex Christensen 2017-09-18 14:59:02 PDT
Created attachment 321137 [details]
Patch
Comment 6 Andy Estes 2017-09-18 16:00:05 PDT
Comment on attachment 321137 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBDatabaseIdentifier.cpp:38
> -IDBDatabaseIdentifier::IDBDatabaseIdentifier(const String& databaseName, const SecurityOrigin& openingOrigin, const SecurityOrigin& mainFrameOrigin)
> +IDBDatabaseIdentifier::IDBDatabaseIdentifier(const String& databaseName, const SecurityOriginData& openingOrigin, const SecurityOriginData& mainFrameOrigin)

Can the SecurityOriginDatas be rvalue references?

> Source/WebKit/Platform/IPC/ArgumentCoders.h:274
> +    static std::optional<Vector<T, inlineCapacity>> decode(Decoder& decoder)

It would be nice if we implemented the non-optional version of this function in terms of the optional-returning version.

> Source/WebKit/Platform/IPC/ArgumentCoders.h:280
> +        Vector<T, inlineCapacity> vector;

We should use reserveInitialCapacity() here since we know the size.

> Source/WebKit/Platform/IPC/ArgumentCoders.h:286
> +            vector.append(WTFMove(*element));

We should use uncheckedAppend().

> Source/WebKit/Platform/IPC/ArgumentCoders.h:324
> +    static std::optional<Vector<T, inlineCapacity>> decode(Decoder& decoder)

It would be nice if we implemented the non-optional version of this function in terms of the optional-returning version.

> Source/WebKit/Shared/RTCNetwork.cpp:143
> +        return result;

WTFMove?

> Source/WebKit/Shared/RTCNetwork.cpp:150
> +    return result;

Ditto.

> Source/WebKit/Shared/RTCNetwork.cpp:213
> +    return result;

Ditto.

> Source/WebKit/Shared/WebCompiledContentRuleListData.cpp:79
> +    return compiledContentRuleListData;

Ditto.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:660
> +    return rect;

Ditto.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:815
> +    return recentSearch;

Ditto.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:944
> +    return pluginInfo;

Ditto.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2258
> +    return blobPart;

Ditto.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2508
> +    return statistics;

Ditto.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2566
> +    return device;

Ditto.

> Source/WebKit/Shared/WebPopupItem.cpp:123
> +    return item;

Ditto.

> Source/WebKit/Shared/Gamepad/GamepadData.cpp:86
> +    return data;

Ditto.

> Source/WebKit/Shared/Plugins/NPIdentifierData.cpp:93
> +    return result;

Ditto.

> Source/WebKit/Shared/Plugins/NPVariantData.cpp:171
> +        return result;

Buncha dittos.
Comment 7 Alex Christensen 2017-09-19 10:26:16 PDT
Created attachment 321208 [details]
Patch
Comment 8 Alex Christensen 2017-09-19 10:40:33 PDT
Created attachment 321210 [details]
Patch
Comment 9 Alex Christensen 2017-09-19 10:57:37 PDT
Created attachment 321214 [details]
Patch
Comment 10 Alex Christensen 2017-09-19 11:17:26 PDT
Created attachment 321217 [details]
Patch
Comment 11 Alex Christensen 2017-09-19 11:54:00 PDT
Created attachment 321227 [details]
Patch
Comment 12 Build Bot 2017-09-19 14:03:14 PDT
Comment on attachment 321227 [details]
Patch

Attachment 321227 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4596440

Number of test failures exceeded the failure limit.
Comment 13 Build Bot 2017-09-19 14:03:15 PDT
Created attachment 321240 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 14 Build Bot 2017-09-19 15:43:18 PDT
Comment on attachment 321227 [details]
Patch

Attachment 321227 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4597191

Number of test failures exceeded the failure limit.
Comment 15 Build Bot 2017-09-19 15:43:20 PDT
Created attachment 321258 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 16 Alex Christensen 2017-09-19 15:51:14 PDT
Created attachment 321259 [details]
Patch
Comment 17 Anders Carlsson 2017-09-19 16:09:15 PDT
Comment on attachment 321259 [details]
Patch

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

> Source/WebKit/Platform/IPC/ArgumentCoders.h:271
> +        Vector<T, inlineCapacity> vector;
> +        vector.reserveInitialCapacity(size);

There's a reason why reserveInitialCapacity wasn't called here - a compromised web process could send MAXINT and cause an allocation failure.
Comment 18 Alex Christensen 2017-09-19 16:11:08 PDT
Wouldn't it crash either way?
Comment 19 Alex Christensen 2017-09-19 16:16:04 PDT
Created attachment 321262 [details]
Patch
Comment 20 Alex Christensen 2017-09-19 16:18:27 PDT
http://trac.webkit.org/r222233
Comment 21 Radar WebKit Bug Importer 2017-09-27 12:24:20 PDT
<rdar://problem/34693204>