Bug 220919 – MachSemaphore does not work well with IPC messages

• Component: WebKit2
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

MachSemaphore does not work well with IPC messages
1) It cannot be sent as "MachSemaphore semaphore", only as "MachSendRight semaphoreSendRight".
2) It cannot be part of an object that is received, since it does not have move constructors.

[Kimmo Kinnunen]

Created attachment 418285 [details]
Patch

[Chris Dumez]

Curious to know how others feel about this change. I came up with the current pattern and I thought it was pretty clear. But if people feel it would be nicer to send a MachSemaphore over IPC then I am certainly not opposed to it.

[Wenson Hsieh]

(In reply to Chris Dumez from comment #2)
> Curious to know how others feel about this change. I came up with the
> current pattern and I thought it was pretty clear. But if people feel it
> would be nicer to send a MachSemaphore over IPC then I am certainly not
> opposed to it.

I agree. Sharing mach ports (MachSendRights) between processes is already a pretty well-established pattern in WebKit2, but making it slightly easier by folding more of the logic into WTF::MachSemaphore doesn't seem like a bad idea.

[Chris Dumez]

(In reply to Chris Dumez from comment #2)
> Curious to know how others feel about this change. I came up with the
> current pattern and I thought it was pretty clear. But if people feel it
> would be nicer to send a MachSemaphore over IPC then I am certainly not
> opposed to it.

Keep in mind, for now at least, it would mean that we have 2 ways to do this: send the whole MachSemaphore or send a MachSendRight since this patch does not update existing users of MachSendRight. Having more than one way to do one thing can be confusing.

It is also not super clear to me how I would update my existing call site to this new pattern. It's not like you send a MachSemaphore to another process and forget about it. You'd want to keep the MachSemaphore for yourself too. While this patch adds a MachSemaphore move constructor, there is no copy construct AFAICT. So I am not super clear on how it would look. This is why I don't particularly like when we add new functionality that is not used in the code. It would have been nicer if existing code would have been ported in this patch so we could have judged more easily how much better it looks.

[Kimmo Kinnunen]

(In reply to Chris Dumez from comment #4)
> It would have been nicer if existing code would have been ported
> in this patch so we could have judged more easily how much better it looks.

I did write this too, but the existing codepaths had a bit of auxiliary issues that did distract to get this completed for what I needed.

I can change this to just do the move constructor, operator and revert the encoder, decoder, messages.py. This way MachSemaphore can be part of a decoded object (Optional<MyObjThatHasMachSemaphore>). Does this sound better?

[Kimmo Kinnunen]

(In reply to Chris Dumez from comment #4)

> It is also not super clear to me how I would update my existing call site to
> this new pattern. It's not like you send a MachSemaphore to another process
> and forget about it. You'd want to keep the MachSemaphore for yourself too.
> While this patch adds a MachSemaphore move constructor

Sending (encoding) and move constructor do not relate.

In this patch, you keep the semaphore you have, business as usual. It's also just encoded business as usual.

[Kimmo Kinnunen]

(In reply to Kimmo Kinnunen from comment #5)
> (In reply to Chris Dumez from comment #4)
> > It would have been nicer if existing code would have been ported
> > in this patch so we could have judged more easily how much better it looks.
> 
> I did write this too, but the existing codepaths had a bit of auxiliary
> issues that did distract to get this completed for what I needed.


Alternatively I can work tomorrow to fix the auxiliary issues in your and Wenson's call sites, but I don't know if that's what is the best.

[Kimmo Kinnunen]

> Sharing mach ports (MachSendRights) between processes is already a pretty well-established pattern in WebKit2

FWIW: Sharing a send right is a means to an end. Typically in typed systems you want to communicate the "end", not the means.

Compare the established patterns:
- SendVector(int64_t sz, 7 ints). 
- SendVector(Vector)

- SendSemaphore(MachSendRight semaphoreSendRight)
- SendSemaphore(MachSemaphore semaphore)

[Chris Dumez]

(In reply to Kimmo Kinnunen from comment #8)
> > Sharing mach ports (MachSendRights) between processes is already a pretty well-established pattern in WebKit2
> 
> FWIW: Sharing a send right is a means to an end. Typically in typed systems
> you want to communicate the "end", not the means.
> 
> Compare the established patterns:
> - SendVector(int64_t sz, 7 ints). 
> - SendVector(Vector)
> 
> - SendSemaphore(MachSendRight semaphoreSendRight)
> - SendSemaphore(MachSemaphore semaphore)

My suggestion would be to update one call site at least (either mine or Wenson) so we can judge how it looks compared to now.

[Kimmo Kinnunen]

Created attachment 418396 [details]
Patch

[Kimmo Kinnunen]

Created attachment 418399 [details]
Patch

[Kimmo Kinnunen]

Created attachment 418401 [details]
Patch

[Kimmo Kinnunen]

Created attachment 418403 [details]
Patch

[Kimmo Kinnunen]

Created attachment 418404 [details]
Patch

[Kimmo Kinnunen]

Created attachment 418410 [details]
Patch

[Chris Dumez]

Comment on attachment 418410 [details]
Patch

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

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h:129
> +#if PLATFORM(COCOA)

This is more if-defing than before so it does not look as nice.

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in:26
> +#if PLATFORM(COCOA)

Ditto.

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:67
> +Ref<RemoteRenderingBackend> RemoteRenderingBackend::create(GPUConnectionToWebProcess& gpuConnectionToWebProcess, RenderingBackendIdentifier identifier, PlatformCreateParams&& params)

No abbreviations in WebKit: params -> parameters

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:72
> +RemoteRenderingBackend::RemoteRenderingBackend(GPUConnectionToWebProcess& gpuConnectionToWebProcess, RenderingBackendIdentifier identifier, PlatformCreateParams&& params)

ditto.

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h:69
> +    struct PlatformCreateParams {

No abbreviations.

> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:54
> +    struct PlatformCreateParams {

No abbreviations

> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:185
> +void RemoteAudioDestinationManager::deleteAudioDestination(RemoteAudioDestinationIdentifier identifier, Messages::RemoteAudioDestinationManager::DeleteAudioDestinationAsyncReply&& completionHandler)

Why these changes to replace CompletionHandlers with other more obscure types? This is a lot less readable. CompletionHandler is the common type we use for async IPC functions and its type makes it clear what we need to return.

> Source/WebKit/Shared/RemoteRenderingBackendCreationParameters.h:-35
> -struct RemoteRenderingBackendCreationParameters {

This struct abstracted away some ifdefing so I am unclear why we are removing it.

> Source/WebKit/WebProcess/GPU/media/RemoteAudioDestinationProxy.cpp:-209
> -    m_renderSemaphore = nullptr;

Why isn't this resetting the semaphore anymore?

[Kimmo Kinnunen]

Comment on attachment 418410 [details]
Patch

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

Given that there's reclutancy to read the ChangeLog, I'm getting a feeling the change isn't welcome. Fixing all that's wrong in the call sites isn't necessarily my war, so I'm definitively fine dropping the feature and just getting the move constructors.
Even though I think I sending MachSemaphore over tag type is correct insofar as MachSemaphores should be in messages.in (they shouldn't) I'm not the owner of the components and I don't think I should be arguing for changing them..

>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h:129
>> +#if PLATFORM(COCOA)
> 
> This is more if-defing than before so it does not look as nice.

Sure, but the de-facto webkit .messages.in style is ifdefing, if I understand correctly. There is not a de-facto rule as to "you should introduce a yet another manual struct to extract the platform specific with possibility to make errors in decoding/encoding". Not only that, but in general case it is incorrect, as explained in the commit msg.

>> Source/WebKit/Shared/RemoteRenderingBackendCreationParameters.h:-35
>> -struct RemoteRenderingBackendCreationParameters {
> 
> This struct abstracted away some ifdefing so I am unclear why we are removing it.

Explained in the commit msg, it is incorrect method to pass information to the IPC call.

>> Source/WebKit/WebProcess/GPU/media/RemoteAudioDestinationProxy.cpp:-209
>> -    m_renderSemaphore = nullptr;
> 
> Why isn't this resetting the semaphore anymore?

The semaphore send direction is changed, as explained in the commit message.

[Kimmo Kinnunen]

Created attachment 418446 [details]
Patch

[Chris Dumez]

(In reply to Kimmo Kinnunen from comment #17)
> Comment on attachment 418410 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418410&action=review
> 
> Given that there's reclutancy to read the ChangeLog, I'm getting a feeling
> the change isn't welcome. Fixing all that's wrong in the call sites isn't
> necessarily my war, so I'm definitively fine dropping the feature and just
> getting the move constructors.

I am not trying to be difficult here or say that the change is not welcome. If that were the case, I would not waste this trying to review this.
However, I do want to make sure we're moving in the right direction and the new code looks better than previously before I can approve.

The changelog is hard for me to follow because it is unstructured and does not align with changes in the code at function level.
I think that if we introduce new code / logic, we should use it in existing code to make sure it works and so that we can more easily judge how it looks.
I pointed out some oddness and some extra if-defing that (to me at least) did not make the new code obviously better.

If you want to add a move constructor, that's probably OK as long as you actually use it for something (otherwise it is just dead code). A move constructor is small enough that you should be able to include the change with whatever new code you introduce that requires it.

> Even though I think I sending MachSemaphore over tag type is correct insofar
> as MachSemaphores should be in messages.in (they shouldn't) I'm not the
> owner of the components and I don't think I should be arguing for changing
> them..
> 
> >> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h:129
> >> +#if PLATFORM(COCOA)
> > 
> > This is more if-defing than before so it does not look as nice.
> 
> Sure, but the de-facto webkit .messages.in style is ifdefing, if I
> understand correctly. There is not a de-facto rule as to "you should
> introduce a yet another manual struct to extract the platform specific with
> possibility to make errors in decoding/encoding". Not only that, but in
> general case it is incorrect, as explained in the commit msg.
> 
> >> Source/WebKit/Shared/RemoteRenderingBackendCreationParameters.h:-35
> >> -struct RemoteRenderingBackendCreationParameters {
> > 
> > This struct abstracted away some ifdefing so I am unclear why we are removing it.
> 
> Explained in the commit msg, it is incorrect method to pass information to
> the IPC call.

I don't know that that means.

> 
> >> Source/WebKit/WebProcess/GPU/media/RemoteAudioDestinationProxy.cpp:-209
> >> -    m_renderSemaphore = nullptr;
> > 
> > Why isn't this resetting the semaphore anymore?
> 
> The semaphore send direction is changed, as explained in the commit message.

Please look into how we normally format WebKit changelogs. We often provide explanations at function level for things like that instead of having a big changelog message blob that tries to explain all the changes everywhere.

The fact is that I did try and read the changelog and it was hard to follow.

[Chris Dumez]

Comment on attachment 418446 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        Implement an encoder and decoder for WTF::MachSemaphore. Makes it possible to send it

This kind of paragraph is good for overall approach comments, not for function level changes / explanations because it is hard to match them with the diff.

> Source/WebKit/ChangeLog:52
> +        (WebKit::GPUConnectionToWebProcess::createRenderingBackend):

I would expect explanations in there, at function level for non-obvious stuff.

[Kimmo Kinnunen]

(In reply to Chris Dumez from comment #19)
> I am not trying to be difficult here or say that the change is not welcome.
> If that were the case, I would not waste this trying to review this.
> However, I do want to make sure we're moving in the right direction and the
> new code looks better than previously before I can approve.
..
> The fact is that I did try and read the changelog and it was hard to follow.

Ok, thanks for spending the time. Sorry that it was hard to follow.

I'll put this pending for a moment, move the move constructor hunks to the patch I'm working on, and later work on formatting the changelog and then come back to this issue.

[Radar WebKit Bug Importer]

<rdar://problem/73826848>

[Kimmo Kinnunen]

Created attachment 418991 [details]
Patch

[Kimmo Kinnunen]

Created attachment 418992 [details]
Patch

[Sam Weinig]

Comment on attachment 418992 [details]
Patch

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

> Source/WebKit/Platform/IPC/Semaphore.h:2
> + * Copyright (C) 2020 Apple Inc. All rights reserved.

Should be 2021!

> Source/WebKit/Platform/IPC/Semaphore.h:32
> +#if PLATFORM(COCOA)

I think I would use OS(DARWIN) here instead, as these are really OS level concepts.

> Source/WebKit/Platform/IPC/Semaphore.h:40
> +namespace IPC {
> +class Decoder;
> +class Encoder;
> +class Semaphore {

This should have some newlines after the "namespace IPC {" and before "class Semaphore {".

> Source/WebKit/Platform/IPC/Semaphore.h:48
> +    Semaphore& operator=(Semaphore&&);
> +    void encode(Encoder&) const;

We usually put a newline between these sections.

> Source/WebKit/Platform/IPC/Semaphore.h:51
> +#if PLATFORM(COCOA)

I think I would use OS(DARWIN) here instead.

> Source/WebKit/Platform/IPC/Semaphore.h:63
> +#if PLATFORM(COCOA)

I think I would use OS(DARWIN) here instead.

[Kimmo Kinnunen]

Thanks for the review! A clarification is needed, though, for future understanding:

(In reply to Sam Weinig from comment #25)
> > Source/WebKit/Platform/IPC/Semaphore.h:63
> 
> > Source/WebKit/Platform/IPC/Semaphore.h:32
> > +#if PLATFORM(COCOA)
> 
> I think I would use OS(DARWIN) here instead, as these are really OS level
> concepts.

How does this fit to the existing body of work?
Does the code go into IPC/darwin/SemaphoreDarwin.cpp?
Does the file get referenced by SourcesDarwin.txt?
How does that relate to IPC/cocoa/ConnectionCocoa.cpp, which is the implementation that transfers the thing?

Is there benefit of using OS(DARWIN) instead of PLATFORM(COCOA)?

[Sam Weinig]

(In reply to Kimmo Kinnunen from comment #26)
> Thanks for the review! A clarification is needed, though, for future
> understanding:
> 
> (In reply to Sam Weinig from comment #25)
> > > Source/WebKit/Platform/IPC/Semaphore.h:63
> > 
> > > Source/WebKit/Platform/IPC/Semaphore.h:32
> > > +#if PLATFORM(COCOA)
> > 
> > I think I would use OS(DARWIN) here instead, as these are really OS level
> > concepts.
> 
> How does this fit to the existing body of work?
> Does the code go into IPC/darwin/SemaphoreDarwin.cpp?

This seems reasonable.

> Does the file get referenced by SourcesDarwin.txt?

No, you should still use SourceCocoa.txt. The build system is per-port.

> How does that relate to IPC/cocoa/ConnectionCocoa.cpp, which is the
> implementation that transfers the thing?

I think at somepoint, that should be renamed to IPC/darwin/ConnectionDarwin.cpp, but that doesn't have to happen now.

> 
> Is there benefit of using OS(DARWIN) instead of PLATFORM(COCOA)?

The biggest benefit is that it is intended to be more clear by being more specific. Generally, we have been trying to get off of the PLATFORM() macros and onto more specific ones, usually HAVE()/USE()/ENABLE(), but also OS() where appropriate. You can see more on this by looking in the header where PLATFORM is defined:

https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/PlatformLegacy.h 

It states:

/* PLATFORM() - handles OS, operating environment, graphics API, and
   CPU. This macro will be phased out in favor of platform adaptation
   macros, policy decision macros, and top-level port definitions. */


As with many things in WebKit, these transitions are slow and not all reviewers are aware or vigilant about them.

[Kimmo Kinnunen]

Created attachment 419106 [details]
Patch

[Kimmo Kinnunen]

(In reply to Sam Weinig from comment #27)
> (In reply to Kimmo Kinnunen from comment #26)
> > Does the file get referenced by SourcesDarwin.txt?
> 
> No, you should still use SourceCocoa.txt. The build system is per-port.
> 

> The biggest benefit is that it is intended to be more clear by being more
> specific. Generally, we have been trying to get off of the PLATFORM() macros
> and onto more specific ones, usually HAVE()/USE()/ENABLE(), but also OS()
> where appropriate. You can see more on this by looking in the header where

Thanks for the explanations.

Did the suggestions.

[Kimmo Kinnunen]

Created attachment 419120 [details]
add reviewer

[EWS]

Committed r272305: <https://trac.webkit.org/changeset/272305>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419120 [details].

[Don Olmstead]

This breaks the PlayStation build.

Our pthread.h internally includes a semaphore.h and ends up including this added file instead of the one in the sdk. We develop on Windows so this is a case-insensitive.

Can you please rename the file?

[Myles C. Maxfield]

Comment on attachment 419120 [details]
add reviewer

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

> Source/WebKit/Platform/IPC/Semaphore.h:54
> +#if OS(DARWIN)

We don't usually put these kinds of platform guards on function declarations in headers. it's A-OK to declare a function that isn't defined and is never called. From that perspective, adding this #if check is unnecessary and just clutters the header with noise.

[Chris Dumez]

Comment on attachment 419120 [details]
add reviewer

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

>> Source/WebKit/Platform/IPC/Semaphore.h:54
>> +#if OS(DARWIN)
> 
> We don't usually put these kinds of platform guards on function declarations in headers. it's A-OK to declare a function that isn't defined and is never called. From that perspective, adding this #if check is unnecessary and just clutters the header with noise.

That's news to me and I actually prefer with the #ifdefs. As a port developer, you don't want to have to guess which functions are OK to call or not.

[Darin Adler]

Comment on attachment 419120 [details]
add reviewer

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

>>> Source/WebKit/Platform/IPC/Semaphore.h:54
>>> +#if OS(DARWIN)
>> 
>> We don't usually put these kinds of platform guards on function declarations in headers. it's A-OK to declare a function that isn't defined and is never called. From that perspective, adding this #if check is unnecessary and just clutters the header with noise.
> 
> That's news to me and I actually prefer with the #ifdefs. As a port developer, you don't want to have to guess which functions are OK to call or not.

I think I agree with both of you:

1) When possible, I really do like the idea of taking advantage of the fact that we don’t need to put an #if around every last declaration. A lot of time it seems excessive to carefully write #if around each time and things can be a bit more tidy.

2) When it is important for clarity, puting the #if in anyway is helpful to both document and it can even make it a compile time error rather than a runtime error to call something that’s platform specific.

[Kimmo Kinnunen]

I can of course remove #ifdefs from the calling code and the API interface header and then add NotImplemented() in the implementation for the catch-all implementation.

Having the #ifdef in the calling code would prompt question which in my mental model is answered when I look at the header and see the corresponding #ifdef. For me personally this is ok. This is currently in, for the purpose of it trading off more ifdefs with the GTK/WPE compile-time errors on unimplemented functions.

Having the #ifdef in the calling code would prompt question which would prompt another unanswered question to me. I'd then jump through few other hoops to get the answer. For me personally this is a bit inconvenient.

Not having the #ifdef in the calling code nor headers would not prompt any questions to me. It'd prompt questions only to the person trying to implement GTK/WPE part of this, since their code would compile but not work until they notice the NotImplemented logging.  For me personally this is ok. 

As a side remark, current calling code #ifdef is confusing since it #ifdefs on COCOA but then the Semaphore #ifdefs on DARWIN so in that way removing all ifdefs would side-step fixing that inconsistency spot.

[Kimmo Kinnunen]

(In reply to Kimmo Kinnunen from comment #36)
> Having the #ifdef in the calling code would prompt question which would
> prompt another unanswered question to me. I'd then jump through few other
> hoops to get the answer. For me personally this is a bit inconvenient.

This should read:
Having the #ifdef in the calling code but not in the API header ...

[Darin Adler]

It’s an aesthetic choice without an obvious answer. I know some people like code to always be super-explicit, and others like code to be tidy, and sometimes these two things are in conflict. I don’t have universal solution to that.

[Myles C. Maxfield]

I wasn't actually suggesting using NotImplemented. I was suggesting this: https://bugs.webkit.org/attachment.cgi?id=424285&action=review The definitions just don't exist on non-Darwin ports.

By the way, I believe the aesthetic choice I mention above comes comes from an email that Maciej wrote in 2014: message:[e-mail]%3E though this email doesn't exactly match the situation we're describing here.

[Myles C. Maxfield]

> 2) When it is important for clarity, puting the #if in anyway is helpful to both document and it can even make it a compile time error rather than a runtime error to call something that’s platform specific.

> As a port developer, you don't want to have to guess which functions are OK to call or not.

If the functions are declared but not defined, the error would be a link-time error rather that a compile-time error (but, importantly, not a run-time error). Both link-time errors and compile-time errors have the equal benefit that it's obvious to a port developer which functions are OK to call or not.

[Chris Dumez]

(In reply to Myles C. Maxfield from comment #40)
> > 2) When it is important for clarity, puting the #if in anyway is helpful to both document and it can even make it a compile time error rather than a runtime error to call something that’s platform specific.
> 
> > As a port developer, you don't want to have to guess which functions are OK to call or not.
> 
> If the functions are declared but not defined, the error would be a
> link-time error rather that a compile-time error (but, importantly, not a
> run-time error). Both link-time errors and compile-time errors have the
> equal benefit that it's obvious to a port developer which functions are OK
> to call or not.

What I do it use ifdefs in the header and the cpp. It has benefits to developers since they know what they can use BEFORE compiling, when actually coding.

I disagree with the feedback and with the fact that using #ifdef statements in the header is not what we usually do in the WebKit project.

[Myles C. Maxfield]

> When it is important for clarity,

Philosophically, the signal() and wait() operations on a semaphore shouldn't be platform-specific.

[Myles C. Maxfield]

> Philosophically, the signal() and wait() operations on a semaphore shouldn't
> be platform-specific.

I phrased this poorly. What I meant was "Philosophically, the fact that semaphores contain signal() and wait() operations shouldn't be platform-specific." (Of course the operations themselves will be platform-specific.)