Summary: | Reimplement WebCore::isInWebProcess() family as cross-platform by using AuxiliaryProcessInitializationParameters.processType | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, cdumez, cgarcia, don.olmstead, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Fujii Hironori
2021-01-11 18:48:34 PST
Created attachment 417425 [details]
WIP patch
Created attachment 417426 [details]
WIP patch
Created attachment 417428 [details]
Patch
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. Comment on attachment 417428 [details]
Patch
Thank you for the review. I will revise the patch.
Created attachment 417488 [details]
Patch
Created attachment 417489 [details]
Patch
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. 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. 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. Created attachment 417496 [details]
Patch
Created attachment 417497 [details]
Patch
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. 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. Created attachment 417501 [details]
Patch
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. Created attachment 417552 [details]
Patch for landing
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. Committed r271452: <https://trac.webkit.org/changeset/271452> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417552 [details]. |