Bug 185315 – Check for com.apple.datamigrator before declaring simulators booted

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

When iOS simulators are being booted, they run a data migrator process, which is the process which frequently blocks the installing of apps.  We should check that there are no more data migrator processes before declaring that iOS simulators have been booted.

[Jonathan Bedard]

Created attachment 339577 [details]
Patch

[Jonathan Bedard]

Tested this on a variety of configurations which fail to install apps to simulators, it addressed the problems.

[Radar WebKit Bug Importer]

<rdar://problem/39986261>

[Daniel Bates]

Comment on attachment 339577 [details]
Patch

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

> Tools/ChangeLog:3
> +        webkitpy: Check for com.apple.datamigrator before declaring simulators booted

We should update webkitdirs::runIOSWebKitAppInSimulator() <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitdirs.pm?rev=231302#L2595> and any other tools/scripts that need to know when the simulator is ready to be used.

> Tools/Scripts/webkitpy/common/system/executive_mock.py:181
> +        self._running_pids = {'test-webkitpy': os.getpid()}

Is it necessary to call os.getpid()? Can we just hardcode a dummy canonical pid and avoid the cost of calling os.getpid()?

> Tools/Scripts/webkitpy/xcode/simulated_device.py:368
> +        SimulatedDeviceManager.wait_until_data_migration_is_done(host, deadline - time.time())

deadline - time.time() can be negative. What does that mean?

> Tools/Scripts/webkitpy/xcode/simulated_device.py:370
>          return SimulatedDeviceManager.INITIALIZED_DEVICES

I know you did not touch this code in this patch, but this function as written assumes that all devices in SimulatedDeviceManager.INITIALIZED_DEVICES have been booted by the time we get to this line even though SimulatedDeviceManager._wait_until_device_in_state() may have timed out.

> Tools/Scripts/webkitpy/xcode/simulated_device.py:424
> +        deadline = time.time() + timeout

This function assumes timeout > 0.

> Tools/Scripts/webkitpy/xcode/simulated_device.py:427
> +            time.sleep(1)

The placement of this sleep() means that this function will wait 1 second regardless of the value of timeout.

[Jonathan Bedard]

Comment on attachment 339577 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/system/executive_mock.py:181
>> +        self._running_pids = {'test-webkitpy': os.getpid()}
> 
> Is it necessary to call os.getpid()? Can we just hardcode a dummy canonical pid and avoid the cost of calling os.getpid()?

We could, but the existing code for MockExecutive does the same thing on line 75.  I don't have an opinion either way, I was just being consistent with existing code.

>> Tools/Scripts/webkitpy/xcode/simulated_device.py:368
>> +        SimulatedDeviceManager.wait_until_data_migration_is_done(host, deadline - time.time())
> 
> deadline - time.time() can be negative. What does that mean?

This would indicate that the timeout has already expired.

I don't think we need any special logic for negative timeouts, they will function similarly to a timeout of 0.

>> Tools/Scripts/webkitpy/xcode/simulated_device.py:370
>>          return SimulatedDeviceManager.INITIALIZED_DEVICES
> 
> I know you did not touch this code in this patch, but this function as written assumes that all devices in SimulatedDeviceManager.INITIALIZED_DEVICES have been booted by the time we get to this line even though SimulatedDeviceManager._wait_until_device_in_state() may have timed out.

_wait_until_device_in_state would raise an exception if it timed out.  Generally, the code in this file uses exceptions to communicate to callers that errors have occurred.

>> Tools/Scripts/webkitpy/xcode/simulated_device.py:424
>> +        deadline = time.time() + timeout
> 
> This function assumes timeout > 0.

Actually, this code work work just fine if timeout was negative.  In that case, we would get one chance to check the pid before raising an exception.

[Jonathan Bedard]

Comment on attachment 339577 [details]
Patch

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

>> Tools/ChangeLog:3
>> +        webkitpy: Check for com.apple.datamigrator before declaring simulators booted
> 
> We should update webkitdirs::runIOSWebKitAppInSimulator() <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitdirs.pm?rev=231302#L2595> and any other tools/scripts that need to know when the simulator is ready to be used.

Are we sure it's actually worth it?

I say this for a few reasons:
    - In practice, simulators are only blocked on data migrator when multiple are booted simultaneously.  This is not a work flow supported by webkitdirs::runIOSWebKitAppInSimulator()
    - webkitdirs::runIOSWebKitAppInSimulator() is not in any automated work-flows that I'm aware of. If the data migrator is stuck and a human is behind the script, it's pretty obvious.

[Jonathan Bedard]

Created attachment 339604 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 339604 [details]
Patch

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

r=me with three optional items to consider.

> Tools/Scripts/webkitdirs.pm:2457
> +    while (system("/bin/ps -eo pid,comm | /usr/bin/grep '$process'") == 0) {

Optional:  Hopefully 'com.apple.datamigrator' doesn't appear in the path to a completely different binary on macOS.  :)  Could mitigate this by checking for the process name at the end of the line:

    while (system("/bin/ps -eo pid,comm | /usr/bin/grep '" + $process + "\$'") == 0) {

NOTE: UNTESTED.  Not sure if I'm escaping the $ at the end of the regex enough.

I don't think this has to be fixed to land the patch (since behavior would be "obvious"--hang running Simulator app), but might be good to fix if it's easy to do.

> Tools/Scripts/webkitdirs.pm:2487
> +    waitUntilProcessNotRunning('com.apple.datamigrator');

Optional super-nit:  Use double-quotes for strings in Perl unless there is a reason to use single quotes.  (Python is the opposite.)

> Tools/Scripts/webkitpy/xcode/simulated_device.py:368
> +        SimulatedDeviceManager.wait_until_data_migration_is_done(host, deadline - time.time())

Optional:  You could state that negative timeouts are equivalent to 0 by using a max() function here, too:

        SimulatedDeviceManager.wait_until_data_migration_is_done(host, max(0, deadline - time.time()))

[Jonathan Bedard]

Created attachment 339734 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 339734 [details]
Patch

Clearing flags on attachment: 339734

Committed r231452: <https://trac.webkit.org/changeset/231452>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.