run-safari/run-webkit-app fail to quit iOS simulator after Xcode installation

[Aakash Jain]

Sometimes iOS Simulator fails to quit and following message is displayed in logs:

syntax error: Can’t get application id “com.apple.iphonesimulator”. (-1728)
Failed to quit iOS Simulator: Bad file descriptor at OpenSource/Tools/Scripts/webkitdirs.pm line 2291.

[Aakash Jain]

see rdar://problem/26499824

[Aakash Jain]

Created attachment 281070 [details]
Proposed patch

[Daniel Bates]

Comment on attachment 281070 [details]
Proposed patch

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

> Tools/Scripts/webkitdirs.pm:77
> +       &registerIOSSimulator

Please remove this because it seems unnecessary to export this function for use outside this module. Scripts should not need to explicitly concern themselves with the Launch Services registration state of the iOS Simulator app, including registering the iOS Simulator app with Launch Services. Registering with Launch Services is an implementation detail.

> Tools/Scripts/webkitdirs.pm:2259
> +sub trim($)
> +{
> +    my $string = shift;
> +    $string =~ s/^\s+//;
> +    $string =~ s/\s+$//;
> +    return $string;
> +}

Please remove this function. It should be sufficient to remove trailing newline by making use of the built-in function chomp(). See <http://perldoc.perl.org/functions/chomp.html> for more details.

> Tools/Scripts/webkitdirs.pm:2265
> +    print "Waiting for iOS Simulator to be in state: $waitUntilState, current state: $device->{state}\n";

Please do not print such a message as it just adds noise that is emitted to standard output for all callers of this function. I know that we have existing code that prints messages with status information when launching/quitting/relaunching the simulator. We should look to remove such messages. It should be the responsibility of the calling script to decide whether to emit a message to standard output/error based on the verbosity it wants to provide to a person. In general, WebKit tools tend to follow the Unix tool convention of being silent on success and noisy on failure.

> Tools/Scripts/webkitdirs.pm:2299
> +sub registerIOSSimulator(;$)

This prototype implies that this function takes an optional scalar value. But this function does not take an optional scalar value. See <http://perldoc.perl.org/perlsub.html#Prototypes> for more details.

> Tools/Scripts/webkitdirs.pm:2301
> +    print "Registering iOS Simulator.\n";

Ditto.

> Tools/Scripts/webkitdirs.pm:2303
> +    my $simulatorPath = trim(`xcode-select --print-path`)."/Applications/Simulator.app";
> +    system("/System/Library/Frameworks/CoreServices.framework/Frameworks/LaunchServices.framework/Versions/Current/Support/lsregister", "-v", "-f", "$simulatorPath");

I would write this as:

chomp(my $developerDirectory = `xcode-select --print-path`);
my $simulatorApp = File::Spec->catfile($developerDirectory, "Applications", "Simulator.app");
my $result = system("/System/Library/Frameworks/CoreServices.framework/Frameworks/LaunchServices.framework/Versions/Current/Support/lsregister", "-f", $simulatorApp);
die "Failed to register Simulator.app with Launch Services: $result" if $result;

Notice that I explicitly omitted passing the command line flag -v, which will display progress information, to lsregister so that we do not make extra noise.

> Tools/Scripts/webkitdirs.pm:2311
> +    exitStatus(system {"osascript"} "osascript", "-e", 'tell application id "com.apple.iphonesimulator" to quit') == 0 or registerIOSSimulator;

As far as I can tell this will not fix the initial occurrence of "Can’t get application id “com.apple.iphonesimulator”. (-1728)" and only prevents such failure on subsequent calls to quitIOSSimulator(). That is, when "Can’t get application id “com.apple.iphonesimulator”. (-1728)" we do not try to quit the simulator app after registering it.

[Daniel Bates]

Comment on attachment 281070 [details]
Proposed patch

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

>> Tools/Scripts/webkitdirs.pm:2311
>> +    exitStatus(system {"osascript"} "osascript", "-e", 'tell application id "com.apple.iphonesimulator" to quit') == 0 or registerIOSSimulator;
> 
> As far as I can tell this will not fix the initial occurrence of "Can’t get application id “com.apple.iphonesimulator”. (-1728)" and only prevents such failure on subsequent calls to quitIOSSimulator(). That is, when "Can’t get application id “com.apple.iphonesimulator”. (-1728)" we do not try to quit the simulator app after registering it.

Disregard this remark.

[Daniel Bates]

 > > Tools/Scripts/webkitdirs.pm:2303
> > +    my $simulatorPath = trim(`xcode-select --print-path`)."/Applications/Simulator.app";
> > +    system("/System/Library/Frameworks/CoreServices.framework/Frameworks/LaunchServices.framework/Versions/Current/Support/lsregister", "-v", "-f", "$simulatorPath");
> 
> I would write this as:
> 
> chomp(my $developerDirectory = `xcode-select --print-path`);
> my $simulatorApp = File::Spec->catfile($developerDirectory, "Applications",
> "Simulator.app");
> my $result = system("/System/Library/Frameworks/CoreServices.framework/Frameworks/LaunchServices.framework/Versions/Current/Support/lsregister", "-f", $simulatorApp);

The last line should read:

my $result = exitStatus(system("/System/Library/Frameworks/CoreServices.framework/Frameworks/LaunchServices.framework/Versions/Current/Support/lsregister", "-f", $simulatorApp));

[Daniel Bates]

> Tools/Scripts/webkitdirs.pm:2311
> +    exitStatus(system {"osascript"} "osascript", "-e", 'tell application id "com.apple.iphonesimulator" to quit') == 0 or registerIOSSimulator;

Is it necessary that we register Simulator.app with Launch Services if osascript returns a non-zero exit status? Would it be sufficient to bail out early if osascript returns a non-zero exit status?

[Aakash Jain]

This is the behavior currently. We bail out early if osascript returns a non-zero exit status.

In various scripts, we do relaunchIOSSimulator at the beginning. If for some reason, Simulator app is not registered with LaunchServices, we bail out, causing the tests to prematurely stop. Instead if we register the Simulator app, relaunchIOSSimulator() would succeed and tests would run normally.

[Aakash Jain]

Created attachment 281095 [details]
Updated patch

Thanks for the careful review. Useful review comments, I have incorporated them. Please review again.

[Daniel Bates]

Comment on attachment 281095 [details]
Updated patch

(In reply to comment #7)
> This is the behavior currently. We bail out early if osascript returns a
> non-zero exit status.
> 
> In various scripts, we do relaunchIOSSimulator at the beginning. If for some
> reason, Simulator app is not registered with LaunchServices, we bail out,
> causing the tests to prematurely stop. 

By "bail out early" I meant return early without calling die(). That is, change the line in quitIOSSimulator() from:

exitStatus(system {"osascript"} "osascript", "-e", 'tell application id "com.apple.iphonesimulator" to quit') == 0 or die "Failed to quit iOS Simulator: $!";

to

if (exitStatus(system {"osascript"} "osascript", "-e", 'tell application id "com.apple.iphonesimulator" to quit')) {
    return;
}

> Instead if we register the Simulator app, relaunchIOSSimulator() would succeed and tests would run normally.

I propose that we make the above change to early return from quitIOSSimulator() when osascript returns a non-zero exit status and that we partially or fully revert <http://trac.webkit.org/changeset/184202> such that relaunchIOSSimulator() launches the simulator using its filesystem path instead of its bundle ID. Launching the simulator using open(1) by its filesystem path will implicitly register it with Launch Services.

[Aakash Jain]

Created attachment 281109 [details]
Updated patch

Agree. Made the changes.

[Daniel Bates]

Comment on attachment 281109 [details]
Updated patch

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

> Tools/ChangeLog:3
> +        iOS Simulator fails to quit intermittently

I updated the title of this bug to better reflect what I suspect is the cause of this failure. Please update the title in this ChangeLog.

> Tools/ChangeLog:8
> +

It would be good to explain the motivation for this fix is to make our tooling more robust if for some reason Simulator.app was not registered with LaunchServices. My guess is that this can happen if a person never launches Simulator.app after an Xcode install. It would be good to validate this hypothesis. Regardless, it does not seem ridiculous to make our tools more robust by not die()ing when it cannot quit Simulator.app because osascript exits with a non-zero exit status that is most likely caused by a lack of an entry for Simulator.app in the LaunchServices database.

> Tools/ChangeLog:12
> +        Launch iOS Simulator using complete path.
> +        Partially reverting http://trac.webkit.org/changeset/184202

Very minor: Please end each sentence with a period. I also do not see the need to have each sentence on its own line.

> Tools/ChangeLog:15
> +        Do not die if quitting ios Simulator fails, so that relaunchIOSSimulator can 
> +        still attempt to launch the simulator.

Although the change to quitIOSSimulator() has the side effect of not killing a script that called it implicitly via relaunchIOSSimulator(), the motivation for teaching quitIOSSimulator() is to handle the scenario where LaunchServices is not aware of Simulator.app for some reason. This is the reason for making quitIOSSimulator() early return when it fails to terminate Simulator.app by its bundle identifier.

> Tools/Scripts/webkitdirs.pm:-2280
> -    # FIXME: <rdar://problem/20916140> Switch to using CoreSimulator.framework for launching and quitting iOS Simulator

Is this FIXME no longer applicable?

> Tools/Scripts/webkitdirs.pm:2292
> +    if (exitStatus(system {"osascript"} "osascript", "-e", 'tell application id "com.apple.iphonesimulator" to quit')) {

This is OK as-is. We could special case this early return behavior to only happen if osascript exited unsuccessfully because it could not find com.apple.iphonesimulator (assuming this is straightforward to do) and die() otherwise so that we can diagnose other osascript failures. Though there may not be much value to do so or at least I cannot think of any other reason for osascript exiting unsuccessfully other than for something catastrophic and out of our control.

> Tools/Scripts/webkitdirs.pm:2293
> +        return;

I suggest that we add a comment above this line to explain that osascript can return a non-zero exit status if Simulator.app is not registered in LaunchServices. My guess is that this could happen if Simulator.app is never launched after a new Xcode install.

[Aakash Jain]

Comment on attachment 281109 [details]
Updated patch

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

>> Tools/ChangeLog:8
>> +
> 
> It would be good to explain the motivation for this fix is to make our tooling more robust if for some reason Simulator.app was not registered with LaunchServices. My guess is that this can happen if a person never launches Simulator.app after an Xcode install. It would be good to validate this hypothesis. Regardless, it does not seem ridiculous to make our tools more robust by not die()ing when it cannot quit Simulator.app because osascript exits with a non-zero exit status that is most likely caused by a lack of an entry for Simulator.app in the LaunchServices database.

Added.

>> Tools/ChangeLog:12
>> +        Partially reverting http://trac.webkit.org/changeset/184202
> 
> Very minor: Please end each sentence with a period. I also do not see the need to have each sentence on its own line.

Done.

>> Tools/Scripts/webkitdirs.pm:-2280
>> -    # FIXME: <rdar://problem/20916140> Switch to using CoreSimulator.framework for launching and quitting iOS Simulator
> 
> Is this FIXME no longer applicable?

good catch, added it back.

>> Tools/Scripts/webkitdirs.pm:2293
>> +        return;
> 
> I suggest that we add a comment above this line to explain that osascript can return a non-zero exit status if Simulator.app is not registered in LaunchServices. My guess is that this could happen if Simulator.app is never launched after a new Xcode install.

added

[Aakash Jain]

Created attachment 281151 [details]
Updated patch

[WebKit Commit Bot]

Comment on attachment 281151 [details]
Updated patch

Rejecting attachment 281151 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 281151, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Tools/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/1492581

[Aakash Jain]

Created attachment 281152 [details]
Updated patch

Added the "Reviewed by".

[WebKit Commit Bot]

Comment on attachment 281152 [details]
Updated patch

Clearing flags on attachment: 281152

Committed r201986: <http://trac.webkit.org/changeset/201986>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.