Bug 181155 – Add a ProcessIdentifier, vended from the UI process, to each child process

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Add a ProcessIdentifier, vended from the UI process, to each child process

This will make it painless to guarantee globally unique object identifiers no matter which process generates an object identifier as long as the process identifier is included in the mix.

[Brady Eidson]

Created attachment 330209 [details]
Patch

[Brent Fulgham]

Comment on attachment 330209 [details]
Patch

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

Looks good! I had a few idle thoughts, but nothing you need to address before landing. r=me.

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=181155

Yes, please.

> Source/WebCore/platform/Process.cpp:46
> +    static bool hasInitialized;

Don't we usually prefer a dispatch_once for this kind of initialization? Maybe we don't for cross-platform code.

    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^ {
        globalIdentifier = generateObjectIdentifier<ProcessIdentifierType>();
    });

> Source/WebKit/Shared/ChildProcess.cpp:73
> +#endif

I think 'processIdentifier' is guaranteed to exist (on all platforms), since you default initialize it in the ChildProcessProxy, and always add it to the init parameters in getLaunchOptions.

> Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceEntryPoint.mm:90
> +    if (processIdentifierString.isEmpty())

Should this assert? It would be super weird if this ever failed, wouldn't it?

[Brady Eidson]

(In reply to Brent Fulgham from comment #2)
> Comment on attachment 330209 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330209&action=review
> 
> Looks good! I had a few idle thoughts, but nothing you need to address
> before landing. r=me.
> 
> > Source/WebCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=181155
> 
> Yes, please.
> 
> > Source/WebCore/platform/Process.cpp:46
> > +    static bool hasInitialized;
> 
> Don't we usually prefer a dispatch_once for this kind of initialization?
> Maybe we don't for cross-platform code.

Right, cross platform. That said I totally remembered std::once_flag and std::call_once since I uploaded this - Will use that instead.

> 
>     static dispatch_once_t onceToken;
>     dispatch_once(&onceToken, ^ {
>         globalIdentifier = generateObjectIdentifier<ProcessIdentifierType>();
>     });
> 
> > Source/WebKit/Shared/ChildProcess.cpp:73
> > +#endif
> 
> I think 'processIdentifier' is guaranteed to exist (on all platforms), since
> you default initialize it in the ChildProcessProxy, and always add it to the
> init parameters in getLaunchOptions.

It's always added to LaunchOptions in ChildProcessProxy in the UI process, that is true.
Unfortunately:
 - In the child process itself the code that populates ChildProcessInitializationParameters is platform-specific, and I can only write the Cocoa port
 - WK1 is still a thing.

> 
> > Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceEntryPoint.mm:90
> > +    if (processIdentifierString.isEmpty())
> 
> Should this assert? It would be super weird if this ever failed, wouldn't it?

That's handled in XPCServiceInitializer where the delegates to populate these things are called.

e.g.:
    if (!delegate.getProcessIdentifier(processIdentifier))
        exit(EXIT_FAILURE);

[Brady Eidson]

Created attachment 330244 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 330244 [details]
Patch

Clearing flags on attachment: 330244

Committed r226308: <https://trac.webkit.org/changeset/226308>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36261367>