1876317 - TestRunnerActivity refreshExtensionList() should not drop ExtensionWrapper state with browserAction / pageAction data

Status: RESOLVED FIXED

(WebExtensions :: Android, defect, P3)

Description

[Rob Wu [:robwu]]

refreshExtensionList() is supposedly synchronizing the TestRunnerActivity's view of the extension list, with the actual state from Gecko.
It does so by discarding the full list of extensions and then storing the list of extensions: https://searchfox.org/mozilla-central/source/mobile/android/test_runner/src/main/java/org/mozilla/geckoview/test_runner/TestRunnerActivity.java#612-614

The problem with the implementation is that it drops state from ExtensionWrapper (currently: pageAction/browserAction registrations), and consequently the test may lose track of the browserAction registered at install time and cause test failures such as bug 1875948. This implementation issue has been part of the implementation from the beginning, at bug 1681360.

This is also an issue in non-test code (in Android-Components), but I will file a separate bug for that.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1876317 - Replace TestActivityRunner DebuggerDelegate with AddonManagerDelegate to selectively add/remove ExtensionWrappers.

This patch applies changes to TestActivityRunner to replace its use of
DebuggerDelegate for refresh the ExtensionWrapper instances associated
to the extensions installed on the Gecko side with a AddonManagerDelegate
instance that does create and rmeove ExtensionWrapper instances
selectively instead of dropping all of them and replace them with new
instances as it was currently being done by the refreshExtensionList
TestActivityRunner method.

The previous behavior has been already a source of unexpected test
failures in the past (see the other bugs linked to this bugzilla
issue for a few examples) and it did was also part of the reasons
for the perma-failure hit by test_ext_runtime_getContext.html
more recently (see Bug 1988445).

In Bug 1988445 the reason for the test failure was not due to the fact
that refreshExtensionList was clearing the ExtensionWrapper instances
on the TestRunnerActivity but a race between the time needed for
refreshExtensionList to get the update list of extensions metadata
(which was async on both GeckoView and Gecko sides) vs. the call
to the onBrowserAction method from the TestActivityRunner
ActionDelegate (originated from the browserAction onManifestEntry
lifecycle method), which was hitting a crash due to onBrowserAction
method trying to access the browserActions property on a non existing
entry of the mExtensions HashMap.

This patch prevents the crash hit by test_ext_runtime_getContext.html
by creating the ExtensionWrapper instances from the AddonManagerDelegate
onInstalling method (onInstalled ends up being too late and hitting the
same race and conseguent crash).

NOTE: this patch is also technically taking care of the TestRunnerActivity
part of Bug 1876329, using the 3rd of the possible approaches described in
Bug 1876329 comment 0 (but it doesn't fully cover it yet, separately
from this fix we would need to apply similar kind of changes to
TestRunner for the xpcshell test harness and Android-Components to avoid
the similar issues that installing extensions temporarily on Fenix may
also be able to hit).

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Push to try: https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=153927

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/852300392400
https://hg.mozilla.org/integration/autoland/rev/cdf505053581
Replace TestActivityRunner DebuggerDelegate with AddonManagerDelegate to selectively add/remove ExtensionWrappers. r=geckoview-reviewers,robwu,ohall

Comment 4

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/cdf505053581