Bug 142145 - LayoutTestRelay: App environment variables not set for --guard-malloc or --leaks
Summary: LayoutTestRelay: App environment variables not set for --guard-malloc or --leaks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-01 07:43 PST by David Kilzer (:ddkilzer)
Modified: 2015-03-01 12:10 PST (History)
9 users (show)

See Also:

Attachments
Patch v1 (2.62 KB, patch)
2015-03-01 07:48 PST, David Kilzer (:ddkilzer) 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2015-03-01 07:43:51 PST 
When running iOS Simulator tests with --guard-malloc or --leaks, LayoutTestRelay is not passing through the appropriate environment variables, so DumpRenderTree.app and WebKitTestRunner.app are not running with these environment variables set.
Comment 1 David Kilzer (:ddkilzer) 2015-03-01 07:48:14 PST 
Created attachment 247630 [details]
Patch v1
Comment 2 Alexey Proskuryakov 2015-03-01 10:56:57 PST 
Comment on attachment 247630 [details]
Patch v1

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

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:211
> +            @"__XPC_DYLD_FRAMEWORK_PATH": productDirectory,

Adding __XPC_ variants of DYLD_FRAMEWORK_PATH and DYLD_LIBRARY_PATH doesn't seem like part of this fix. What does it fix?
Comment 3 WebKit Commit Bot 2015-03-01 10:57:22 PST 
Comment on attachment 247630 [details]
Patch v1

Clearing flags on attachment: 247630

Committed r180864: <http://trac.webkit.org/changeset/180864>
Comment 4 WebKit Commit Bot 2015-03-01 10:57:25 PST 
All reviewed patches have been landed.  Closing bug.
Comment 5 Darin Adler 2015-03-01 11:07:28 PST 
Comment on attachment 247630 [details]
Patch v1

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

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:225
> +        environmentVariables = [dictionary copy];

Is this tool compiled with ARC? If not, we need a [dictionary release].
Comment 6 David Kilzer (:ddkilzer) 2015-03-01 11:39:34 PST 
(In reply to comment #2)
> Comment on attachment 247630 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247630&action=review
> 
> > Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:211
> > +            @"__XPC_DYLD_FRAMEWORK_PATH": productDirectory,
> 
> Adding __XPC_ variants of DYLD_FRAMEWORK_PATH and DYLD_LIBRARY_PATH doesn't
> seem like part of this fix. What does it fix?

It doesn't fix any known issue.  It makes the environment for any XPC-launched process consistent with its parent.  Is that bad?
Comment 7 David Kilzer (:ddkilzer) 2015-03-01 11:41:40 PST 
(In reply to comment #5)
> Comment on attachment 247630 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247630&action=review
> 
> > Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:225
> > +        environmentVariables = [dictionary copy];
> 
> Is this tool compiled with ARC? If not, we need a [dictionary release].

Yes, it's compiled with ARC.
Comment 8 David Kilzer (:ddkilzer) 2015-03-01 11:49:56 PST 
(In reply to comment #3)
> Comment on attachment 247630 [details]
> Patch v1
> 
> Clearing flags on attachment: 247630
> 
> Committed r180864: <http://trac.webkit.org/changeset/180864>

Fix think-o:  Committed r180866: <http://trac.webkit.org/changeset/180866>
Comment 9 Alexey Proskuryakov 2015-03-01 12:10:08 PST 
> It doesn't fix any known issue.  It makes the environment for any XPC-launched process consistent with its parent.  Is that bad?

To me, it seems wrong to have code that does strange things with weird underscores and doesn't actually achieve anything. The functionality of __XPC environment variables is deeply suspect in general, and I expect that people will be looking at these lines in the future, trying to figure out if they can be removed or reimplemented.