Bug 152545 - [Cocoa] Use the non-Development variants of XPC services for development
Summary: [Cocoa] Use the non-Development variants of XPC services for development
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: mitz
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-23 20:40 PST by mitz
Modified: 2016-02-01 03:39 PST (History)
14 users (show)

See Also:


Attachments
I. Make the non-Development variants usable in engineering builds and use them (15.35 KB, patch)
2015-12-23 20:52 PST, mitz
buildbot: commit-queue-
Details | Formatted Diff | Diff
II. Get rid of the Development variants (108.58 KB, patch)
2015-12-23 21:12 PST, mitz
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (153.08 KB, application/zip)
2015-12-23 21:28 PST, Build Bot
no flags Details
I. Make the non-Development variants usable in engineering builds and use them (18.84 KB, patch)
2015-12-23 22:03 PST, mitz
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (938.05 KB, application/zip)
2015-12-23 22:55 PST, Build Bot
no flags Details
A version that gives the services distinctive names in engineering builds (132.81 KB, patch)
2015-12-24 11:27 PST, mitz
no flags Details | Formatted Diff | Diff
A version that gives the services distinctive names in engineering builds (122.03 KB, patch)
2015-12-24 11:30 PST, mitz
no flags Details | Formatted Diff | Diff
Use the non-Development variants of XPC services for development (141.94 KB, patch)
2016-01-03 18:59 PST, mitz
no flags Details | Formatted Diff | Diff
Use the non-Development variants of XPC services for development (138.12 KB, patch)
2016-01-24 09:25 PST, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2015-12-23 20:40:24 PST
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.
Comment 1 mitz 2015-12-23 20:52:26 PST
Created attachment 267887 [details]
I. Make the non-Development variants usable in engineering builds and use them
Comment 2 mitz 2015-12-23 21:12:54 PST
Created attachment 267888 [details]
II. Get rid of the Development variants
Comment 3 Build Bot 2015-12-23 21:28:27 PST
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.
Comment 4 Build Bot 2015-12-23 21:28:29 PST
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
Comment 5 mitz 2015-12-23 22:03:32 PST
Created attachment 267890 [details]
I. Make the non-Development variants usable in engineering builds and use them

Tried to fix Yosemite crashes.
Comment 6 Alexey Proskuryakov 2015-12-23 22:12:09 PST
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.
Comment 7 mitz 2015-12-23 22:36:39 PST
(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.
Comment 8 Build Bot 2015-12-23 22:55:11 PST
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
Comment 9 Build Bot 2015-12-23 22:55:14 PST
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
Comment 10 Alex Christensen 2015-12-23 22:59:47 PST
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?
Comment 11 Alexey Proskuryakov 2015-12-23 23:04:09 PST
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.
Comment 12 mitz 2015-12-24 11:27:00 PST
Created attachment 267902 [details]
A version that gives the services distinctive names in engineering builds
Comment 13 mitz 2015-12-24 11:30:12 PST
Created attachment 267903 [details]
A version that gives the services distinctive names in engineering builds
Comment 14 Alexey Proskuryakov 2015-12-24 12:25:33 PST
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.
Comment 15 mitz 2015-12-26 18:55:39 PST
(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.
Comment 16 mitz 2016-01-03 18:59:31 PST
Created attachment 268170 [details]
Use the non-Development variants of XPC services for development
Comment 17 Alexey Proskuryakov 2016-01-04 09:18:50 PST
> The environment variable story doesn’t change.

Why does the patch modify Tools/Scripts/webkitpy/port/driver.py in this case?
Comment 18 mitz 2016-01-04 09:42:14 PST
(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.
Comment 19 Alexey Proskuryakov 2016-01-04 09:48:22 PST
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.
Comment 20 mitz 2016-01-04 09:56:25 PST
(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?
Comment 21 Alexey Proskuryakov 2016-01-04 10:43:31 PST
> 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?
Comment 22 Alexey Proskuryakov 2016-01-04 10:44:03 PST
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.
Comment 23 Alexey Proskuryakov 2016-01-06 16:28:28 PST
Does this still break run-webkit-tests --additional-env-vars=...?
Comment 24 Sam Weinig 2016-01-06 16:57:13 PST
(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.
Comment 25 Alexey Proskuryakov 2016-01-06 23:22:44 PST
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.
Comment 26 Brian Burg 2016-01-07 10:46:21 PST
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?
Comment 27 Darin Adler 2016-01-18 19:13:40 PST
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.
Comment 28 mitz 2016-01-18 19:15:33 PST
(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.
Comment 29 Daniel Bates 2016-01-18 19:54:59 PST
(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.
Comment 30 mitz 2016-01-24 09:25:26 PST
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.
Comment 31 mitz 2016-01-28 18:05:16 PST
Committed <http://trac.webkit.org/r195795>.
Comment 32 Csaba Osztrogonác 2016-02-01 03:39:09 PST
(In reply to comment #31)
> Committed <http://trac.webkit.org/r195795>.

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