1905153 - Non-functional extension APIs at startup due to race condition, when asyncLoadModule is called before loadModule

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Rob Wu [:robwu]]

I have been investigating a report where a very simple extension (with only tabs.onCreated & tabs.onUpdated) never woke up its event page at https://discourse.mozilla.org/t/one-of-my-manifest-v3-addons-is-not-waking-up-when-it-should/131349

I audited the code to rule out many issues, and arrived at only one way that the reported issue could possibly have happened (full analysis in this document, which I'm reporting here. The exact impact of the conditions and all affected APIs is listed at the end of this report.

Race condition

If the stars are aligned poorly, a race condition could occur that causes a call to extension.apiManager.getAPI(module, extension, "addon_parent") in primeListeners to throw. This happens via loadModule (specifically _checkLoadModule, failing the module.asyncLoaded check) if asyncLoadModule has been called before for the same module name, possibly from a completely unrelated extension (as all extensions share the same SchemaAPManager instance (aka Management aka ExtensionParent.apiManager).

In the case that I analyzed, the error would have been:

Error: Module 'tabs' currently being lazily loaded

asyncLoadModule has several callers, including _callHandlers which may be called from the apiManager constructor when any add-on was disabled. The “tabs” module does indeed have a registration for “disable” (and “update”) events in ext-browser.json.
For “update”, there is an update handler that also calls asyncLoadModule, but with more levels of asynchronicity, so either the timing was very unlucky, or the “disable” case happened.

In the parent process, this race condition can happen anywhere where there is potential for loading extension API modules synchronously (after an async load). Although the implementation is shared by extension API manager in the parent and child processes, the issue is limited to the parent process because we only perform synchronous API module loading in the child process.

There are four callers of apiManager.getAPI (which performs a synchronous API load). As the breakdown below shows, the impact of this issue is currently limited to EventManager.primeListeners:

• SchemaAPIManager's emitManifestEntry, which is dead code (not called anywhere)
• CanOfAPI's loadAPI, only called by findAPIPath, which is only called from the child (never the parent)
• EventManager constructor, which is typically called from the module that implements this, so not susceptible to this race condition.
• EventManager.primeListeners, which is the primary method responsible for "priming" event listeners, i.e. registering placeholder listeners whose responsibility is to wake up the event page if the event is fired. This is this bug.

Impacted APIs

This affects all APIs whose implementation inherits from ExtensionAPIPersistent and have "events" in their module definition, which are the following APIs (along with the events that should happen at startup for any add-on to trigger this bug):

• browserAction (update, uninstall, disable)
• commands (uninstall)
• contextMenus / menus (startup)
• pageAction (update, uninstall, disable)
• tabs (update, disable) - this is the original report
• contextualIdentities (startup, also uninstall, disable, enabling because it is a "settings" module)

The impact is that if any of the add-on lifecycle events mentioned above occur at browser startup, that the event page will not wake up despite the a persisted event registration for one of the above events, or even a event registration for an API namespace that happened after the failing event registration.

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1905153 - Support loadModule after asyncLoadModule

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb040184e20b
Support loadModule after asyncLoadModule r=rpl

Comment 3

[Atila Butkovits]

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

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1905153 - Support loadModule after asyncLoadModule

Original Revision: https://phabricator.services.mozilla.com/D215155

Comment 5

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: The background script of an extension may not start at browser startup due to a race condition.
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Not reproducible, it is a race condition. But fully covered by automated tests.
• Risk associated with taking this patch: None
• Explanation of risk level: This patch removes a redundant "assertion" and updates the code path triggering that assertion to behave as expected. This behavior is verified by automated tests. The issue is very well-understood, and the fix is very targeted.
• String changes made/needed: none
• Is Android affected?: yes

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9410660 [details]
Bug 1905153 - Support loadModule after asyncLoadModule

We're in RC week already, so it's too late to get this into Beta. Will keep this on the radar in the event of an RC respin or the scheduled mid-cycle dot release otherwise.

Comment 7

[Rob Wu [:robwu]]

Can we get this in ESR128? My desire for uplifting is mainly to improve the reliability of ESR128.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Yeah, if it goes into the mid-cycle dot release I'll take it on ESR128 also. Otherwise, we can move the uplift request over there once the approval flags are created.

Comment 9

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/3fb651855a5e

Comment 10

[Rob Wu [:robwu]]

FYI the fix in 129 has been verified externally by the original reporter at https://discourse.mozilla.org/t/one-of-my-manifest-v3-addons-is-not-waking-up-when-it-should/131349/41

Edit: this comment was meant for bug 1905505 (https://bugzilla.mozilla.org/show_bug.cgi?id=1905505#c6). This bug here cannot easily be verified because it is a race condition, one that has been simulated by the automated unit test.