1534969 - browser tests: extension.id is unset after extension.startup() when useAddonManager is set

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[Rob Wu [:robwu]]

Both xpcshell and browser tests have an ExtensionTestUtils.loadExtension method to generate an extension for testing, with a common interface.

extension.id is one of the commonly used properties, but it is not reliable in browser tests (i.e. unset).

Minimal test case:

// Let this be a browser test
add_task(async () => {
  let ext = ExtensionTestUtils.loadExtension({
    manifest: {},
    useAddonManager: "temporary", // or "permanent"
  });
  await ext.startup();
  Assert.ok(ext.id, "Extension ID must be set");
  await ext.unnload();
});

The above assertion fails. This is because the following happens:

1. ExtensionTestUtils.loadExtension forwards the extension creation request to the main process, where ExtensionTestCommon.generate in SpecialPowersObserverAPI.js is called.
2. ExtensionTestCommon.generate returns a MockExtension (instead of an Extension) because the useAddonManager property is set.
3. When the test starts the extension, a listener to the "startup" event is added, which is responsible for forwarding the extension ID (and UUID) to the test runner.

Since the extension is a MockExtension, the extension.on("startup", ...) call is handled by MockExtension. However, the event registration in MockExtension is deferred until the extension has started: https://searchfox.org/mozilla-central/rev/89414a1df52d06cfc35529afb9a5a8542a6e4270/toolkit/components/extensions/ExtensionTestCommon.jsm#79-90,93,110-114,126,140
Consequently, the test will miss the startup event from Extension.jsm, and the extension ID is never forwarded to the test runner.

This issue is currently hidden in most tests, because ext-backgroundPage.js also triggers the "startup" event, and most tests have a background page.
I have however observed this issue multiple times during development, and after observing it once again while working on bug 1525729, I decided to investigate and fix the root cause.

Comment 1

[Rob Wu [:robwu]]

I'm going to propose a patch with minimal changes (patching MockExtension), since we're close to the merge date.

The original implementation in SpecialPowersObserverAPI used the "startup" event on the Management object, but started using the "startup" event on extension instead in https://hg.mozilla.org/mozilla-central/rev/933325344e21a683e2a08f01fd7892ec835ea992 (to fix memory leaks).
These memory leaks happened because when extension is a MockExtension (i.e. in browser tests where useAddonManager is used), the ext == extension condition never passed (because ext is an actual Extension), and the Management.off("startup", startupListener); was never called (and the extension ID is not set either).

If we're able to identify the correct extension in the "startup" event of Management, then we can use that instead of the "startup" event of extension, and resolve bug 1522973 by removing the extension.emit("startup", extension); call.

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1534969 - Ensure that extension.id/uuid is set in browser tests

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57a544b33b51
Ensure that extension.id/uuid is set in browser tests r=aswan

Comment 4

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/57a544b33b51

Comment 5

[Vlad Jiman]

Is QA validation needed for this fix? If yes, please provide some info on how to validate. Thanks!