Bug 135589 – REGRESSION (r171968): run-safari --simulator fails to launch Safari on iOS

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Farler]

Now that IPhoneSimulatorNotification.pm is no longer, we should move over the runSafari mechanism over to simctl launch.

[David Farler]

Created attachment 236070 [details]
Patch

[David Farler]

Created attachment 236071 [details]
Patch

[Daniel Bates]

Comment on attachment 236071 [details]
Patch

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

I'm r-'ing this patch since it seems like we're missing logic to install the built MobileSafari. This patch also adds a function called installIOSWebKitAppInSimulator(), but doesn't use it.

> Tools/ChangeLog:3
> +        Move run-safari âsimulator over to simctl launch

Nit: "—" => "--" (notice these are two dash characters)

> Tools/ChangeLog:14
> +        (runIOSWebKitAppInSimulator):
> +        Use simctl launch.

It seems sufficient to collapse these lines into a single line that reads:

(runIOSWebKitAppInSimulator): Use simctl launch.

> Tools/ChangeLog:16
> +        (installIOSWebKitAppInSimulator):
> +        Use simctl install.

Similarly, it seems sufficient to write this line as:

(installIOSWebKitAppInSimulator): Use simctl install.

> Tools/Scripts/webkitdirs.pm:2200
> +        sleep 2;

This is OK as-is. I wish we could wait for some kind of exit status code or stdout output message instead of polling the file system every 2 seconds.

> Tools/Scripts/webkitdirs.pm:2201
> +    } while ($device->{state} ne '3');

Nit: ' => "

Unless you explicitly want to prevent string interpolation, please use double quotes instead of single quotes for string literals.

> Tools/Scripts/webkitdirs.pm:2202
> +    sleep 5;

How did you come to pick 5 seconds to sleep? I take it you picked it from experimenting. Regardless, I suggest we add an inline comment to this line that explains why we are sleeping as well as how this time was chosen. Maybe something like: Wait for services to start; 5 seconds is sufficient in practice.

> Tools/Scripts/webkitdirs.pm:2248
> +sub runIOSWebKitAppInSimulator

Did you intend to remove the prototype for this function? Do you intend to make the second argument required? If so, then we should remove line 2258 (below).

> Tools/Scripts/webkitdirs.pm:2269
> +    foreach my $key (keys(%simulatorENV)) {

Nit: We tend to omit the parentheses around keys() in our Perl code, and hence I would write this line as:

foreach my $key (keys %simulatorENV) {

> Tools/Scripts/webkitdirs.pm:2274
> +    system('xcrun', '-sdk', 'iphonesimulator', 'simctl', 'launch', $simulatedDevice->{UDID}, $appIdentifier, @$applicationArguments);

Nit: ' => "

Unless you explicitly want to prevent string interpolation, please use double quotes instead of single quotes for string literals.

> Tools/Scripts/webkitdirs.pm:2275
> +    return $?;

Instead of explicitly querying the special variable $? for the exit status I suggest that we inline the previous line into this line and use exitStatus() to retrieve the actual exit status, such that this line reads:

return exitStatus(system("xcrun", "-sdk", "iphonesimulator", "simctl", "launch", $simulatedDevice->{UDID}, $appIdentifier, @$applicationArguments));

The benefit of this approach is that it improves the readability of the code by making use of the return value of system() instead of indirectly querying for the exit status.

> Tools/Scripts/webkitdirs.pm:2292
> +sub installIOSWebKitAppInSimulator($$;$)

Where is this function used?

Also, the third argument of this function isn't used in the proposed changes.

> Tools/Scripts/webkitdirs.pm:2300
> +    system('xcrun', '-sdk', 'iphonesimulator', 'simctl', 'install', $simulatedDevice->{UDID}, $appBundle);
> +    return $?;

Similarly, I would write this as:

return exitStatus(system("xcrun", "-sdk", "iphonesimulator", "simctl", "install", $simulatedDevice->{UDID}, $appBundle);

[David Farler]

(In reply to comment #3)
> (From update of attachment 236071 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236071&action=review
> 
> I'm r-'ing this patch since it seems like we're missing logic to install the built MobileSafari. This patch also adds a function called installIOSWebKitAppInSimulator(), but doesn't use it.

I appreciate the comments. I’m fixing all of them but a few comments for some below.

> > Tools/Scripts/webkitdirs.pm:2200
> > +        sleep 2;
> 
> This is OK as-is. I wish we could wait for some kind of exit status code or stdout output message instead of polling the file system every 2 seconds.

Same here. I’ll see if we can get a synchronous simctl method to wait until all simulator services are ready, although I’m not sure if such signals even exist yet.

> > Tools/Scripts/webkitdirs.pm:2202
> > +    sleep 5;
> 
> How did you come to pick 5 seconds to sleep? I take it you picked it from experimenting. Regardless, I suggest we add an inline comment to this line that explains why we are sleeping as well as how this time was chosen. Maybe something like: Wait for services to start; 5 seconds is sufficient in practice.

This is similar to the sleep 2 - there just isn’t a fully synchronous version of this but I’ll see if we can get an enhancement for the future. This is from my experience an ok value but I’m going to up this by a second or two and add a comment.

> 
> > Tools/Scripts/webkitdirs.pm:2248
> > +sub runIOSWebKitAppInSimulator
> 
> Did you intend to remove the prototype for this function? Do you intend to make the second argument required? If so, then we should remove line 2258 (below).

I didn’t - I will add it back.

> > Tools/Scripts/webkitdirs.pm:2292
> > +sub installIOSWebKitAppInSimulator($$;$)
> 
> Where is this function used?

I forgot to add back the invocation to this. I noticed that the tool was installing a copy from the actual SDK itself, which we don’t want to do — we only want to install a MobileSafari if it is from the product directory, which I’ve added back. I renamed mobileSafariBundle to defaultSDKMobileSafariBundle.

We may need to revisit this in the future. It is pretty strongly advised not to inspect the list of installed applications for a device in the CoreSimulator API. When installing a copy of an application, it doesn’t go into the SDK, but into ~/Library/Developer/CoreSimulator/devices/$UUID/data/Containers/Bundle/Application/$APPUUID/AppName.app. So, it might be necessary to nuke contents and settings or forcefully uninstall an app by its identifier in some cases (speculating here).

> 
> Also, the third argument of this function isn't used in the proposed changes.

I’ll add back the optional simulatorOptions to pick up optional environment or arguments.

[Daniel Bates]

Created attachment 244566 [details]
Work-in-progress patch

[Daniel Bates]

Created attachment 244738 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 244738 [details]
Patch

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

r=me

> Tools/Scripts/webkitdirs.pm:2145
> +    return substr($appBundle, 0, length($simulatorApplicationsPath)) eq $simulatorApplicationsPath;

Nit:  I think there is a slightly safer way to do this with File::Spec using abs2rel() with $simulatorApplicationsPath as the base and making sure the result doesn't have any ".." directories in the path, but this is probably fine until someone complains.  :)

This does not have to be fixed to land the patch.

[Daniel Bates]

(In reply to comment #7)
> > Tools/Scripts/webkitdirs.pm:2145
> > +    return substr($appBundle, 0, length($simulatorApplicationsPath)) eq $simulatorApplicationsPath;
> 
> Nit:  I think there is a slightly safer way to do this with File::Spec using
> abs2rel() with $simulatorApplicationsPath as the base and making sure the
> result doesn't have any ".." directories in the path, but this is probably
> fine until someone complains.  :)

Instead of using File::Spec->abs2rel() and looking for "..", I'll perform a string prefix match of Cwd::realpath($appBundle) with $simulatorApplicationsPath. Cwd::realpath() will compute the canonical pathname with respect to the filesystem, resolving symbolic links and relative-path components:

sub isIOSSimulatorSystemInstalledApp($)
{
    my ($appBundle) = @_;
    my $simulatorApplicationsPath = iosSimulatorApplicationsPath();
    return substr(realpath($appBundle), 0, length($simulatorApplicationsPath)) eq $simulatorApplicationsPath;
}

[Daniel Bates]

Committed r178601: <http://trac.webkit.org/changeset/178601>