181454 – Make in-process MessagePorts be (mostly) asynchronous

Bug 181454 - Make in-process MessagePorts be (mostly) asynchronous

Summary: Make in-process MessagePorts be (mostly) asynchronous

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	WebCore Misc. (show other bugs)
Version:	Other
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Brady Eidson

URL:
Keywords:	InRadar

Depends on:
Blocks:	181178
	Show dependency tree / graph

Reported:	2018-01-09 15:13 PST by Brady Eidson
Modified:	2021-01-14 12:09 PST (History)
CC List:	11 users (show)

See Also:	220627

Attachments
Preliminary (119.25 KB, patch) 2018-01-17 16:23 PST, Brady Eidson	no flags	Details \| Formatted Diff \| Diff
Preliminary (119.28 KB, patch) 2018-01-17 16:56 PST, Brady Eidson	ews-watchlist: commit-queue-	Details \| Formatted Diff \| Diff
Preliminary (119.72 KB, patch) 2018-01-17 17:04 PST, Brady Eidson	ews-watchlist: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.16 MB, application/zip) 2018-01-17 18:32 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.46 MB, application/zip) 2018-01-17 18:40 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews100 for mac-sierra (2.60 MB, application/zip) 2018-01-17 18:59 PST, EWS Watchlist	no flags	Details
Preliminary (119.74 KB, patch) 2018-01-17 19:08 PST, Brady Eidson	ews-watchlist: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.08 MB, application/zip) 2018-01-17 20:02 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews101 for mac-sierra (2.20 MB, application/zip) 2018-01-17 20:19 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.34 MB, application/zip) 2018-01-17 20:41 PST, EWS Watchlist	no flags	Details
Preliminary (119.26 KB, patch) 2018-01-17 20:42 PST, Brady Eidson	no flags	Details \| Formatted Diff \| Diff
Preliminary (119.32 KB, patch) 2018-01-17 22:17 PST, Brady Eidson	ews-watchlist: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.59 MB, application/zip) 2018-01-17 23:23 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.28 MB, application/zip) 2018-01-17 23:41 PST, EWS Watchlist	no flags	Details
Preliminary (118.45 KB, patch) 2018-01-18 09:53 PST, Brady Eidson	no flags	Details \| Formatted Diff \| Diff
Patch (126.59 KB, patch) 2018-01-18 13:16 PST, Brady Eidson	no flags	Details \| Formatted Diff \| Diff
Patch (126.66 KB, patch) 2018-01-18 20:47 PST, Brady Eidson	no flags	Details \| Formatted Diff \| Diff
Patch (126.69 KB, patch) 2018-01-18 20:53 PST, Brady Eidson	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (17) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Brady Eidson 2018-01-09 15:13:22 PST

Make in-process MessagePorts be fully asynchronous

While working on https://bugs.webkit.org/show_bug.cgi?id=181178 the approach of using "MesasgePortChannels" became too unwieldly. It works, but is fragile.

Instead, even in-process MessagePorts should be fully asynchronous (and this means that even a local MessagePort should never directly know about a remote one even if they are in the same process... even if they are in the same ScriptExecutionContext)

Comment 1 Brady Eidson 2018-01-17 16:22:37 PST

"Fully asynchronous except for a sync function to support GC", really

Comment 2 Brady Eidson 2018-01-17 16:23:48 PST

Created attachment 331557 [details]
Preliminary

Comment 3 EWS Watchlist 2018-01-17 16:27:31 PST

Attachment 331557 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/service/context/SWContextManager.cpp:29:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:155:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:163:  MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannel.cpp:167:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/dom/MessagePort.h:48:  The parameter name "scriptExecutionContext" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100:  MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/MessageChannel.h:39:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/workers/WorkerObjectProxy.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 13 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 4 Brady Eidson 2018-01-17 16:56:05 PST

Created attachment 331562 [details]
Preliminary

Comment 5 EWS Watchlist 2018-01-17 16:59:28 PST

Attachment 331562 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:163:  MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100:  MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 6 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 6 Brady Eidson 2018-01-17 17:04:25 PST

Created attachment 331564 [details]
Preliminary

Comment 7 EWS Watchlist 2018-01-17 17:06:20 PST

Attachment 331564 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:163:  MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100:  MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 6 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 8 EWS Watchlist 2018-01-17 18:32:55 PST

Comment on attachment 331562 [details]
Preliminary

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

Number of test failures exceeded the failure limit.

Comment 9 EWS Watchlist 2018-01-17 18:32:57 PST

Created attachment 331573 [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.6

Comment 10 EWS Watchlist 2018-01-17 18:40:51 PST

Comment on attachment 331564 [details]
Preliminary

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

Number of test failures exceeded the failure limit.

Comment 11 EWS Watchlist 2018-01-17 18:40:52 PST

Created attachment 331574 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

Comment 12 EWS Watchlist 2018-01-17 18:58:58 PST

Comment on attachment 331564 [details]
Preliminary

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

Number of test failures exceeded the failure limit.

Comment 13 EWS Watchlist 2018-01-17 18:59:00 PST

Created attachment 331577 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

Comment 14 Brady Eidson 2018-01-17 19:08:22 PST

Created attachment 331578 [details]
Preliminary

Accidentally removed a necessary unlockEarly() in the previous patches

Comment 15 EWS Watchlist 2018-01-17 19:10:03 PST

Attachment 331578 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:164:  MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100:  MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 6 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 16 EWS Watchlist 2018-01-17 20:02:18 PST

Comment on attachment 331578 [details]
Preliminary

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

Number of test failures exceeded the failure limit.

Comment 17 EWS Watchlist 2018-01-17 20:02:19 PST

Created attachment 331581 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

Comment 18 EWS Watchlist 2018-01-17 20:19:52 PST

Comment on attachment 331578 [details]
Preliminary

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

New failing tests:
security/cannot-read-self-from-file.html
fast/xmlhttprequest/xmlhttprequest-no-file-access.html

Comment 19 EWS Watchlist 2018-01-17 20:19:54 PST

Created attachment 331582 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

Comment 20 EWS Watchlist 2018-01-17 20:41:55 PST

Comment on attachment 331578 [details]
Preliminary

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

Number of test failures exceeded the failure limit.

Comment 21 EWS Watchlist 2018-01-17 20:41:56 PST

Created attachment 331583 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

Comment 22 Brady Eidson 2018-01-17 20:42:37 PST

Created attachment 331584 [details]
Preliminary

Comment 23 EWS Watchlist 2018-01-17 20:45:18 PST

Attachment 331584 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:164:  MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100:  MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 6 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 24 Brady Eidson 2018-01-17 22:17:12 PST

Created attachment 331597 [details]
Preliminary

Wow.
Turns out RunLoop::main().dispatch() and callOnMainThread() have completely different semantics even though they seem like synonyms for one-another.

Comment 25 EWS Watchlist 2018-01-17 22:21:13 PST

Attachment 331597 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:164:  MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100:  MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 6 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 26 EWS Watchlist 2018-01-17 23:23:19 PST

Comment on attachment 331597 [details]
Preliminary

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

Number of test failures exceeded the failure limit.

Comment 27 EWS Watchlist 2018-01-17 23:23:20 PST

Created attachment 331601 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

Comment 28 EWS Watchlist 2018-01-17 23:41:20 PST

Comment on attachment 331597 [details]
Preliminary

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

Number of test failures exceeded the failure limit.

Comment 29 EWS Watchlist 2018-01-17 23:41:21 PST

Created attachment 331602 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

Comment 30 Brady Eidson 2018-01-18 09:53:34 PST

Created attachment 331635 [details]
Preliminary

Comment 31 EWS Watchlist 2018-01-18 09:57:38 PST

Attachment 331635 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:164:  MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100:  MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 6 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 32 Brady Eidson 2018-01-18 13:16:17 PST

Created attachment 331653 [details]
Patch

Comment 33 EWS Watchlist 2018-01-18 13:18:16 PST

Attachment 331653 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:164:  MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100:  MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 6 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 34 Alex Christensen 2018-01-18 16:04:54 PST

Comment on attachment 331653 [details]
Patch

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

> Source/WebCore/dom/MessagePort.cpp:229
> +        auto innerHandler = [this, protectedThis = makeRef(*this)](Vector<MessageWithMessagePorts>&& messages) {

MSVC doesn't like this line.  I think it would be fixed if you did protectedThis = WTFMove(protectedThis)

Comment 35 Alex Christensen 2018-01-18 16:21:02 PST

Comment on attachment 331653 [details]
Patch

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

> Source/WebCore/dom/messageports/MessagePortChannel.h:55
> +    void takeAllMessagesForPort(const MessagePortIdentifier&, Function<void(Vector<MessageWithMessagePorts>&&, Function<void()>&&)>&&);

This could use CompletionHandler

> Source/WebCore/dom/messageports/MessagePortChannel.h:65
> +    unsigned m_refCount { 1 };

What's this for?

> Source/WebCore/dom/messageports/MessagePortChannelProvider.cpp:47
> +void MessagePortChannelProvider::setSharedProvider(MessagePortChannelProvider& provider)

This doesn't seem to be called.  Will it be?

> Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:40
> +void MessagePortChannelProviderImpl::performActionOnAppropriateThread(Function<void()>&& action)

performActionOnMainThread?

> Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:102
> +    // FIXME: Remove this sync function call when GC logic is made asynchronous.

Is this really ever going to happen?

> Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45
> +    bool hasMessagesForPorts_temporarySync(const MessagePortIdentifier&, const MessagePortIdentifier&) final;

We typically don't like _ in function names.
Why is this temporary sync?

Comment 36 Brady Eidson 2018-01-18 17:13:57 PST

(In reply to Alex Christensen from comment #35)
> Comment on attachment 331653 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331653&action=review
> 
> > Source/WebCore/dom/messageports/MessagePortChannel.h:55
> > +    void takeAllMessagesForPort(const MessagePortIdentifier&, Function<void(Vector<MessageWithMessagePorts>&&, Function<void()>&&)>&&);
> 
> This could use CompletionHandler

I might try this, but every time I do I run into some nullness requirement.

> 
> > Source/WebCore/dom/messageports/MessagePortChannel.h:65
> > +    unsigned m_refCount { 1 };
> 
> What's this for?

Leftover from testing/debugging with my own ref/deref()  :( Removed.

> > Source/WebCore/dom/messageports/MessagePortChannelProvider.cpp:47
> > +void MessagePortChannelProvider::setSharedProvider(MessagePortChannelProvider& provider)
> 
> This doesn't seem to be called.  Will it be?

Yes!

But I can remove it from here.

> > Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:40
> > +void MessagePortChannelProviderImpl::performActionOnAppropriateThread(Function<void()>&& action)
> 
> performActionOnMainThread?

Yah, sure.

> 
> > Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:102
> > +    // FIXME: Remove this sync function call when GC logic is made asynchronous.
> 
> Is this really ever going to happen?

Absolutely.

> > Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45
> > +    bool hasMessagesForPorts_temporarySync(const MessagePortIdentifier&, const MessagePortIdentifier&) final;
> 
> We typically don't like _ in function names.
> Why is this temporary sync?

I called this out in the ChangeLog - I want this to look ugly and wrong and discourage use. It's going away in the async GC followup.

Comment 37 Alex Christensen 2018-01-18 17:14:52 PST

Comment on attachment 331653 [details]
Patch

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

r=me once windows bots are green.

> Source/WebCore/dom/messageports/MessagePortChannel.h:74
> +    MessagePortIdentifier m_ports[2];

These could be pairs.  Or not.

> Source/WebCore/dom/messageports/MessagePortChannelProvider.cpp:41
> +            globalProvider = new MessagePortChannelProviderImpl;

I feel like this is what NeverDestroyed is for.

> Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37
> +    __attribute__ ((noreturn)) ~MessagePortChannelRegistry();

This probably doesn't work on MSVC.  It looks like we need __declspec(noreturn) in a macro.

Comment 38 Brady Eidson 2018-01-18 20:37:39 PST

(In reply to Alex Christensen from comment #37)
> Comment on attachment 331653 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331653&action=review
> 
> r=me once windows bots are green.
> 
> > Source/WebCore/dom/messageports/MessagePortChannelProvider.cpp:41
> > +            globalProvider = new MessagePortChannelProviderImpl;
> 
> I feel like this is what NeverDestroyed is for.

Kindof! But with the additional semantic of the outside client being able to override that makes NeverDestroyed a little less applicable.
> 
> > Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37
> > +    __attribute__ ((noreturn)) ~MessagePortChannelRegistry();
> 
> This probably doesn't work on MSVC.  It looks like we need
> __declspec(noreturn) in a macro.

Ugh.

Comment 39 Brady Eidson 2018-01-18 20:47:53 PST

Created attachment 331704 [details]
Patch

Comment 40 Brady Eidson 2018-01-18 20:53:05 PST

Created attachment 331706 [details]
Patch

Comment 41 EWS Watchlist 2018-01-18 20:55:07 PST

Attachment 331706 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:164:  MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45:  hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100:  MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 42 WebKit Commit Bot 2018-01-18 22:33:17 PST

Comment on attachment 331706 [details]
Patch

Clearing flags on attachment: 331706

Committed r227190: <https://trac.webkit.org/changeset/227190>

Comment 43 WebKit Commit Bot 2018-01-18 22:33:19 PST

All reviewed patches have been landed.  Closing bug.

Comment 44 Radar WebKit Bug Importer 2018-01-18 22:34:51 PST

<rdar://problem/36647079>

Comment 45 Jonathan Bedard 2018-01-19 10:13:45 PST

Follow-up build fix required to remove unused lambda captures <https://trac.webkit.org/changeset/227213>.

Comment 46 Michael Catanzaro 2018-01-23 09:26:47 PST

Committed r227420: <https://trac.webkit.org/changeset/227420>