Bug 152545 – [Cocoa] Use the non-Development variants of XPC services for development

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

The purpose of the Development variants of the WebKit XPC services is to allow the service to link against the development WebKit dylibs rather than the system ones. This can be accomplished instead by including dyld environment load commands in engineering builds of the normal variants of the services, eliminating the need for separate services and for reexecing.

Created attachment 267887 [details]
I. Make the non-Development variants usable in engineering builds and use them

Created attachment 267888 [details]
II. Get rid of the Development variants

[Build Bot]

Comment on attachment 267887 [details]
I. Make the non-Development variants usable in engineering builds and use them

Attachment 267887 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/600357

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 267889 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

Created attachment 267890 [details]
I. Make the non-Development variants usable in engineering builds and use them

Tried to fix Yosemite crashes.

[Alexey Proskuryakov]

Comment on attachment 267890 [details]
I. Make the non-Development variants usable in engineering builds and use them

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

> Source/WebKit2/ChangeLog:3
> +        [Cocoa] Use the non-Development variants of XPC services for development

Doesn't this mean that one won't be able to practically use "process attach -waitfor -name" any more? That seems unacceptable.

(In reply to comment #6)
> Comment on attachment 267890 [details]
> I. Make the non-Development variants usable in engineering builds and use
> them
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267890&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        [Cocoa] Use the non-Development variants of XPC services for development
> 
> Doesn't this mean that one won't be able to practically use "process attach
> -waitfor -name" any more? That seems unacceptable.

I don’t think this will present a problem in practice. A typical system doesn’t have WebKit services unrelated to development starting up frequently when the user isn’t interacting with any WebKit app. Things like receiving a new mail message, or a webpage in Safari reloading itself typically don’t cause a new service to start up.

[Build Bot]

Comment on attachment 267890 [details]
I. Make the non-Development variants usable in engineering builds and use them

Attachment 267890 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/600590

New failing tests:
fast/text/international/system-language/declarative-language.html

[Build Bot]

Created attachment 267893 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Alex Christensen]

So when I'm trying to attach to my debug build of Safari, and I have the system Safari open with 5 tabs, I won't be able to look for com.apple.WebKit.WebContent.Development any more?  How will I figure out which of the 6 com.apple.WebKit.WebContents it is?

[Alexey Proskuryakov]

Comment on attachment 267890 [details]
I. Make the non-Development variants usable in engineering builds and use them

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

>>> Source/WebKit2/ChangeLog:3
>>> +        [Cocoa] Use the non-Development variants of XPC services for development
>> 
>> Doesn't this mean that one won't be able to practically use "process attach -waitfor -name" any more? That seems unacceptable.
> 
> I don’t think this will present a problem in practice. A typical system doesn’t have WebKit services unrelated to development starting up frequently when the user isn’t interacting with any WebKit app. Things like receiving a new mail message, or a webpage in Safari reloading itself typically don’t cause a new service to start up.

I didn't realize that -waitfor doesn't attach to processes that are already running, I incorrectly assumed that its behavior matched Xcode. Xcode is what I actually use for debugging most of the time, and attaching to "com.apple.WebKit.WebContent" by name will actually pick a running process instead of waiting for a new one.

Also, I'm not sure whether it's true now or in the future that WebKit services don't start up frequently. Spotlight indexers, for example, could do that pretty easily.

Created attachment 267902 [details]
A version that gives the services distinctive names in engineering builds

Created attachment 267903 [details]
A version that gives the services distinctive names in engineering builds

[Alexey Proskuryakov]

Comment on attachment 267903 [details]
A version that gives the services distinctive names in engineering builds

I think that this still breaks too many things, because production builds will no longer use .Development. As a result,
- attaching to these will be hard, and this is very relevant, especially for other teams inside Apple;
- crash logs from bots will no longer have .Development, complicating bug triage;
- user defaults will not be isolated.

If we can agree that the new behavior is better (obviously, I currently don't think so), then the patch should also update Tools/WebKitTestRunner/TestController.cpp, which hardcodes .Development in two places.

I'm not sure how this changes the story for inheriting environment variables. Will environment variables with __XPC_ still be inherited in all the cases where they are now? If not, this patch may need to update some scripts too.

(In reply to comment #14)
> Comment on attachment 267903 [details]
> A version that gives the services distinctive names in engineering builds
> 
> I think that this still breaks too many things, because production builds
> will no longer use .Development. As a result,
> - attaching to these will be hard, and this is very relevant, especially for
> other teams inside Apple;
> - crash logs from bots will no longer have .Development, complicating bug
> triage;
> - user defaults will not be isolated.

I believe that the only use production builds that may be impacted in the ways you’ve described is internal to Apple. I think I have a way to address your concerns within the tools we use at Apple. The details are outside the scope of this bug.

> If we can agree that the new behavior is better (obviously, I currently
> don't think so), then the patch should also update
> Tools/WebKitTestRunner/TestController.cpp, which hardcodes .Development in
> two places.

This won’t be necessary if what I have in mind works out.

> I'm not sure how this changes the story for inheriting environment
> variables. Will environment variables with __XPC_ still be inherited in all
> the cases where they are now? If not, this patch may need to update some
> scripts too.

The environment variable story doesn’t change.

Created attachment 268170 [details]
Use the non-Development variants of XPC services for development

[Alexey Proskuryakov]

> The environment variable story doesn’t change.

Why does the patch modify Tools/Scripts/webkitpy/port/driver.py in this case?

(In reply to comment #17)
> > The environment variable story doesn’t change.
> 
> Why does the patch modify Tools/Scripts/webkitpy/port/driver.py in this case?

There’s no change to how XPC works: to be included in the services’ environment, variables need to be __XPC_-prefixed in the environment of the parent. The Development services would also grab the "environment" array from the "re-exec" message, which is why the missing prefixed variables in driver.py went unnoticed.

[Alexey Proskuryakov]

So there is actually a huge change, right? We never needed those __XPC_ variants in practice.

In fact, using __XPC_ prefixed variables is a bad idea, we need a solution that doesn't rely on these going forward.

(In reply to comment #19)
> So there is actually a huge change, right? We never needed those __XPC_
> variants in practice.

Not sure. There’s existing use of __XPC_-prefixed environment variables in setupMacWebKitEnvironment() in webkitdirs.pm and OSXSafariDriver in osx_safari_driver.py.

> In fact, using __XPC_ prefixed variables is a bad idea, we need a solution
> that doesn't rely on these going forward.

Solution to what problem?

[Alexey Proskuryakov]

> Solution to what problem?

Not sure if you are trolling now. You never said what problem you were trying to solve with this patch, and now you are asking me about that?

[Alexey Proskuryakov]

Comment on attachment 268170 [details]
Use the non-Development variants of XPC services for development

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

> Source/WebKit2/ChangeLog:3
> +        [Cocoa] Use the non-Development variants of XPC services for development

Discussing this patch in bug comments.

[Alexey Proskuryakov]

Does this still break run-webkit-tests --additional-env-vars=...?

[Sam Weinig]

(In reply to comment #23)
> Does this still break run-webkit-tests --additional-env-vars=...?

What is the use case for this? In general, I don't think it is a good idea that the subprocesses automatically inherit any state like this. If additional state is needed we should be passing it explicitly. 

I'd really like to see this landed soon.  I think Dan has done a good job addressing your concerns, and the removal of the duplicate processes will be a good win for reducing complexity of our code.

[Alexey Proskuryakov]

Comment on attachment 268170 [details]
Use the non-Development variants of XPC services for development

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

> What is the use case for this? In general, I don't think it is a good idea that the subprocesses automatically inherit any state like this. If additional state is needed we should be passing it explicitly.

For example, --additional-env-vars="JSC_slowPathAllocsBetweenGCs" or --additional-env-vars="CFNETWORK_DIAGNOSTICS=..." This stuff is pretty relevant.

I tend to agree with your general assessment, however this patch doesn't get us closer to that goal. What actually changes with this patch is that environment is still inherited, but requires using the __XPC hack that we shouldn't be using in my opinion. So, we get most of the downsides of inheriting the environment, but lose in clarity, maintainability and perhaps in future compatibility too.

> I think Dan has done a good job addressing your concerns, and the removal of the duplicate processes will be a good win for reducing complexity of our code.

Certainly so. It's still not clear to me if the patch is an overall win, given that it trades code complexity for a few minor behavior regressions, and increases our reliance on __XPC.

> Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.mm:63
> +                int fd = xpc_dictionary_dup_fd(event, "stdout");

The change here is that production WebKit2 clients can now see stdout/stderr. This is not an improvement, because clients are generally not trusted, so giving them additional unnecessary information is not right.

I can't think of any concrete attack using this info though.

> Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.mm:101
> +    if (bootstrap) {
> +        if (xpc_object_t languages = xpc_dictionary_get_value(bootstrap.get(), "OverrideLanguages")) {
> +            NSDictionary *existingArguments = [[NSUserDefaults standardUserDefaults] volatileDomainForName:NSArgumentDomain];
> +            NSMutableDictionary *newArguments = [existingArguments mutableCopy];

Do we need an autorelease pool around this code? We didn't have it in the old location, but I'm not sure why that's OK.

Here as well, we are now giving production WebKit clients functionality that they don't need.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:3521
> -		BC3DE46615A91763008D26FC /* com.apple.WebKit.WebContent.xpc */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = com.apple.WebKit.WebContent.xpc; sourceTree = BUILT_PRODUCTS_DIR; };
> +		BC3DE46615A91763008D26FC /* com.apple.WebKit.WebContent.Development.xpc */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = com.apple.WebKit.WebContent.Development.xpc; sourceTree = BUILT_PRODUCTS_DIR; };

These changes look somewhat suspicious, but I don't know how to verify them.

[Blaze Burg]

Comment on attachment 268170 [details]
Use the non-Development variants of XPC services for development

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

> Source/WebKit2/ChangeLog:11
> +        when the WebKit dylibs are expected to be reloacted. A new build setting,

typo: 'relocated'

> Source/WebKit2/ChangeLog:36
> +        * Configurations/DatabaseService.xcconfig: Append WK_XPC_SERIVICE_SUFFIX to PRODUCT_NAME,

typo: 'SERVICE'

> Source/WebKit2/ChangeLog:38
> +        * Configurations/NetworkService.xcconfig: Append WK_XPC_SERIVICE_SUFFIX to PRODUCT_NAME, add

ditto

> Source/WebKit2/Configurations/BaseXPCService.xcconfig:45
> +WK_RELOCATABLE_FRAMEWORKS_LDFLAGS_ = $(WK_RELOCATABLE_FRAMEWORKS_LDFLAGS_NO);

I'm not familiar with this idiom (defining a suffix-less variant). Is it to guard against WK_RELOCATABLE_FRAMEWORKS being undefined?

[Darin Adler]

Comment on attachment 268170 [details]
Use the non-Development variants of XPC services for development

Seems like a good idea. But patch did not apply in EWS.

(In reply to comment #27)
> Comment on attachment 268170 [details]
> Use the non-Development variants of XPC services for development
> 
> Seems like a good idea. But patch did not apply in EWS.

It looks like svn-apply is no longer compatible with patches that svn-create-patch creates if files are being removed.

[Daniel Bates]

(In reply to comment #28)
> (In reply to comment #27)
> > Comment on attachment 268170 [details]
> > Use the non-Development variants of XPC services for development
> > 
> > Seems like a good idea. But patch did not apply in EWS.
> 
> It looks like svn-apply is no longer compatible with patches that
> svn-create-patch creates if files are being removed.

Please file a bug for this and CC me.

Created attachment 269695 [details]
Use the non-Development variants of XPC services for development

Fixed some change log typos, added autorelease pool, updated more tools.

Committed <http://trac.webkit.org/r195795>.

[Csaba Osztrogonác]

(In reply to comment #31)
> Committed <http://trac.webkit.org/r195795>.

Mac cmake buildfix landed in https://trac.webkit.org/changeset/195955