1625892 - Refactor runner.js of Raptor web extension for improved async/await handling

Status: RESOLVED FIXED

(Testing :: Raptor, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Tab handling via the Tabs API for WebExtensions is mostly async. But the Raptor web extension doesn't care about that fact. Here an example when removing a tab:

https://searchfox.org/mozilla-central/rev/9c6e7500c0015a2c60be7b1b888261d95095ce27/testing/raptor/webext/raptor/runner.js#446

I will evaluate which calls need to use await to correctly wait for the underlying action.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Actually it's not just that we miss to use the await statement for several WebExtension API calls, but also that a lot of setTimeout uses can cause hangs of Raptor when exceptions are thrown within those. This is mostly the case for our application timeouts while running Raptor.

To solve all that a refactoring of the web extension is necessary. All of it is now done for runner.js and I pushed a try build for a lot of Raptor jobs across platforms and products:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=acde47b91d1ef64bdb1b25cb9ed87093dec21bbd

So far it looks pretty promising and only the tp6m-c-3 job for MotoG5 opt is showing some anomalies for a page load test. I will have to check if there is also a need to refactor pageload.js.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1625892 - [raptor] Add missing usage of await for several async WebExtension APIs.

Not awaiting the promises to be resolved will cause race conditions,
which clearly lead to test failures.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1625892 - [raptor] Refactor webextension code for opening and closing tabs.

When awaiting the promise to be resolved, the returned Tab object
will contain the neccessary reference to the id. As such no separate
listeners have to be defined. Side-effect with listeners is that those
are async and could cause race conditions in the sync program flow.

Depends on D69561

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1625892 - [raptor] Start scenario timer before collecting results.

To start the scenario timer as close as possible to the page
timeout alarm, it needs to be called before "collectResults".

Depends on D69562

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1625892 - [raptor] Don't close the very last open tab.

For GeckoView vehicles no additional tab is created during the
initialization. As such one and only open tab cannot be closed.

Depends on D69563

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1625892 - [raptor] Add updateTab method.

Move the code into its own function to simplify the code.

Depends on D69564

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1625892 - [raptor] Re-add clearing of local storage.

This has been removed by bug 1620827 without any effect. As
such it has to be re-added to be consistent with Chrome.

Depends on D69565

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1625892 - [raptor] Replace setTimeout calls for delays with sleep method.

Various setTimeout usages to delay code execution causes multi-level
code stacking. By using the newly introduced sleep function, the code
can be streamlined and simplified a lot.

Using setTimeout also prevented us for correclty handling possible
Javascript errors, so that those weren't correctly propagated up the
stack. As such no webext_error gets send to the control server, which
should force an application shutdown. Instead Raptor would hang forever
until the application timeout happens.

Depends on D69566

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1625892 - [raptor] Simplify waitForResults.

Separates the checks for completion and the final clean-up steps
to simplify the code.

Also the same interval for checking for results is used across
all the test types now.

Depends on D69567

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1625892 - [raptor] Remove usage of testTabID.

There is no need to store the current testTabID, given
that we can always query for it. Also it would prevent
unforseen behavior in case of undefined or the custom
code for the default value 0.

Because Raptor always works with the current tab, lets
query for the current tab id whenever some tab action
is triggered.

Depends on D69568

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Lets keep this bug for runner.js only. Otherwise even more commits will be added here. I will file a new bug for refactoring the pageload.js file.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9025c0ed8a06
[raptor] Add missing usage of await for several async WebExtension APIs. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/03f735885911
[raptor] Refactor webextension code for opening and closing tabs. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/034c5165ee5f
[raptor] Start scenario timer before collecting results. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/40e988e1e60c
[raptor] Don't close the very last open tab. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/59b42b7138e4
[raptor] Add updateTab method. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/36e8abbfb6c8
[raptor] Re-add clearing of local storage. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/fe9e6463fdfc
[raptor] Replace setTimeout calls for delays with sleep method. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/89a9f28cbf4c
[raptor] Simplify waitForResults. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/95231e9159d0
[raptor] Remove usage of testTabID. r=perftest-reviewers,sparky

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02cb65a8e837
Fix Eslint failure. CLOSED TREE

Comment 14

[Cristian Brindusan [:cbrindusan]]

Backed out 10 changesets (Bug 1625892) for android raptor failures.
https://hg.mozilla.org/integration/autoland/rev/e8503cd4b5fc059e664f81f2385f54f2ae79f188

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=02cb65a8e83796ec72d810cf7c15da8e8c72de66&selectedJob=296236244

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296236244&repo=autoland&lineNumber=1586

There is an Eslint failure too:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296212836&repo=autoland&lineNumber=289

Comment 15

[Cristian Brindusan [:cbrindusan]]

and also another raptor failure: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296229338&repo=autoland&lineNumber=1525

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The problem is that something went wrong with the last rebase against central and I missed to spot the following failure:

04-04 00:41:50.552 7710 7728 E WebExtensions: [JavaScript Error: "ReferenceError: can't access lexical declaration `tabId' before initialization" {file: "moz-extension://208b4918-ab53-442e-bf2f-94d48be6f2f0/runner.js" line: 233}]

(In reply to Cristian Brindusan [:cbrindusan] from comment #15)

and also another raptor failure: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296229338&repo=autoland&lineNumber=1525

That is the same problem as above. I will push again with a fix.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2a334ceab53
[raptor] Add missing usage of await for several async WebExtension APIs. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/57ad041ef416
[raptor] Refactor webextension code for opening and closing tabs. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/eb8fd48c1f18
[raptor] Start scenario timer before collecting results. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/ce75c935f554
[raptor] Don't close the very last open tab. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/132d8b210eb8
[raptor] Add updateTab method. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/9cb04362d0ef
[raptor] Re-add clearing of local storage. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/0019971a7246
[raptor] Replace setTimeout calls for delays with sleep method. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/4fd7a1493fde
[raptor] Simplify waitForResults. r=perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/787a82df4966
[raptor] Remove usage of testTabID. r=perftest-reviewers,sparky

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d2a334ceab53
https://hg.mozilla.org/mozilla-central/rev/57ad041ef416
https://hg.mozilla.org/mozilla-central/rev/eb8fd48c1f18
https://hg.mozilla.org/mozilla-central/rev/ce75c935f554
https://hg.mozilla.org/mozilla-central/rev/132d8b210eb8
https://hg.mozilla.org/mozilla-central/rev/9cb04362d0ef
https://hg.mozilla.org/mozilla-central/rev/0019971a7246
https://hg.mozilla.org/mozilla-central/rev/4fd7a1493fde
https://hg.mozilla.org/mozilla-central/rev/787a82df4966

Comment 19

[Alexandru Ionescu (needinfo me) [:alexandrui]]

This is the revision that introduced the regression: [raptor] Refactor webextension code for opening and closing tabs.