Closed Bug 1534969 Opened 7 months ago Closed 7 months ago

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

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox-esr60 unaffected, firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: robwu, Assigned: robwu)

References

Details

Attachments

(1 file)

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.

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.

See Also: → 1522973
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/57a544b33b51
Ensure that extension.id/uuid is set in browser tests r=aswan
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

Flags: needinfo?(rob)
Flags: needinfo?(rob) → qe-verify-
You need to log in before you can comment on or make changes to this bug.