1845352 - Avoid intermittent extension process termination in the middle of a xpcshell test run

Status: RESOLVED FIXED

(WebExtensions :: Android, task, P2)

Description

[Rob Wu [:robwu]]

When I remove the skip-if = os == "android" from xpcshell-remote.ini, xpcshell tests will run with the extension process enabled. After doing that, I noticed intermittent test failures on Try and locally. After investigating local adb logcat output, it turns out that these failures are caused by the extension process getting terminated by https://searchfox.org/mozilla-central/rev/d6960b4e32d09bff32865694e32384eb9bca4af5/mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/GeckoServiceChildProcess.java#177-181 . The timing coincides with when there is a moment when there are no active extension documents in the test.

Even if another extension starts loading, the scheduled termination of the extension process still continues and interrupts the startup of the extension's background page.

Interestingly, the termination is detected as an abnormal shutdown, so extension-process-crash is emitted.

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1845352 - Remove "quit-application-granted" call from AddonTestUtils.promiseShutdownManager

The "quit-application-granted" notification was originally part of
AddonTestUtils.promiseShutdownManager because XPIProvider.jsm used to
subscribe to that notification to determine whether to shut down.

A refactor in bug 1461145 replaced the notification with the use of
AsyncShutdown.quitApplicationGranted, which is already covered by
MockAsyncShutdown.quitApplicationGranted. To simulate add-on manager
shutdown, it is therefore unnecessary to trigger the notification.

Worse, the dispatch of the notification causes other parts of Firefox to
commence shutdown, with unexpected side effects. For example, the linked
bug describes unexpected extension process termination in the middle of
a xpcshell test:

• Due to bug 1845778, it was possible for 2 concurrent extension
  processes to exist. And when the extension document hosted in the
  second extension process unloaded, that process would be shut down
  because dom.ipc.keepProcessesAlive.extension=1 in tests (not 2).

• This in its turn triggers the "ipc:content-shutdown" notification that
  gets translated to "extension-process-crash". The implementation here
  assumes that there is only one extension process, so the notification
  causes ext-backgroundPage.js to sometimes interpret the termination
  as a process crash (since bug 1762225).

• On Android, the process exit is detected as a process crash, because
  "intentional" extension process shutdowns are sometimes flagged as
  abnormal shutdowns. This could happen because ContentParent.cpp has
  Android-specific logic to eagerly kill content processes to minimize
  the chance of having a dead process being around. In contrast, the
  non-Android implementation is more patient (bug 1668376).

Comment 2

[Rob Wu [:robwu]]

Linking relevant bugs referenced in the commit message at comment 1.

Note: I'm also linking bug 1827665 for visibility of the issue here. Anything that relies on "extension-process-crash" is prone to intermittent test failures when AddonTestUtils.promiseShutdownManager is called.

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1845352 - Unit tests for extension process reuse

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10ecf0a64753
Remove "quit-application-granted" call from AddonTestUtils.promiseShutdownManager r=rpl

Comment 5

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/10ecf0a64753

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d805c3c1c30
Unit tests for extension process reuse r=rpl

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/5d805c3c1c30