Open Bug 1769184 Opened 3 years ago Updated 6 months ago

consider modifying xpcshell tests to support a conditioned profile

Categories

(WebExtensions :: General, enhancement, P3)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: jmaher, Unassigned)

References

Details

probably not a high priority, but we are working towards getting tests running in CI that have a more complete profile that is updated daily. Many tests are written assuming a fresh/blank profile.

browser/components/extensions/test/xpcshell/
test_ext_bookmarks.js
test_ext_topSites.js
test_ext_urlbar.js
toolkit/mozapps/extensions/test/xpcshell/
test_XPIStates.js
test_shutdown_early.js
test_sideloads_after_rebuild.js
test_system_upgrades.js
test_systemaddomstartupprefs.js
test_update_isPrivileged.js
toolkit/components/extensions/test/xpcshell/
test_ExtensionStorageSync_migration_kinto.js
test_ext_schemas_manifest_permissions.js
test_ext_schemas_privileged.js
test_ext_startup_cache_telemetry.js
test_extension_permissions_migration.js

The Bugbug bot thinks this bug should belong to the 'Core::Gecko Profiler' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → Gecko Profiler
Product: WebExtensions → Core

Sorry Bugbug, I think the "profiles" in the description refer to Firefox user profiles, not Gecko Profiler profiles.
Moving bug back to the WebExternsions product.

Component: Gecko Profiler → Untriaged
Product: Core → WebExtensions
Depends on: 1783828

What many of these tests have in common is that they involve updated extension schemas.
I suspect that the schema updated are not correctly processed because part of the schemas / registrations are stored in StartupCache.

In bug 1783828, I have shown that the consequence of this is that any change to an extension API schema is going to result in a test failure, and that the failure may disappear once the conditioned profile has been updated in 1~2 days. E.g. because of:

This is a condprof test-only issue. In Firefox, the StartupCache is wiped whenever a new version is released. That logic is triggered from https://searchfox.org/mozilla-central/rev/c0bed29d643393af6ebe77aa31455f283f169202/toolkit/xre/nsAppRunner.cpp#5178-5181, e.g. wiped when the app version info in the profile directory's compatibility.ini does not match the application version. The caller of this is ultimately XRE_main, which is called by Firefox on desktop and Android, but not by xpcshell (XRE_XPCShellMain).

To test whether the startupcache may be affecting test results, run the condprof tests with MOZ_PURGE_CACHES=1 environment variable to forcibly clear the caches.

See Also: → 1757168
Severity: -- → N/A
Priority: -- → P3
Depends on: 1795285
See Also: → 1878264
Depends on: 1891740
Depends on: 1896874
See Also: → 1766026

@Joel
The condprof tests haven't caught any meaningful failures in extension code, but are a frequent time sink, sometimes resulting in backouts, and otherwise at least resulting in time wasted on investigation. I'd like to reduce this impact on our team. I can imagine the following options:

What approach do you prefer?

Here is an analysis of the experienced failures (see bug dependencies if you need to look at individual examples).

condprof + xpcshell tests are very unrealistic, because a conditioned profile can contain a startupcache. Ordinarily, when Firefox is run, this startup cache is cleared when the version in compatibility.ini (including appBuildID) changes. This logic runs in mochitests (which use the real browser), but not xpcshell tests (because that logic is never triggered - see end of comment 3). In practice, the startupCache of a conditioned profile is cleared when run in a mochitest, because a new build of the binary results in a different buildID, so in practice the startupCache part of the conditioned profile is always considered to be out of date.

There are two classes of unrealistic failures that have been observed here:

  • extension JSON schema updates are not picked up, because that is cached in the JSON schema (comment 3 already explains this in detail).
  • .sys.mjs file changes are not picked up, and the xpcshell test runs against the cached version of the file instead of the actual implementation. An example of that is at the bottom of https://bugzilla.mozilla.org/show_bug.cgi?id=1892669#c12
Flags: needinfo?(jmaher)
See Also: → 1892669
Depends on: 1898471
Depends on: 1899260
Regressions: 1899260
No longer regressions: 1899260

thanks for the details and I am not able to come up with more options. Wiping the startupcache seems to be the most robust method, but I am not sure of how feasible that is. Why I like this is it might help keep other tests and future scenarios from running into problems.

If this isn't so feasible, then going with the removal of extensions tests from xpcshell+condprof seems the next best thing. While I would like to change the sheriffing model to ignore a small number of same failures before action (comments, backout, etc.), it would require a larger set of changes to many tools and processes.

If it is really as easy as MOZ_PURGE_CACHES=1, then I think we should go that route. Would it hurt to do this for ALL xpcshell tests?

Flags: needinfo?(jmaher)

(In reply to Joel Maher ( :jmaher ) (UTC -8) from comment #5)

thanks for the details and I am not able to come up with more options. Wiping the startupcache seems to be the most robust method, but I am not sure of how feasible that is. Why I like this is it might help keep other tests and future scenarios from running into problems.

Technically it is feasible to delete the startup cache, e.g. from add_setup(); (which checks a global var to opt out?), at least in extension tests.

A general implementation is to call Services.obs.notifyObservers(null, "startupcache-invalidate"); as seen at https://searchfox.org/mozilla-central/rev/f60bb10a5fe6936f9e9f9e8a90d52c18a0ffd818/toolkit/components/extensions/test/xpcshell/test_ext_webRequest_permission.js#30

If we want to start smaller, we could delete webext.sc.lz4 as seen at https://searchfox.org/mozilla-central/rev/f60bb10a5fe6936f9e9f9e8a90d52c18a0ffd818/toolkit/components/extensions/test/xpcshell/test_ext_geckoProfiler_schema.js#4-12

If this isn't so feasible, then going with the removal of extensions tests from xpcshell+condprof seems the next best thing.

It is feasible to wipe the caches, the question is - is there any value in condprof in xpcshell tests? Extension tests typically set up the relevant part of the environment, and are not expected to be affected by random other prefs from the conditioned browser profile.

If it is really as easy as MOZ_PURGE_CACHES=1, then I think we should go that route. Would it hurt to do this for ALL xpcshell tests?

MOZ_PURGE_CACHES does not do anything in xpcshell tests because XRE_main is not called in xpcshell (XRE_XPCShellMain is run instead).

ok, thanks for clarifying. So it seems like we either:

  1. find a way to do https://searchfox.org/mozilla-central/rev/c0bed29d643393af6ebe77aa31455f283f169202/toolkit/xre/nsAppRunner.cpp#5178-5181 from xpcshell (maybe add to XRE_XPCShellMain inside a #ifdef toggled by an env var?)
  2. disable extension tests in xpcshell+condprof (remove https://searchfox.org/mozilla-central/source/browser/components/extensions/test/xpcshell/xpcshell.toml#5)

looking into #1, it is way out of my knowledge domain and I am not sure after fiddling around for a half hour how to make this work. the code in nsAppRunner has a lot of other stuff it depends on which is where I started getting lost.

I would default to #2 if this needed to be done sooner and nobody else was keen on hacking on this.

Depends on: 1900033

In bug 1900033 I'm removing condprof from the tags in extension xpcshell tests.

Component: Untriaged → General
You need to log in before you can comment on or make changes to this bug.