Porting webcompat interventions injections from contentScripts to scripting API

[rpl]

This PR include a draft of the changes needed to port the webcompat interventions builtin addon from the contentScripts.register API method to the scripting.registerContentScripts API method.

The motivation behind it is that the content scripts dynamically registered using contentScripts.register are expected to be unregistered automatically if the background page is destroyed, while the ones registered through the scripting.registerContentScripts API are expected to stay registered even after the background page is destroyed.

While this PR does not aim to convert the persistent background page into an event page, it may still be interesting to consider it on its own, because it would still help to make the webcompat interventions to be more resilient to extension process shutdown/crashes, because the interventions content scripts will be stay registered even after the background page is gone due to an extension process shutdown/crash.

TODO Task list:

• determine what to do with Shim cookieStoreId property (not yet supported by the scripting API, tracked by Bug 1764572)
• rebase on top of a more recent github repo tip
• apply additional changes based on the last round of review comments (change _availableInjections into just a Set of script ids, port Shim to the scripting API, remove cookieStoreId from the firebase shim)
• apply changes needed because enableContentScripts should account that enableInjection can be called for single injection from AboutCompatBroker (and it wouldn't get an array of registered content scripts)
• apply changes needed to make sure Shims also accounts for already scripting API's registered content script when the webcompat background page is loaded after a crash, like we are doing for Injections
• manually test out again the new version in a local nightly build
• determine STR for a Scenario 3: test enabling/disabling specific shims and injection from AboutCompatBroker after the webcompat background page has been reloaded automatically after a crash

---

Manual Testing STR

• export the EXPORT_MC_LOCATION env var pointing to a mozilla-central clone and run npm run jake export-mc as described in this repo README.md doc file
• build and run Firefox from that mozilla-central clone

Scenario 1: webcompat background page reloaded automatically after an extension process crash

• Open a tab to about:processes and expect the Extension process to be listed, and take note of its process ID
• Open the browser console, enable Multiprocess mode and confirm that the logs from webcompat (the ones originated from the shims.js by default)
• Navigate a tab to about:crashextensions
• Switch back to about:processes and expect the Extension process to still be listed but its process ID to be a new one
• Confirm in the browser console that the logs of a successfull webcompat background page load have been logged (e.g. the logs emitted from shims.js) and no unexpected error (e.g. registering an already registered content script) has been logged
• Navigate a tab to a page where an injection or a shim are expected to be active, e.g. http://webcompat-addon-testbed.herokuapp.com/ and confirm that the expected injection or shim are active (or check if their content script are listed in the Debugger panel as expected)

Scenario 2: webcompat background page not running anymore due to extension process crashes exceeded threshold

• open a tab on about:config and set extensions.background.disableRestartPersistentAfterCrash to true to disable the autorestart of the persistent background pages after a crash (to simulate the condition of extension process crashes exceeding the threshold and the process not being restarted anymore)
• Open a tab to about:processes and expect the Extension process to be listed
• open a new tab and navigate it to about:crashextensions
• switch back to about:processes and expect the Extension process to not be listed anymore
• Navigate a tab to a page where an injection or a shim are expected to be active, e.g. http://webcompat-addon-testbed.herokuapp.com/ and confirm that the expected injection or shim are active (or check if their content script are listed in the Debugger panel as expected)

Scenario 3: webcompat injections and shims enabled/disabled from about:compat page

• Open a tab on about:compat and confirm that the intervention for "Sites relying on window.InstallTrigger is enabled"
• Open a new tab and load one of the websites where that intervention is still applied for, e.g. https://ifcinema.institutfrancais.com/
• Expected: open the webconsole panel for the tab and confirm that InstallTrigger global is set to the expected value (the string ”"This property has been shimed for Web Compatibility reasons.")
• Switch back to the about:compat tab and disable the intervention for Sites relying on window.InstallTrigger
• Expected: Reload the test tab (e.g. https://ifcinema.institutfrancais.com/) and expect InstallTrigger global to not be defined
• Switch back to the about:compat tab and re-enable the intervention for Sites relying on window.InstallTrigger
• Expected: open the webconsole panel for the tab and confirm that InstallTrigger global is set to the expected value (the string "This property has been shimed for Web Compatibility reasons.")

[denschub]

Thanks for your work here! We'll check it out as soon as we can. :)

[rpl]

Thanks for your work here! We'll check it out as soon as we can. :)

Thanks a lot!

As a side note, in addition to the unit tests in this PR, here is an STR I used to verify the resiliency of the interventions content scripts registered from the extension background page after a process crash have triggered the background page to be destroyed (a similar STR can also be tried on Firefox for Android, but the extension process has to be enabled manually there):

• sync this extension with a mozilla-central working tree as described in this repository README.md file
• run artifact-based mach build for a Firefox Desktop instance
• start the new instance
• open a new tab on about:processes and confirm the Extension process is running
• open a new tab on a webpage to the webcompat test page: webcompat-addon-testbed.herokuapp.com
• Open the devtools webconsole panel for the tab opened on the webcompat test page
  • Expected (sanity check): executing isTestFeatureSupported from the devtools webconsole is expected to return true
• Open a new tab and navigate it to about:crashextensions
• Switch to the about:processes tab and confirm that the Extension process is gone
• Switch back to the tab opened on the webcompat test page and reload the tab
  • Expected: executing isTestFeatureSupported from the devtools webconsole is expected to still return true with the changes included in this PR (and isTestFeatureSupported to not be defined if the STR is executed on a build without the changes in this PR)

I did also run mach test-interventions with and without this changes for comparison to guide myself while drafting this PR, I plan to run those again after this PR looks got from a preliminary feedback/review pass perspective.

[rpl]

@Rob--W would you mind to also take a look at this PR and provide feedback from your perspective?

[wisniewskit]

Looks great, thanks! R+ with just a few nits/notes.

Also one question just in case we need to every worry about it: do the current ESRs support this as well, or should we avoid backporting this change if we ever update the interventions on ESR 102/115?

[Rob--W]

This is a step in the right direction: the positive aspect of this PR is that it ensures that injections continue to work even when the background unloads.

An issue is that the central management logic executes on each execution of run.js, and if the content script state exists from before the last background load, then there will be a registration error at https://searchfox.org/mozilla-central/rev/4e22b886bbd4c274f268c4a285ab7da00e95c99b/browser/extensions/webcompat/run.js#14-15

I think that it works just fine right now, other than logspam, but please confirm that.

Another nice-to-have (not required because it's pre-existing) is to register all content scripts with one registerContentScripts call, because that is much more efficient than doing multiple calls - each registration results in a broadcast IPC to all content processes, which is not cheap at all.

[rpl]

I just pushed a few more changes that I had added locally but didn't push yet.

Let's now merge this yet, I'm going to test this out locally explicitly and I want to double-check a couple more things around the AboutCompatBroker.

[rpl]

Looks great, thanks! R+ with just a few nits/notes.

Also one question just in case we need to every worry about it: do the current ESRs support this as well, or should we avoid backporting this change if we ever update the interventions on ESR 102/115?

@wisniewskit I double-checked in which version the scripting API was landed and what changes have been applied so far and left the following inline comment related to that:

// NOTE: scripting API has been introduced in Gecko 102,
// prior to Gecko 105 persistAcrossSessions option was required
// and only accepted false persistAcrossSessions, after Gecko 105
// is optional and defaults to true.

[rpl]

@wisniewskit I have applied some more fixes, but there is still some more I want to double-check around the call to getRegisteredScripts and took some notes about some more things I want to test out manually locally a bit more, and so this is not yet to be merged.

[Rob--W]

The only purpose of _activeInjections is to keep the handle around for unregistration. Since registration is now fully managed through the scripting API by id, we can remove all traces of _activeInjections.

[rpl]

@Rob--W if we port (or when we port, if we are not going to do that in this PR) the shims to the scripting API then we would still to tell from a getRegisteredContentScripts result which are the ones registered from Injections and which are registered from Shims and so I think we would still need to keep _activeInjections

If we want we could consider changing _activeInjections to be a Set of injection objects or injection ids instead of a Map if we want that.

[Rob--W]

If cookieStoreId support is causing that much hardship, then it can be removed. The Firebase shim is limited to PBM because it's only needed there.

The shim itself is already no-op when run in non-PBM (due to feature detection before overwriting). Due to the use of wrappedJSObject, in theory the page can void a global and trigger shimming. If we want to counter that, then we can modify

webcompat-addon/src/shims/firebase.js

Line 17 in 16f6e7b

(function () {

by adding

if (navigator.serviceWorker) {
  return; // SW available, not running in PBM.
}

[wisniewskit]

And actually, in hindsight this might be a good idea anyway, since if users disable serviceWorkers via about:config, this can keep the site working a bit better for them regardless of whether they're in PBM or not.

[rpl]

If we can live without cookieStoreIds, then yeah we could port Shim to the scripting API and avoid the workaround added to avoid hitting Bug 1851173 (and the big inline comment added to explain why that workaround is there).

The gain for Shim may not be as the one for Injections, because it seems that the Shim may also depend from the background page covering other parts of the expected behavior for the shim (e.g. the parts covered by the webRequest listeners), and so even if the content script would still be injected while the background page may not be around (which would be the case if there are so many crashes that we would stop to reload the background page automatically and leave the user to choose if they want to try again) the shim may not still work if it also depends from what the background page was meant to cover.

@wisniewskit based on your deeper knowledge about what the shims are expected to be doing and how they collaborate with the background page to achieve that, do you think that there may be any chance that a shim content script without the background page running could misbehave?
would you mind to point me to a couple of cases that you think would be worth for me to test out explicitly and look into more deeply to get a better picture of this part and form a more informed opinion on my side about what we would expect to happen (vs. what should happen)?

[rpl]

As a side note, looking to the firebase.js shim, that one should still work fine without the background page, and so as I was expecting some shim do not depend from the background page still being around (and I could look for specific properties in the list of shims to determine which ones depends on the background page, e.g. I suspect that the ones with needsShimHelpers or the ones redirecting urls may be part of the shims that depends also on the background page being running)

[rpl]

@wisniewskit @Rob--W I applied the remaining few changes to cover the last round of comments, than I confirmed that I needed some more to cover a few details that got missed in the previous review passes:

• Injections can be enabled/disabled from AboutCompatBroker and Injections enableContentScripts in that case is called without the array of already registered content script ids (and that was not going to work for sure without the additional changes I applied in the commit with the subject related to this)
• Shims also need to check for already registered ones, otherwise they would hit an "already registered" error from the registerContentScripts call, but unlike Injections to only call getRegisteredContentScritps during the bootup would require a larger rework and so I went for the short term option from the patch with the subject related to this part

@wisniewskit I've also run some more manual test on this version and I wrote down my STRs in the PR description in case you may also want to run a manual test locally. There is a scenario 3 related to write down the manual STRs to exercise the AboutCompatBroker part, if you have the STRs for that I may give that a try and write down Scenario 3 too.

This should now be ready for the final (and hopefully last ;-)) pass from my perspective.

@wisniewskit as a side note, as I think we may have mentioned you when we meet in Montreal, we didn't need this to land in 118, but we were aiming and hoping to get it vendored in the firefox-android repo in 119 (we may instead not strictly need it to be vendored in mozilla-central in the same timeframe, but I guess it may still be better to don't diverge the two), do you think that vendoring it in the firefox-android github repo can fit into the 119 timeframe?

[willdurand]

@wisniewskit hey, can you please take another look? Thanks!

[wisniewskit]

Looks great, thanks! Could you rebase and bump the minor version number to 118.2.0? (https://phabricator.services.mozilla.com/D186793 just landed ahead of this) I think it's time to land this in the desktop/mobile nightlies and test it out.

[rpl]

@wisniewskit thanks, I've just rebased and squashed the commits from this PR on the current repo tip, and added another commit which bumps the package and manifest versions to 118.2.0.

[wisniewskit]

I'm in the process of landing this now, via https://bugzilla.mozilla.org/show_bug.cgi?id=1853013 and mozilla-mobile/firefox-android#3624