1755589 - EventManager.primeListeners must perform more validation and clean-ups

Status: RESOLVED FIXED

(WebExtensions :: General, task, P2)

Description

[Rob Wu [:robwu]]

I have looked at the current uses of primeListener (bug 1748548, bug 1748547) and pending patches (bug 1748550), and found some major problems that is going to cause problems in the future.

The implementation of persistent listeners is brittle, but worked when the number of consumers was small (webRequest, proxy, and in the past runtime messaging). Since we are on the verge of using this mechanism for many more modules (bug 1587876), this ad hoc approach isn't maintainable any more, and the primitives need to be rock solid.

Specifically:

• EventManager.primeListeners assumes that the ExtensionAPI's primeListener method is infallible. When primeListener throws, the callers in ext-backgroundPage.js fail, and prevents the extension from starting altogether.

  • The implementations of primeListener in bug 1748548 may throw because it is not future-proof (see below).
  • The current patch's revision of bug 1748550 can throw if a pref is set.

• Persistent listeners are stored in addonStartup.json.lz4 (via startupData), but not properly cleaned up when needed.
  The flow is roughly:

  1. Retrieve persistent listeners.
  2. Delegate to the ExtensionAPI's primeListener method to register primed listeners.
  3. When a listener is registered by the extension during startup, mark the listener as such.
  4. When the extension's background script has finished starting, delete any listeners that were not registered.

  The persisted listeners should be ignored and cleared in the following situations:

  • Module does not support primeListener any more. DONE in bug 1624235.
  • Module is unavailable (does not exist or permission for namespace is gone) - DONE in bug 1637059.
  • When the event is not recognized by the module (i.e. when primeListener doesn't return a handler), then the listener should be erased, after looking up the handler with primeListener because the final clean-up (step 4) doesn't clear events when listener.primed is void.
    • NOTE: The current implementation does no permission/access checks on individual APIs in a namespace; it assumes that access to the namespace implies access to the individual APIs. This assumption was valid until now, but needs to be validated or at least documented for further use.

• We currently save persisted events and carry that along with all browser and extension updates (via callers of XPIProvider.persistStartupData). If we use this mechanism for all extension events, then the addonStartup.json.lz4 file may be too inflated (and the implementation would have to account for all possible values forever). Since we already skip some primed when startupBlocking is unset, it may be worth re-considering the storage location of non-startup blocking persistent listeners. The StartupCache is tied to the browser version and extension version (i.e. cleared when either version changes), which is desirable for extension updates, but possibly not so desirable for browser updates. Starting all extensions after a browser update may not be a bad thing though, especially since we already have a runtime.onInstalled event that is supposedly fired when the browser version changes.

  • related bug/comments: bug 1748535

Comment 1

[Shane Caraveo (:mixedpuppy)]

Thanks Rob,
The issue in clearPrimedListeners and primeListeners can be addressed in this bug. Module level issues can be handled separately however the change here should protect against those. I have a patch started for that.

The permission issue is specific to each module and can be delegated to the modules primeListener call.

We decided to stay with the current startup data storage as-is until we are down the road further and at a point where we feel there are no unknowns in the implementation. Then we can evaluate storage.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1755589 make prime listener and cleanup more resilient

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd827e1d18a7
make prime listener and cleanup more resilient r=rpl,robwu

Comment 4

[Cristian Tuns]

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