Bug 168397 – webkitpy: Simulators in the process of shutting down are considered booted

• Component: Tools / Tests
• Status: RESOLVED CONFIGURATION CHANGED

[Jonathan Bedard]

<http://trac.webkit.org/changeset/209337> allows for layout tests to be run on a currently booted simulator as opposed to starting new simulated devices for each test run.  Currently, webkitpy cannot distinguish between a simulator which is in the process of shutting down and one which is booted.

<rdar://problem/30475232> tracks this bug internally.

[Jonathan Bedard]

Created attachment 301666 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 301666 [details]
Patch

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

> Tools/Scripts/webkitpy/xcode/simulator.py:540
> +                    Simulator._shutting_down.add(device.udid)

This seems racy, as we also start and shut down simulators internally.

To fix this bug, can we just check the state in the code path that decides whether to use an existing simulator. Even so, there will still be some raciness, but at least it will be isolated to the code path where it has to be.

[Daniel Bates]

Comment on attachment 301666 [details]
Patch

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

> Tools/ChangeLog:8
> +

We should explain explicitly what the bug is. From my understanding the purpose of this patch is to fix an issue where `simctl list` supports displaying that a device is in the process of "shutting down", but the device-specific plist only reflects when the device is in the "shutdown" state. When this bug is observed, `simctl lists` shows that the device as shutting down while the device state in the device-specific plist is BOOTED. Is this correct? Regardless, please explain the issue that we are fixing.

> Tools/Scripts/webkitpy/xcode/simulator.py:346
> +    _shutting_down = set()

We should add a comment to explain that this set is used to workaround a bug between the time `simctl list` lists the device as shutdown and when the device state is recorded as shut down in the plist.

> Tools/Scripts/webkitpy/xcode/simulator.py:369
> -        'SHUTTING_DOWN'
> +        'SHUTTING DOWN',

We should not be modifying this key. From my understanding this dictionary should be kept in sync with class DeviceState and CoreSimulator/SimDevice.h. The latter is mentioned in the comment above this code.

> Tools/Scripts/webkitpy/xcode/simulator.py:441
> +        if udid in Simulator._shutting_down:
> +            if state == Simulator.DeviceState.BOOTED:
> +                return Simulator.DeviceState.SHUTTING_DOWN
> +            else:
> +                Simulator._shutting_down.remove(udid)
> +        return state

Can we make the else-case the special case as it represents when the plist state reflects the actual device state - the device is shutdown?

[Jonathan Bedard]

Comment on attachment 301666 [details]
Patch

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

>> Tools/ChangeLog:8
>> +
> 
> We should explain explicitly what the bug is. From my understanding the purpose of this patch is to fix an issue where `simctl list` supports displaying that a device is in the process of "shutting down", but the device-specific plist only reflects when the device is in the "shutdown" state. When this bug is observed, `simctl lists` shows that the device as shutting down while the device state in the device-specific plist is BOOTED. Is this correct? Regardless, please explain the issue that we are fixing.

Yes, that it correct.

>> Tools/Scripts/webkitpy/xcode/simulator.py:369
>> +        'SHUTTING DOWN',
> 
> We should not be modifying this key. From my understanding this dictionary should be kept in sync with class DeviceState and CoreSimulator/SimDevice.h. The latter is mentioned in the comment above this code.

That comment is referring to 'class DeviceState', not 'NAME_FOR_STATE.'

NAME_FOR_STATE is only used in device_state_description at the moment.  It seems to me that the strings in NAME_FOR_STATE should match the ones returned by simctl list.  This is true except for the underscore in 'SUTTING_DOWN.'

[Jonathan Bedard]

Created attachment 302298 [details]
Patch

[Daniel Bates]

(In reply to comment #4)
> Comment on attachment 301666 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301666&action=review
> 
> >> Tools/ChangeLog:8
> >> +
> > 
> > We should explain explicitly what the bug is. From my understanding the purpose of this patch is to fix an issue where `simctl list` supports displaying that a device is in the process of "shutting down", but the device-specific plist only reflects when the device is in the "shutdown" state. When this bug is observed, `simctl lists` shows that the device as shutting down while the device state in the device-specific plist is BOOTED. Is this correct? Regardless, please explain the issue that we are fixing.
> 
> Yes, that it correct.
> 
> >> Tools/Scripts/webkitpy/xcode/simulator.py:369
> >> +        'SHUTTING DOWN',
> > 
> > We should not be modifying this key. From my understanding this dictionary should be kept in sync with class DeviceState and CoreSimulator/SimDevice.h. The latter is mentioned in the comment above this code.
> 
> That comment is referring to 'class DeviceState', not 'NAME_FOR_STATE.'
> 
> NAME_FOR_STATE is only used in device_state_description at the moment.  It
> seems to me that the strings in NAME_FOR_STATE should match the ones
> returned by simctl list.  This is true except for the underscore in
> 'SUTTING_DOWN.'

From my understanding this dictionary defines a mapping that is implied by its name. Making a device state to a human readable name for logging purposes. It's not to be used for general purpose parsing. Simon Fraser may have more insight.

[Jonathan Bedard]

(In reply to comment #6)
> From my understanding this dictionary defines a mapping that is implied by
> its name. Making a device state to a human readable name for logging
> purposes. It's not to be used for general purpose parsing. Simon Fraser may
> have more insight.


That's the only thing it's used for at the moment, yes.  It doesn't seem designed for parsing, but it is also giving the exact same strings that would be needed for parsing, except the underscore in "SHUTTING_DOWN."

The current patch compares against the "BOOTED" string, so I didn't need to edit any of them.

[Alexey Proskuryakov]

Comment on attachment 302298 [details]
Patch

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

The approach seems right to me.

> Tools/Scripts/webkitpy/xcode/simulator.py:327
> +#FIXME: rdar://problem/30638036, Refactor simulator.py and simulated_device.py

There should be a space after "#". Can you explain what kind of refactoring this radar tracks? The comment is quite mysterious as is.

Maybe something like:
# FIXME: Eliminate circular dependency between Simulator and SimulatorDevice (cf. rdar://problem/30638036).
# FIXME: Use JSON output from simctl.

[Jonathan Bedard]

Comment on attachment 302298 [details]
Patch

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

>> Tools/Scripts/webkitpy/xcode/simulator.py:327
>> +#FIXME: rdar://problem/30638036, Refactor simulator.py and simulated_device.py
> 
> There should be a space after "#". Can you explain what kind of refactoring this radar tracks? The comment is quite mysterious as is.
> 
> Maybe something like:
> # FIXME: Eliminate circular dependency between Simulator and SimulatorDevice (cf. rdar://problem/30638036).
> # FIXME: Use JSON output from simctl.

I will elaborate on this.

The vagueness is deliberate because the refactor includes at least three distinct tasks, although I suspect even more than that since I believe using JSON output will hugely effect the DeviceType and runtime classes.

[Jonathan Bedard]

Created attachment 302410 [details]
Patch

[Daniel Bates]

Comment on attachment 302410 [details]
Patch

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

> Tools/Scripts/webkitpy/xcode/simulator.py:168
> -# FIXME: This class is fragile because it parses the output of the simctl command line utility, which may change.
> -#        We should find a better way to query for simulator device state and capabilities. Maybe take a similiar
> -#        approach as in webkitdirs.pm and utilize the parsed output from the device.plist files in the sub-
> -#        directories of ~/Library/Developer/CoreSimulator/Devices?
> -#        Also, simctl has the option to output in JSON format (xcrun simctl list --json).
> +# FIXME: rdar://problem/30638036, Refactor simulator.py and simulated_device.py
> +# FIXME: Eliminate circular dependency between Simulator and SimulatorDevice
> +# FIXME: Use JSON output from simctl
> +# FIXME: Better device state tracking

I feel the original comment is more informative than the 5 FIXME comments. If we want to reference <rdar://problem/30638036> then I suggest we add a remark to the existing comment.

> Tools/Scripts/webkitpy/xcode/simulator.py:438
>              if device.state == Simulator.DeviceState.BOOTED:
> -                return device
> +                for line in lines:
> +                    device_match = self.devices_re.match(line)
> +                    if (device_match and device_match.group("udid") == device.udid
> +                        and device_match.group('state').upper() == Simulator.NAME_FOR_STATE[Simulator.DeviceState.BOOTED]):
> +                            return device

I suggest that we update the device parsing logic to parse the device state and store it in each Device instance in private field called _stateWhenParsed (or _simctlState) and add a new method to class Device, say stateSlowPath or stateSlowPathUsingSimctl or stateByQueryingSimctl, that instantiates a new Simulator instance to parse the device listing, iterates over the parsed devices and return device d where d.udid == device.udid and d._simctlState == Simulator.DeviceState.BOOTED. Otherwise, this method returns None. Then we can update this code to read:

if device.state != Simulator.DeviceState.BOOTED:
    return device

# Slow path; query `simctl list` for the device state as CoreSimulator does not update the device plist (returned by Device.state - fast path)
# so as to differentiate between a device that is booted from a device that is shutting down.
if device.stateSlowPath() == Simulator.DeviceState.BOOTED:
    return device

[Jonathan Bedard]

Created attachment 303072 [details]
Patch

[Daniel Bates]

Comment on attachment 303072 [details]
Patch

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

> Tools/Scripts/webkitpy/xcode/simulated_device.py:39
> +    def __init__(self, name, udid, available, runtime, host, simctl_state=Simulator.DeviceState.SHUTDOWN):

Is it necessary to have a default value for simctl_state?

> Tools/Scripts/webkitpy/xcode/simulator.py:383
> +                                simctl_state=Simulator.name_to_state_map(device_match.group('state')))

I suggest that we have a conversion function that maps the simctl state to DeviceState instead of modifying NAME_FOR_STATE as the output of "simctl list" represents human readable output and is likely to change whereas the DeviceState constants and elements in NAME_FOR_STATE reflect the names of the constants in CoreSimulator/SimDevice.h. One way to do this is to define a dictionary that maps the simctl state to the corresponding DeviceState constant:

_DEVICE_STATE_FOR_SIMCTL_STATE = {
    'Creating': DeviceState.CREATING,
    ...
    'Shutting down': DeviceState.SHUTTING_DOWN,
}

Then we can write this line (line 383) as:

self._DEVICE_STATE_FOR_SIMCTL_STATE[device_match.group('state')]

[Jonathan Bedard]

Created attachment 304421 [details]
Patch

[Daniel Bates]

Comment on attachment 304421 [details]
Patch

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

> Tools/Scripts/webkitpy/xcode/simulator.py:185
>      devices_re = re.compile(
> -        '\s*(?P<name>.+) \((?P<udid>[^)]+)\) \((?P<state>[^)]+)\)( \((?P<availability>[^)]+)\))?')
> +        '\s*(?P<name>.+) \((?P<udid>[^)]+)\) \((?P<state>[^)]+)\)')
> +    devices_re_available = re.compile(
> +        '\s*(?P<name>.+) \((?P<udid>[^)]+)\) \((?P<state>[^)]+)\) \((?P<availability>[^)]+)\)')

Can you elaborate on this change?

> Tools/Scripts/webkitpy/xcode/simulator.py:223
> +        'Sutting Down': DeviceState.SHUTTING_DOWN,

'Sutting Down' => 'Shutting Down'?

> Tools/Scripts/webkitpy/xcode/simulator.py:380
> +            device_available = True
> +            if device_match and device_match.group('state') not in self._DEVICE_STATE_FOR_SIMCTL_STATE:
> +                device_match = self.devices_re_available.match(line)
> +                device_available = False
> +

Can you elaborate on this?

[Jonathan Bedard]

Comment on attachment 304421 [details]
Patch

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

>> Tools/Scripts/webkitpy/xcode/simulator.py:185
>> +        '\s*(?P<name>.+) \((?P<udid>[^)]+)\) \((?P<state>[^)]+)\) \((?P<availability>[^)]+)\)')
> 
> Can you elaborate on this change?

This is an issue introduced with r213164.  Originally, we attempted to be clever with a regular expression which would encompass the availability output along with the name, udid and state.  This output was untested in our unit tests (a unit test is added to test this functionality in this patch).  This approach works so long as name does not contain parenthesis as well.  With the addition of the iPad Pro, names may contain parenthesis and a line may or may not contain a closing set of parenthesis.  In order to accommodate for all of these cases, a second regular expression is needed which explicitly tests for availability.  This bug was not revealed until attempting to map state strings to DeviceState values and can't really be handled without the changes in this patch which is why I included this fix.

So far, we have not been impacted by this change because we never attempt to run boot a simulator with an unavailable SDK in automation.  Even though unavailable devices were constructed improperly (an unavailable device would return it's state as it's UDID), the fact that such devices aren't used hid this problem.

>> Tools/Scripts/webkitpy/xcode/simulator.py:380
>> +
> 
> Can you elaborate on this?

See comments on line 185.

[Jonathan Bedard]

Created attachment 304645 [details]
Patch

[Jonathan Bedard]

Per some of Dan's in-person comments, https://bugs.webkit.org/show_bug.cgi?id=169757 contains the regular-expression fix, this patch only deals with passing simctl state to SimulatedDevice.

[Daniel Bates]

Comment on attachment 304645 [details]
Patch

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

> Tools/Scripts/webkitpy/xcode/simulator_unittest.py:62
> +    iPhone 5 (1C3767FF-C268-4961-B6DA-F4F75E99EF5D) (Creating)

Where are we testing that the state of this entry is "Creating"?

> Tools/Scripts/webkitpy/xcode/simulator_unittest.py:66
> +    iPhone 6 (7BF9F835-0CEA-4EE3-BD15-A62BD9F4B691) (Shutting Down)

Where are we testing that the state of this entry is "Shutting Down"?

[Jonathan Bedard]

Comment on attachment 304645 [details]
Patch

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

>> Tools/Scripts/webkitpy/xcode/simulator_unittest.py:66
>> +    iPhone 6 (7BF9F835-0CEA-4EE3-BD15-A62BD9F4B691) (Shutting Down)
> 
> Where are we testing that the state of this entry is "Shutting Down"?

Originally, this was because I had intended form simctl_last_state to be a private implementation detail.  But currently, that is not the implementation, so you are correct, we should be testing this variable.

[Jonathan Bedard]

Created attachment 304653 [details]
Patch

[Daniel Bates]

Comment on attachment 304653 [details]
Patch

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

> Tools/Scripts/webkitpy/xcode/simulator.py:441
> +            if device.state == Simulator.DeviceState.BOOTED and device.last_simctl_state == Simulator.DeviceState.BOOTED:

This is brittle and assumes a person creates a new Simulator() before calling this function. Otherwise, device.last_simctl_state will be stale.

[Jonathan Bedard]

Created attachment 304827 [details]
Patch

[Daniel Bates]

Comment on attachment 304827 [details]
Patch

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

> Tools/Scripts/webkitpy/xcode/simulator.py:440
> +        self.refresh()

I thought we were concerned that parsing the "simctl list" output unconditionally would cause a performance regression and hence I suggested a way to implement this function with a slow and fast path in comment #11. I take it this is no longer a concern?

> Tools/Scripts/webkitpy/xcode/simulator.py:442
> +            if device.state == Simulator.DeviceState.BOOTED and device.last_simctl_state == Simulator.DeviceState.BOOTED:

Assuming we can unconditionally re-parse the "simctl list" output then we can simplify this line to read:

if device.last_simctl_state == Simulator.DeviceState.BOOTED:

[Jonathan Bedard]

(In reply to comment #24)
> Comment on attachment 304827 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304827&action=review
> 
> > Tools/Scripts/webkitpy/xcode/simulator.py:440
> > +        self.refresh()
> 
> I thought we were concerned that parsing the "simctl list" output
> unconditionally would cause a performance regression and hence I suggested a
> way to implement this function with a slow and fast path in comment #11. I
> take it this is no longer a concern?
> 
> > Tools/Scripts/webkitpy/xcode/simulator.py:442
> > +            if device.state == Simulator.DeviceState.BOOTED and device.last_simctl_state == Simulator.DeviceState.BOOTED:
> 
> Assuming we can unconditionally re-parse the "simctl list" output then we
> can simplify this line to read:
> 
> if device.last_simctl_state == Simulator.DeviceState.BOOTED:

This does cause a performance regression, but given the intention of this function, I don't think the performance regression will be impactful.  This function is intended to return a currently booted device.  Because this will, under any circumstance, require computation, we only ever call this function when setting up ports.

I don't think it is worth placing in a performance optimization.  Although if you disagree, I have a fairly simple one I could add.

[Build Bot]

Comment on attachment 304827 [details]
Patch

Attachment 304827 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3352449

New failing tests:
fast/mediastream/apply-constraints-audio.html

[Build Bot]

Created attachment 304835 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Jonathan Bedard]

Created attachment 305028 [details]
Patch

[Jonathan Bedard]

This bug is no longer relevant because of <https://bugs.webkit.org/show_bug.cgi?id=180555>.

[Radar WebKit Bug Importer]

<rdar://problem/44913834>

[Maciej Stachowiak]

Comment on attachment 305028 [details]
Patch

Unflagging and obsoleting since this bug was resolved as Config Changed