Bug 177430 – Tools/Scripts/run-jsc --debugger no longer works in recent versions of Mac OS

• Component: JavaScriptCore
• Status: NEW

[Robin Morisset]

The problem is that it relies on DYLD_FRAMEWORK_PATH, and System Protection Integrity in recent versions of Mac OS reset this variable when calling "protected" executables.. such as lldb.

The solution is to run env DYLD_FRAMEWORK_PATH=... in the LLDB repl after it is launched.

[Robin Morisset]

Created attachment 321683 [details]
Patch

[Build Bot]

Comment on attachment 321683 [details]
Patch

Attachment 321683 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4649545

New failing tests:
http/tests/cache-storage/cache-representation.https.html

[Build Bot]

Created attachment 321691 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Alexey Proskuryakov]

Comment on attachment 321683 [details]
Patch

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

The direction is great. I’d like to see another iteration with code cleanup before r+’ing.

> Tools/Scripts/run-jsc:50
> +my $dyld = jscProductDir();

This is not a good variable name. dyld is the dynamic linker, and this is just its configuration option. 

Something like buildDirectory or productDirectory would be better.

> Tools/Scripts/run-jsc:51
> +$ENV{"DYLD_FRAMEWORK_PATH"} = $dyld;

This is only needed or available on Darwin, not everywhere. While technically it’s ok to add unused environment variables, we try to do it cleaner than that.

> Tools/Scripts/run-jsc:56
> +    $jsc = $debuggerCmd . " -O \"env DYLD_FRAMEWORK_PATH=" . $dyld . "\" " . File::Spec->catfile(jscProductDir(), "jsc -- ") . "@ARGV";

What will -O do for debuggers other than lldb?

[Alexey Proskuryakov]

> http/tests/cache-storage/cache-representation.https.html

This failed because the test is flaky, bug 177438.

[Robin Morisset]

Created attachment 322828 [details]
Patch

[Robin Morisset]

> > Tools/Scripts/run-jsc:50
> > +my $dyld = jscProductDir();
> 
> This is not a good variable name. dyld is the dynamic linker, and this is
> just its configuration option. 
> 
> Something like buildDirectory or productDirectory would be better.

Fixed, replaced $dyld by $productDir

> > Tools/Scripts/run-jsc:51
> > +$ENV{"DYLD_FRAMEWORK_PATH"} = $dyld;
> 
> This is only needed or available on Darwin, not everywhere. While
> technically it’s ok to add unused environment variables, we try to do it
> cleaner than that.

It seems to me that supporting non-Darwin platform with this script would be a separate bug/patch. Please note that I merely moved this command, it already happened later in the file.

> > Tools/Scripts/run-jsc:56
> > +    $jsc = $debuggerCmd . " -O \"env DYLD_FRAMEWORK_PATH=" . $dyld . "\" " . File::Spec->catfile(jscProductDir(), "jsc -- ") . "@ARGV";
> 
> What will -O do for debuggers other than lldb?

It would have caused crashes, I fixed it by putting it in a conditional.

[Build Bot]

Comment on attachment 322828 [details]
Patch

Attachment 322828 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4767178

New failing tests:
accessibility/ios-simulator/video-elements-ios.html

[Build Bot]

Created attachment 322834 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Alexey Proskuryakov]

Comment on attachment 322828 [details]
Patch

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

> Tools/ChangeLog:8
> +        The problem is caused by System Integrity Protection on recent versions of Mac OS: it removes some environment variables, including DYLD_FRAMEWORK_PATH before running lldb.

"(see <rdar://problem/21732670>)"

> Tools/Scripts/run-jsc:50
> +my $productDir = jscProductDir();

WebKit coding style strongly prefers avoiding abbreviations, so regardless of how the function is called, I recommend fully spelling out the variable name.

> Tools/Scripts/run-jsc:57
> +        $jsc = "lldb -O \"env DYLD_FRAMEWORK_PATH=" . $productDir . "\" " . File::Spec->catfile(jscProductDir(), "jsc -- ") . "@ARGV";

This needs quotes or escaping in case $productDir path has anything interesting in it (such as quote marks or spaces).

[Robin Morisset]

(In reply to Alexey Proskuryakov from comment #10)
> Comment on attachment 322828 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322828&action=review
> 
> > Tools/ChangeLog:8
> > +        The problem is caused by System Integrity Protection on recent versions of Mac OS: it removes some environment variables, including DYLD_FRAMEWORK_PATH before running lldb.
> 
> "(see <rdar://problem/21732670>)"
OK

> > Tools/Scripts/run-jsc:50
> > +my $productDir = jscProductDir();
> 
> WebKit coding style strongly prefers avoiding abbreviations, so regardless
> of how the function is called, I recommend fully spelling out the variable
> name.
OK

> > Tools/Scripts/run-jsc:57
> > +        $jsc = "lldb -O \"env DYLD_FRAMEWORK_PATH=" . $productDir . "\" " . File::Spec->catfile(jscProductDir(), "jsc -- ") . "@ARGV";
> 
> This needs quotes or escaping in case $productDir path has anything
> interesting in it (such as quote marks or spaces).
Sorry, I don't know any perl (just hit this problem while debugging jsc and tried committing a quick fix). What is the right way to escape any special characters in $productDir?

[Alexey Proskuryakov]

I'm not a Perl expert either. There appear to be many relevant links searching for "perl string escape".

[Stephan Szabo]

Given the additional complexity with quoting and the like here, it'd probably be good to consider moving to the array form of system rather than the string form to avoid the shell processing or automatic splitting.

http://perldoc.perl.org/functions/system.html
"If there is more than one argument in LIST, or if LIST is an array with more than one value, starts the program given by the first element of the list with arguments given by the rest of the list. If there is only one scalar argument, the argument is checked for shell metacharacters, and if there are any, the entire argument is passed to the system's command shell for parsing (this is /bin/sh -c on Unix platforms, but varies on other platforms). If there are no shell metacharacters in the argument, it is split into words and passed directly to execvp , which is more efficient."

Maybe something like (totally untested and unchecked):
@args = ("lldb", "-O", "env DYLD_FRAMEWORK_PATH=" . $productDir, File::Spec->catfile(jscProductDir(), "jsc"), "--", @ARGV);
...
system(@args);

[Fujii Hironori]

Here is my quoteCommand which is tested well.
https://github.com/WebKit/webkit/blob/a61a112436717b2a2797bb6d427c036dc701d0b3/Source/WebCore/bindings/scripts/generate-bindings-all.pl#L218