1905505 - Event page terminates immediately at event page wakeup when an extension event is triggered while the event page is suspended

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[Rob Wu [:robwu]]

The event page lifecycle is managed by a timer (IdleManager) that terminates an event page as soon as there is no detected activity.

The timer starts when the event page wakes up (resetTimer()), and is guaranteed to stop when the background stops for any reason (regular termination, process crash, extension unload) (by calling clearState()).

Whenever any activity occurs, signaled through the internal "background-script-reset-idle" event, resetTimer() is called again, which extends the lifetime by rescheduling the timer. This design assumes that background-script-reset-idle is only called when the background script exists. If the background is already stopped, calling resetTimer() will just schedule a timer that eventually calls terminateBackground().

The first step of terminateBackground() is to await bgStartupPromise, which means that if anything causes terminateBackground() to be called while the background is inactive, that it will immediately terminate the background script at the next startup of the event page.

I audited all entries that trigger "background-script-reset-idle":

• ParentAPIManager.recvAPICall resets the timer before processing an API call. Before it gets there, it looks up the context via getContextById, which throws if the context has already been unloaded. Experimentally I confirmed that unloading the background event browser via setBgStateStopped triggers a synchronous message-manager-close notification that erases the context. If we ever decide to drop the use of this notification in favor of recvProxyClosed (which is tracked in bug 1595186), then we have to carefully examine that doing so will not cause bugs like this.
• Various resets within terminateBackground() that ignore the termination request if there is another pending thing. This will never be called while the background page is stopped because there is an earlier return if the background is not running.
• EventManager's resetIdle may reset idle before dispatching an event. At the surface it seems to work all right: before calling resetIdle() there is always a shouldFire() call, which prevents the logic from running if the context has unloaded.
  • HOWEVER, this logic can also run in the content process, AND ExtensionChild's emit method forwards the message to the parent via the process message manager. When the message is received in the parent, the event is forwarded without further checks, potentially after a context has unloaded.
  • Moreover, the idle reset event is dispatched for EventManagers from any extension context, even non-background pages (a seen in the resetIdleOnEvent definition in EventManager constructor). This means that the bug can trivially be triggered, simply by triggering an event in a non-background extension context when the background is suspended.
    • This "keep event page alive when an extension event is triggered in any extension document" logic seems bizarre, and I filed bug 1905504 to fix this.

This is not a theoretical problem, it has repeatedly been observed in the wild, e.g. at https://discourse.mozilla.org/t/one-of-my-manifest-v3-addons-is-not-waking-up-when-it-should/131349/36

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1905505 - Avoid scheduling background shutdown when it is already stopped

This patch adds a bunch of checks + early returns to make sure that the
idle timer is NOT started when the background context is in the STOPPED
state. Encountering these errors is an implementation flaw, and the
errors should never been seen in practice.

The logs added in this commit should never be seen in the wild.

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1905505 - Remove unnecessary resetIdleOnEvent from child modules

This patch updates every use of EventManager in child/ext-*.js files
that can referenced from extension contexts of envType "addon_child".

Pre patch, these could trigger "background-script-reset-idle" and
result in triggering "background-script-reset-idle" in the parent
despite the background being stopped, and consequently trigger the
reported bug. The previous patch fixes the negative consequence of this
unexpected event, this patch fixes an underlying cause by removing the
triggers.

Forcing resetIdleOnEvent to false does not have any negative impact on
these modules, because all of these events are dependencies of an
EventManager in the parent, which already activates the functionality
associated with resetIdleOnEvent=true.

To minimize the likelihood of affecting unit tests, this patch keeps the
EventManager of test.onMessage at resetIdleOnEvent=true as before, and
defers to bug 1901294 for changing it to false.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fedd86509e3
Avoid scheduling background shutdown when it is already stopped r=rpl
https://hg.mozilla.org/integration/autoland/rev/bec11d4cf346
Remove unnecessary resetIdleOnEvent from child modules r=rpl

Comment 4

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/7fedd86509e3
https://hg.mozilla.org/mozilla-central/rev/bec11d4cf346

Comment 5

[Rob Wu [:robwu]]

[Tracking Requested - why for this release]: This bug causes extensions to shut down unexpectedly in some cases. This bug can be triggered easily because the conditions needed to trigger this bug are not unusual.

Comment 6

[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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Go ahead and nominate this for ESR128 approval whenever you're comfortable doing so.

Comment 8

[Rob Wu [:robwu]]

Attachment: Bug 1905505 - Avoid scheduling background shutdown when it is already stopped

This patch adds a bunch of checks + early returns to make sure that the
idle timer is NOT started when the background context is in the STOPPED
state. Encountering these errors is an implementation flaw, and the
errors should never been seen in practice.

The logs added in this commit should never be seen in the wild.

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

Comment 9

[Rob Wu [:robwu]]

Attachment: Bug 1905505 - Remove unnecessary resetIdleOnEvent from child modules

This patch updates every use of EventManager in child/ext-*.js files
that can referenced from extension contexts of envType "addon_child".

Pre patch, these could trigger "background-script-reset-idle" and
result in triggering "background-script-reset-idle" in the parent
despite the background being stopped, and consequently trigger the
reported bug. The previous patch fixes the negative consequence of this
unexpected event, this patch fixes an underlying cause by removing the
triggers.

Forcing resetIdleOnEvent to false does not have any negative impact on
these modules, because all of these events are dependencies of an
EventManager in the parent, which already activates the functionality
associated with resetIdleOnEvent=true.

To minimize the likelihood of affecting unit tests, this patch keeps the
EventManager of test.onMessage at resetIdleOnEvent=true as before, and
defers to bug 1901294 for changing it to false.

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

Comment 10

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: This bug causes extensions to shut down unexpectedly in some cases. This bug can be triggered easily because the conditions needed to trigger this bug are not unusual.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Not needed, covered by automated unit tests.
• Risk associated with taking this patch: Low
• Explanation of risk level: Change specific to extensions, issue has been thorougly examined. The fix is targeted and minimizes unwanted side effects.
• String changes made/needed: None
• Is Android affected?: yes

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/470f3b67c904
https://hg.mozilla.org/releases/mozilla-esr128/rev/e0bfbbd93ea6