Bug 195393 - run-benchmark should close windows between iterations
Summary: run-benchmark should close windows between iterations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Stephanie Lewis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-06 20:26 PST by Stephanie Lewis
Modified: 2019-04-30 16:29 PDT (History)
8 users (show)

See Also:


Attachments
patch (13.59 KB, patch)
2019-03-06 20:52 PST, Stephanie Lewis
slewis: review-
Details | Formatted Diff | Diff
take2 (7.54 KB, patch)
2019-03-06 21:34 PST, Stephanie Lewis
dewei_zhu: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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