Bug 195393 – run-benchmark should close windows between iterations

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

[Stephanie Lewis]

If old tab state isn't cleared correctly run-benchmark can open old tabs as well as the test.

3 things to fix:
a) Make sure the pref to restore old sessions isn't set
b) Terminate Safari correctly so that we don't restore state on launch because AppKit thinks Safari crashed.
c) Open Safari and close all windows in between iterations so even if Safari is killed we'll have reset the state.

[Stephanie Lewis]

<rdar://problem/46885583>

[Stephanie Lewis]

c is tricky because the solution involves AppleScript and it triggers the automation security feature that says automation can't send apple events without user approval.  

The prompt is a one time prompt so its not a big deal but this means bots can't run the apple-event code.  So we also need to add an option to disable that codepath.

[Stephanie Lewis]

Created attachment 363838 [details]
patch

[EWS Watchlist]

Attachment 363838 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:12:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:8:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Was there a different way to do this that no longer works?

[Stephanie Lewis]

yes.

Comment on attachment 363838 [details]
patch

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

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:8
> +from AppKit import NSRunningApplication

All the BrowserDriver classes will be loaded ahead. We should deferring this import still we use it otherwise, it may break on linux machines or Mac without AppKit module.

> Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:52
> +    parser.add_argument('--no-apple-events', dest='allow_apple_events', action='store_false', default='store_true', help="Calling apple events requires user interation. Disable features which use apple events. Currently used for ensuring windows are closed in between test iterations.")

The option name and dest are opposite which is confusing here. I think we should make it consistent here. Maybe call it --allow-apple-events and default to false?

[Stephanie Lewis]

(In reply to dewei_zhu from comment #7)
> Comment on attachment 363838 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363838&action=review
> 
> > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:8
> > +from AppKit import NSRunningApplication
> 
> All the BrowserDriver classes will be loaded ahead. We should deferring this
> import still we use it otherwise, it may break on linux machines or Mac
> without AppKit module.
> 
> > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:52
> > +    parser.add_argument('--no-apple-events', dest='allow_apple_events', action='store_false', default='store_true', help="Calling apple events requires user interation. Disable features which use apple events. Currently used for ensuring windows are closed in between test iterations.")
> 
> The option name and dest are opposite which is confusing here. I think we
> should make it consistent here. Maybe call it --allow-apple-events and
> default to false?

We want the default to be true and the use to have to explicitly disable the feature

[Stephanie Lewis]

Alexey pointed out some prefs in run-safari to go test

    if (checkForArgumentAndRemoveFromARGV("--no-saved-state")) {
        push @args, ("-ApplePersistenceIgnoreStateQuietly", "YES");
        # FIXME: Don't set ApplePersistenceIgnoreState once all supported OS versions respect ApplePersistenceIgnoreStateQuietly (rdar://15032886).
        push @args, ("-ApplePersistenceIgnoreState", "YES");
    }

so back to the drawing board

[Stephanie Lewis]

Created attachment 363845 [details]
take2

Now with more preferences and less window closing

[EWS Watchlist]

Attachment 363845 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:58:  [OSXBrowserDriver._terminate_processes] No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Comment on attachment 363845 [details]
take2

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

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:60
> +        for app in NSRunningApplication.runningApplicationsWithBundleIdentifier_(bundle_id):

How many apps are we expecting in this list? I think it's better to terminate them all first, then kill any which aren't terminated. It means that instead of waiting 2 * n seconds, you can just wait 2 seconds.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:-25
> -        self._safari_preferences = ["-HomePage", "about:blank", "-WarnAboutFraudulentWebsites", "0", "-ExtensionsEnabled", "0", "-ShowStatusBar", "0", "-NewWindowBehavior", "1", "-NewTabBehavior", "1"]

This seems like a change outside the scope of this bug. Why are we moving it? The only reason I can think of is if the preferences were not respected if run after the OSXBrowserDriver constructor (which seems plausible)

[Stephanie Lewis]

Landed https://trac.webkit.org/changeset/242653/webkit