Bug 195393

Summary: run-benchmark should close windows between iterations
Product: WebKit Reporter: Stephanie Lewis <slewis>
Component: Tools / TestsAssignee: Stephanie Lewis <slewis>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, dewei_zhu, ews-watchlist, glenn, jbedard, lforschler, simon.fraser, slewis
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
slewis: review-
take2 dewei_zhu: review+

Description Stephanie Lewis 2019-03-06 20:26:53 PST
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.
Comment 1 Stephanie Lewis 2019-03-06 20:28:04 PST
<rdar://problem/46885583>
Comment 2 Stephanie Lewis 2019-03-06 20:30:14 PST
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.
Comment 3 Stephanie Lewis 2019-03-06 20:52:12 PST
Created attachment 363838 [details]
patch
Comment 4 EWS Watchlist 2019-03-06 20:54:16 PST
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.
Comment 5 Alexey Proskuryakov 2019-03-06 20:57:52 PST
Was there a different way to do this that no longer works?
Comment 6 Stephanie Lewis 2019-03-06 20:58:56 PST
yes.
Comment 7 dewei_zhu 2019-03-06 21:07:32 PST
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?
Comment 8 Stephanie Lewis 2019-03-06 21:11:13 PST
(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
Comment 9 Stephanie Lewis 2019-03-06 21:12:13 PST
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
Comment 10 Stephanie Lewis 2019-03-06 21:34:37 PST
Created attachment 363845 [details]
take2

Now with more preferences and less window closing
Comment 11 EWS Watchlist 2019-03-06 21:38:44 PST
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.
Comment 12 Jonathan Bedard 2019-04-30 07:52:43 PDT
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)
Comment 13 Stephanie Lewis 2019-04-30 16:29:39 PDT
Landed https://trac.webkit.org/changeset/242653/webkit