Race condition between uBlock / Chromium policies

[anulman]

Prerequisites

• I verified that this is not a filter issue
  • Filter issues MUST be reported at filter issue tracker
• This is not a support issue or a question
  • Support issues and questions are handled at /r/uBlockOrigin
• I performed a cursory search of the issue tracker to avoid opening a duplicate issue
  • Your issue may already be reported
• The issue is not present after wholly disabling uBlock Origin ("uBO") in the browser
  • If the issue is still present after wholly disabling uBO in the browser, then the issue is unrelated to uBO
• I tried to reproduce the issue when...
  • uBO is the only extension
  • uBO with default lists/settings
  • using a new, unmodified browser profile
• I checked the documentation to understand that the issue I report is not a normal behavior

Description

I'm managing a uBlock deployment where we need to update the policy on each deploy because the extension ID is not guaranteed to be consistent between deploys. After we update the policy, we restart Chromium, so we know the extension is running its js/start after the policy file has been written.

Meanwhile, uBlock does not consistently apply the policy's settings. I suspect there is a race condition somewhere, where e.g. Chromium does not guarantee chrome.storage.managed has loaded all its policy files before extensions start to load.

I suspect the chrome.storage.onChanged.addListener API could help fix this easily, and also let deployment admins hot-swap policies as necessary. For example, running µBlock.restoreAdminSettings().then(µBlock.loadSelectedFilterLists) seems to work, although I bet we'd want to adopt some other js/start callbacks too.

A specific URL where the issue occurs

N/A, but here's a Loom: https://www.loom.com/share/55aa104db8d649f1aa622b137ed01cfa

Steps to Reproduce

1. Set up a policy in /etc/chromium/policies/managed/ublock_policy.json
2. uBlock doesn't load the policy unless you reload the extension, or quit and restart Chromium
3. Now quit Chromium and update the policy
4. When you reopen Chromium, uBlock may or may not have loaded the new policy data

Expected behavior:

I expected the policy to be consistently applied in Step 4, and hoped it would have been applied in Step 2

Actual behavior:

The policy was inconsistently applied (today 2/3 times; on Friday's testing 1/4 times) in Step 4, and is never applied in Step 2

Your environment

• uBlock Origin version: 1.35.2
• Browser Name and version: Chromium 90.0.4430.93 (via Mighty)
• Operating System and version: Linux

[anulman]

FWIW I'm experimenting with a patch that registers a listener and runs µBlock.restoreAdminSettings().then(µBlock.loadSelectedFilterLists) vAPI.app.restart() and will share back any results. If it works, I'd appreciate some help re: any other js/start scripts we should run; and any tests we should add/change before landing it

[anulman]

Update: these changes work like a charm; I'm going to look for a more appropriate home for them then open a PR :)

https://www.loom.com/share/a721c88a4e4c441baa548416c38849d8

[anulman]

Here is the PR; I was not able to open it against the upstream repo as I've not previously contributed mightyapp/uBlock#1

[gorhill]

Sounds like this might be related to #1547.

I can't accept the PR, using chrome.storage.onChanged has significant performance and efficiency implications: when uBO persists lists, compiled lists, memory snapshot, an event is fired with both copy of before/after the change, this means MB of memory used for no purpose at all.

Best is to identify whether there is an actual issue with uBO or the browser, and fix this in uBO, or ask the browser devs to fix it on their side if the issue is browser-side.

[gorhill]

If it's related to #1547, then I suspect merely forcing a restart without listening to storage changes is what work. If so, then it's possible to come up with forcing uBO to restart when it detects it's a first run condition, which occurs only at install time.

[gorhill]

Could you try the following changes? This will force a reload at the end of the initialization sequence when uBO detects it's a first run, and that the browser is Chromium-based.

diff --git a/src/js/start.js b/src/js/start.js
index 63285e588..026aa0809 100644
--- a/src/js/start.js
+++ b/src/js/start.js
@@ -111,11 +111,11 @@ const onVersionReady = function(lastVersion) {
     µb.redirectEngine.invalidateResourcesSelfie();
 
     const lastVersionInt = vAPI.app.intFromVersion(lastVersion);
-    if ( lastVersionInt === 0 ) { return; }
 
     // https://github.com/LiCybora/NanoDefenderFirefox/issues/196
     //   Toggle on the blocking of CSP reports by default for Firefox.
     if (
+        lastVersionInt !== 0 &&
         vAPI.webextFlavor.soup.has('firefox') &&
         lastVersionInt <= 1031003011
     ) {
@@ -332,7 +332,7 @@ try {
 
     // https://github.com/uBlockOrigin/uBlock-issues/issues/1365
     //   Wait for onCacheSettingsReady() to be fully ready.
-    await Promise.all([
+    const [ , , lastVersion ] = await Promise.all([
         µb.loadSelectedFilterLists().then(( ) => {
             log.info(`List selection ready ${Date.now()-vAPI.T0} ms after launch`);
         }),
@@ -345,6 +345,7 @@ try {
         vAPI.storage.get(createDefaultProps()).then(fetched => {
             log.info(`First fetch ready ${Date.now()-vAPI.T0} ms after launch`);
             onFirstFetchReady(fetched, adminExtra);
+            return fetched.version;
         }),
         µb.loadUserSettings().then(fetched => {
             log.info(`User settings ready ${Date.now()-vAPI.T0} ms after launch`);
@@ -354,6 +355,13 @@ try {
             log.info(`PSL ready ${Date.now()-vAPI.T0} ms after launch`);
         }),
     ]);
+
+    // https://github.com/uBlockOrigin/uBlock-issues/issues/1547
+    if ( lastVersion === '0.0.0.0' && vAPI.webextFlavor.soup.has('chromium') ) {
+        vAPI.app.restart();
+        return;
+    }
+   
 } catch (ex) {
     console.trace(ex);
 }

---

If this does not work, at least we could merge with your onChanged idea but install a listener if and only if this is a first-install run on a Chromium-based browser.

[anulman]

Thanks @gorhill! I like how your code targets Chromium specifically, thanks for sharing how you do that 😄

While I'm reasonably confident restarting like you propose will work,do you expect the perf hit to "only" be 1 additional MB, and only when uBO is either loading or the user is changing uBO settings? I wonder if that's an acceptable tradeoff; for example in my case the additional MB is negligible (especially if it's released shortly after), and the onChanged listener approach lets me hot-swap policies without having to restart users' browsers. I know at least two others have suggested this would be helpful [1]; here is an example of it working: https://www.loom.com/share/a721c88a4e4c441baa548416c38849d8

One challenge with installing the listener after a first-install run (vs. at the top of the file, like I did) is that the underlying storage may have already changed. I guess we could do a comparison against the last-fetched admin settings as well, but there we are consuming memory again!

Please let me know what you think and I'll be sure to ship the changes :)

[1] See @ghost's comment and @vtknightmare's description on the original issue

[gorhill]

do you expect the perf hit to "only" be 1 additional MB, and only when uBO is either loading or the user is changing uBO settings?

I meant to write "MBs", a memory snapshot will be many MBs.

Example of performance issue related to the use of chrome.storage.onChanged: https://bugs.chromium.org/p/chromium/issues/detail?id=1177857#c9.

So I rather avoid having to subscribe a listener which is meant for all storages just for the sake of listening to the managed one -- it's unfortunate the listener API is not more granular.

As for hot-swapping managed settings, can you also force a reload of extensions through policies once the settings have changed?

[anulman]

Oh gosh, yes copying the bigdict every time uBO updates storage.[local|sync] would be very costly. Thank you for looking out!

As for hot-swapping managed settings, can you also force a reload of extensions through policies once the settings have changed?

I don't think such a mechanism exists by policies, but I'll look into resetting using chrome.management.setEnabled instead.

I'll try applying your diff shortly!

[anulman]

Your diff (also in the above PR) works great! And chrome.management.setEnabled(uBO_id, false, () => chrome.management.setEnabled(uBO_id, true, finalCallback)) seems to work too :)

[gorhill]

If I understand correctly, you can use chrome.management.setEnabled to force a restart of uBO from a policy management tool? If so, then this is the preferred solution because there wouldn't be much use for the patch if the extension can be restarted at will from outside.

[anulman]

Close enough—in our case we deploy the browser with some extensions we force-install by policy; one of those extensions declares the chrome.management extension permission [see more]; from that extension we can call chrome.management.setEnabled(uBlockExtensionId, isEnabled, callback) 😄

I hope this discussion helps any future travelers! @gorhill, is there anything else I can do to help get this merged?