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
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks: 181178
  Show dependency treegraph
 
Reported: 2018-01-09 15:13 PST by Brady Eidson
Modified: 2021-01-14 12:09 PST (History)
11 users (show)

See Also:


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

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>