Bug 220529 – Reimplement WebCore::isInWebProcess() family as cross-platform by using AuxiliaryProcessInitializationParameters.processType

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Fujii Hironori]

Reimplement WebCore::isInWebProcess() families as cross-platform by using AuxiliaryProcessInitializationParameters.processType

I'm going to reimplement the following functions.

 WebCore::isInWebProcess()
 WebCore::isInGPUProcess()
 WebCore::isInNetworkProcess()

[Fujii Hironori]

Created attachment 417425 [details]
WIP patch

[Fujii Hironori]

Created attachment 417426 [details]
WIP patch

[Fujii Hironori]

Created attachment 417428 [details]
Patch

[Alex Christensen]

Comment on attachment 417428 [details]
Patch

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

> Source/WebCore/platform/RuntimeApplicationChecks.cpp:63
> +static ProcessType s_processType = ProcessType::Invalid;

static Optional<AuxiliaryProcessType> auxiliaryProcessType;

> Source/WebCore/platform/RuntimeApplicationChecks.h:35
> +enum class ProcessType : uint8_t {

AuxiliaryProcessType.
Remove Invalid.

> Source/WebKit/Shared/AuxiliaryProcess.h:48
>      enum class ProcessType : uint8_t {

Let's remove this duplicate enum.

[Fujii Hironori]

Comment on attachment 417428 [details]
Patch

Thank you for the review. I will revise the patch.

[Fujii Hironori]

Created attachment 417488 [details]
Patch

[Fujii Hironori]

Created attachment 417489 [details]
Patch

[Alex Christensen]

Comment on attachment 417489 [details]
Patch

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

> Source/WebCore/platform/RuntimeApplicationChecks.cpp:70
> +Optional<AuxiliaryProcessType> auxiliaryProcessType()

If this returns a const Optional<AuxiliaryProcessType>& you can have a function scoped static variable instead of using possibly uninitialized memory if you call checkAuxiliaryProcessType from the UI process.

[Fujii Hironori]

Comment on attachment 417489 [details]
Patch

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

>> Source/WebCore/platform/RuntimeApplicationChecks.cpp:70
>> +Optional<AuxiliaryProcessType> auxiliaryProcessType()
> 
> If this returns a const Optional<AuxiliaryProcessType>& you can have a function scoped static variable instead of using possibly uninitialized memory if you call checkAuxiliaryProcessType from the UI process.

It isn't uninitialized memory.
And, becasue Optional has a constexpr ctor, I think ctor isn't executed at runtime.
Here is the function-scope version.

const Optional<AuxiliaryProcessType>& auxiliaryProcessType()
{
    static AuxiliaryProcessType s_auxiliaryProcessType;
    return s_auxiliaryProcessType;
}

I think there is no difference.

Returning Optional<AuxiliaryProcessType>& requires dereferencing in caller side.
Returning Optional<AuxiliaryProcessType> requires dereferencing in callee side.
I didn't check disassemble code, but I expect returning Optional<AuxiliaryProcessType> is slightly efficient.

[Fujii Hironori]

Comment on attachment 417489 [details]
Patch

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

> Source/WebCore/platform/RuntimeApplicationChecks.cpp:63
> +static AuxiliaryProcessType s_auxiliaryProcessType;

Oops. I fotgot to replace the type. I got your idea. It's uninitialized.

[Fujii Hironori]

Created attachment 417496 [details]
Patch

[Fujii Hironori]

Created attachment 417497 [details]
Patch

[Chris Dumez]

Comment on attachment 417497 [details]
Patch

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

> Source/WebCore/platform/RuntimeApplicationChecks.cpp:65
> +    static Optional<AuxiliaryProcessType> s_auxiliaryProcessType;

I don't think we use prefixes for such static variables usually.

> Source/WebKit/Shared/AuxiliaryProcessMain.h:45
> +    AuxiliaryProcessInitializationParameters& m_parameters;

While this may currently be safe (I have not look deeply enough to verify), this looks dangerous and I wish we did not introduce such a fragile design that could very easily lead to use-after-free.

[Fujii Hironori]

Comment on attachment 417497 [details]
Patch

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

>> Source/WebCore/platform/RuntimeApplicationChecks.cpp:65
>> +    static Optional<AuxiliaryProcessType> s_auxiliaryProcessType;
> 
> I don't think we use prefixes for such static variables usually.

Will fix.

>> Source/WebKit/Shared/AuxiliaryProcessMain.h:45
>> +    AuxiliaryProcessInitializationParameters& m_parameters;
> 
> While this may currently be safe (I have not look deeply enough to verify), this looks dangerous and I wish we did not introduce such a fragile design that could very easily lead to use-after-free.

I agree. I will change the patch look more sane.
I will redesign AuxiliaryProcessMain in another bug ticket later. BTW, it is non-cocoa code.

[Fujii Hironori]

Created attachment 417501 [details]
Patch

[Alex Christensen]

Comment on attachment 417501 [details]
Patch

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

> Source/WebCore/platform/RuntimeApplicationChecks.h:29
> +#include <wtf/Optional.h>

I don't think this is needed.

[Fujii Hironori]

Created attachment 417552 [details]
Patch for landing

[Fujii Hironori]

Comment on attachment 417501 [details]
Patch

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

>> Source/WebCore/platform/RuntimeApplicationChecks.h:29
>> +#include <wtf/Optional.h>
> 
> I don't think this is needed.

Fixed. Thank you.

[EWS]

Committed r271452: <https://trac.webkit.org/changeset/271452>

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

[Radar WebKit Bug Importer]

<rdar://problem/73163290>