[FireMonkey] Auto-Update routine is badly implemented

[pintassilgo]

Sorry if I misunderstood something, but from what I read in the code:

1. At FM startup, the extension loops the scripts list and push those eligible to update to an array. This is the only moment when FM checks if Date.now() > autoUpdateLast + days.
2. If the array is not empty, FM starts listening to browser.idle.onStateChanged.
3. When browser is idle, check for updates is processed and autoUpdateLast is updated.

Problems:

• The defined terminateRemoteUpdate() is unused.
• If FM gets to 3 above, it will check for updates every time the browser is idle, as it no longer checks for autoUpdateLast.
• If you install a new script or remove some, it doesn't matter, the array with updateable scripts is never touched again after FM startup.

FM should use browser.alarms to schedule update checks.

[erosman]

The defined terminateRemoteUpdate() is unused.

Oh.. a typo :man_facepalming: ... when function name was changed in this.cache[0] || this.terminate(); ... fixed. :+1:

If FM gets to 3 above, it will check for updates every time the browser is idle, as it no longer checks for autoUpdateLast.

Until the cache array is cleared.

If you install a new script or remove some, it doesn't matter, the array with updateable scripts is never touched again after FM startup.

After script install, that script doesn't need to be updated immediately. After script removal, that script doesn't need to be updated ever.

FM should use browser.alarms to schedule update checks.

browser.alarms only works when browser is open and is cleared when it is closed. User who shut down Firefox or restart often would reset the timer.

Let me see what improvements can be made.

[pintassilgo]

Until the cache array is cleared.

Right, I missed cache.splice(), sorry.

After script install, that script doesn't need to be updated immediately.

I usually keep my Firefox open for many days and I want everything (extensions, scripts, styles) to check for updates daily. FM says that things will be updated daily (as I set the preference to 1 day), but this is not being honored. It updates just once per session and only if Date.now() > autoUpdateLast + days is true when the session started, since it's never checked again.

browser.alarms only works when browser is open and is cleared when it is closed. User who shut down Firefox or restart often would reset the timer.

What's the problem? autoUpdateLast is stored. On startup, set the timer to autoUpdateLast + days and periodInMinutes: days * 60 * 24.

[erosman]

I usually keep my Firefox open for many days

I understand. In my own case, using Nightly, The Firefox gets restarted often and I shut it down at night.

Let's see what is best way to do this for everyone.

[pintassilgo]

Thanks. No interest in hosting FM in GitHub (or similar)? People could submit pull requests to help you.

[erosman]

No interest in hosting FM in GitHub (or similar)? People could submit pull requests to help you.

It has been discussed before. Since I develop on my own computer and run the under-development version in Firefox to test, and make many changes per day to the code, it is not practical to keep uploading them to Github and then there would be a conflict between versions.

I help develop another extension on Github for a friend, and every time I have to make a considerable update, I have to ask them to stop makes changes until I finish or else it runs into conflict.

[erosman]

Auto-Update ➔ Mass-Update

What about making auto-update a one-click mass-update?

• remove all the idle and lastupdate etc
• add a menu/button for mass-update
• user clicks mass-update to update whenever user wants
• all scripts/CSS will be updated (if there is version & updateURL)

:thinking:

[pintassilgo]

What about making auto-update a one-click mass-update?

One-click mass-update would be a good feature to add, mainly for those who disable auto-update. Firefox has this for extensions, Stylus has...

But, as a replacement for auto-update, I strongly disagree. Auto-update is a must.

I don't think it's hard to implement the auto-update the right way. That's one of the reasons I talked about hosting FM in GitHub for someone to help. I still could upload the .xpi and post it here after writing the code if you're interested.

[pintassilgo]

Since I develop on my own computer and run the under-development version in Firefox to test, and make many changes per day to the code, it is not practical to keep uploading them to Github and then there would be a conflict between versions.

I fully understand. Committing on each change is really not practical. But you could at least commit when you release a version, a single commit with all the changes, just to keep as easy reference for other that may want to look at the code or even fork it.

Maybe even something like a "weekly update", in which you would simply extract the content of your current XPI in GitHub and commit, without needing to wait for a release to not diverge too much.

[erosman]

Looking at other mangers ...

• GM doesn't appear to have auto-update

• VM seems to use setTimeout() https://github.com/violentmonkey/violentmonkey/blob/ff8b0ae8a6dd4e5eeb78246068361e8f35682b38/src/background/index.js#L132-L142

• Stylus seems to use alarms https://github.com/openstyles/stylus/blob/53c71614fcd383e357444cfe73cdd3de527a8f83/background/update-manager.js#L45-L49

[erosman]

I have actually thought of a new auto-update routine.

Before rewriting it, the question is:

Should disabled scripts auto-update or not?

There are some scripts on Github that are 2.5+mb.

[erosman]

Here is the new draft auto-update process ...

• auto-update is checked on every idle
• there is no termination
• 10 auto-updates are performed on each idle until they are finished
• once all auto-updates are completed, a new autoUpdateLast is set
• ATM, it is set for enabled scripts.. but I am still thinking about it (ref #293)

Update removed enabled check

[pintassilgo]

GM doesn't appear to have auto-update

It has and uses setTimeout(): https://github.com/greasemonkey/greasemonkey/blob/master/src/bg/updater.js

alarms uses nsiTimer.

For our purpose, I would go with alarms because it's more flexible then easier to use. You can use when so that you don't need to do the math with Date.now(), you just pass autoUpdateLast + days * 60 * 60 * 24 * 1000. And you can set periodInMinutes to repeat the timer.

[pintassilgo]

Should disabled scripts auto-update or not?

I never saw a auto-update routine that skips disabled items. If someday the user enables the script, it's expected to have the updated version, the installed version may be broken and it will not be clear that it's because it lacks checking for updates.

So I think disabled scripts should auto-update. We already have per-script option to disable auto-update if that's the choice of the user.

In a ideal world, I think you should implement a global option for that, [ ] Check for updates in disabled items, but maybe you think of it as bloating.

Also, about auto-update, a feature that I always desire but that few have is an option to check for updates but just notify, not install. I do this in my Firefox for extensions, so that I can read release notes before updating.

[erosman]

It has and uses setTimeout():

I see... I was looking at auto-update controls in the GM UI and didnt see it. :man_shrugging:
It seems it uses fixed values

https://github.com/greasemonkey/greasemonkey/blob/e0caac00f34f39fe87e23e43df7c33d847f46e11/src/bg/updater.js#L5-L6

const MAX_UPDATE_IN_MS = (1000 * 60 * 60 * 24 * 7);
const MIN_UPDATE_IN_MS = (1000 * 60 * 60 * 3);

If someday the user enables the script, it's expected to have the updated version, the installed version may be broken and it will not be clear that it's because it lacks checking for updates. So I think disabled scripts should auto-update. We already have per-script option to disable auto-update if that's the choice of the user.

OK.. we will go with that. It also makes it easier to work on #293

Also, about auto-update, a feature that I always desire but that few have is an option to check for updates but just notify, not install. I do this in my Firefox for extensions, so that I can read release notes before updating.

An extension is one item. A user may have 100s scripts and generating data for notification and confirmation by user for each would be impractical. In most cases there is no readable notice to present to user.

[pintassilgo]

Here is the new draft auto-update process ...

• I guess your approach is not optimized in CPU usage. There's no need to keep idle.onStateChanged listening forever, a timer (I recommend using alarms) would be the best choice. But this is a minor detail with no practical implication.

10 auto-updates are performed on each idle until they are finished

I think updating in chunks is a good thing, but since you're waiting for idle I think it loses the purpose. Update in chunks in real time (setting an interval in seconds) OR wait for idle and update everything. Not both.

ATM, it is set for enabled scripts.. but I am still thinking about it (ref #293)

Styles must autoupdate too. If not including disabled styles, at least in the way I said in the other issue (disabled styles required in enabled styles should autoupdate too).

[pintassilgo]

An extension is one item. A user may have 100s scripts and generating data for notification and confirmation by user for each would be impractical. In most cases there is no readable notice to present to user.

Here's how my Firefox works:

• Firefox checks daily for updates and there's no way to change that, but I'm OK with the interval.
• I use extensions.update.autoUpdateDefault = false, so it doesn't install available updates and I don't have to manually disable autoupdating in each addon.
• Everyday, after ending the check for updates, if there are available updates a new tab opens in about:addons in Available updates view, listing extensions with available updates and with individual buttons to update or see release notes. This new tab open as active, of course.
• It also supports a blocklist if I'm deliberately using an outdated version of an addon and don't want to be notified daily about available update.

[erosman]

I guess your approach is not optimized in CPU usage. There's no need to keep idle.onStateChanged listening forever, a timer (I recommend using alarms) would be the best choice. But this is a minor detail with no practical implication.

• Both are constantly running processes whether it is browser.idle.onStateChanged.addListener() or browser.alarms.create().
• Additionally, alarms require extra permission
• The updates are done on idle so regardless of anything, the idle process is necessary otherwise the update might run when the browser is very tied up and impact performance

I think updating in chunks is a good thing, but since you're waiting for idle I think it loses the purpose. Update in chunks in real time (setting an interval in seconds) OR wait for idle and update everything. Not both.

The way it is, FM auto-updates 10 scripts per idle. Some scripts can be very large, AND for people on slow internet connection, it would make a difference not to update 100+ script in a row. It is the time to make a cup of tea... user goes to make a cup of tea and 10 updates are done on idle and when user comes back, browser is not tied in long update process.

[pintassilgo]

Additionally, alarms require extra permission

If FM was already a popular addon I would understand the fear of requesting new permission on next update, but I don't think this would be relevant yet. As for now, I guess FM is still restricted to heavy-users that understand and don't care about requesting alarms permission. Anyway, there's setTimeout.

Both are constantly running processes whether it is browser.idle.onStateChanged.addListener() or browser.alarms.create().

With alarms or setTimeout, any code will only run when the alarm resolves. In your way, there are a few statements that will run when it's not needed. But this is negligible. Also I don't know how setTimeout and nsiTimer are implemented in low level, maybe they have some processing even while the timer hasn't resolved yet.

It is the time to make a cup of tea... user goes to make a cup of tea and 10 updates are done on idle and when user comes back, browser is not tied in long update process.

You're listening to state changes, so just stop updating when state == 'active'. But this would add a little extra complexity (lines) to the code, so I understand if you reject.

[pintassilgo]

Also, about auto-update, a feature that I always desire but that few have is an option to check for updates but just notify, not install. I do this in my Firefox for extensions, so that I can read release notes before updating.

Use case scenario: For any reason, I forked the code. But I still want to be notified about available updates so that I can update my fork accordingly. Without that feature, there's no way to do this, I would need to remember to occasionally and manually check if the original style/script was updated.

[erosman]

For any reason, I forked the code. But I still want to be notified about available updates so that I can update my fork accordingly. Without that feature, there's no way to do this, I would need to remember to occasionally and manually check if the original style/script was updated.

If only the notice is needed that a script/CSS was updated, the log shows that.

[erosman]

Auto-update process rewritten for v2.19. Feel free to reopen if there are issues.

[pintassilgo]

Thanks. There's still a little issue preventing auto-update from working, some undefined variables. Click the commit above to see.

Also, I:

• Fixed another undefined variables inside catch.
• Fixed wrong \@homepage when updateURL has custom settings parameters like https://userstyles.org/styles/107316/vk-plus.css?ik-background=ik-none&ik-page=ik-white&ik-sidebar=ik-fixed&ik-topbar=ik-fixed&ik-images=ik-square&ik-reply=ik-fixed&ik-overlay=ik-no&ik-footer=ik-hidden&ik-text=ik-visible&ik-topfix=ik-right&ik-ads=ik-no.

P.S.: I can't reopen this issue, Reopen button is not available for me.

[erosman]

Thank you ...

I missed those cssURL :man_facepalming:

Homepage also changed to the following:

@homepage       ${item.updateURL.replace(/\.css.*/, '')}

[pintassilgo]

I thought this simple regex wouldn't be safe because a UserStyle can be named "something.css" (so the URL would be "something.css.css"), then I made that little more complex regex, but it seems that USO escapes dots in URL, so it's fine.

Well, the main issue preventing auto-update from working are the undefined name and updateURL in app.js. If you prefixed them with item., I think you can close this issue.

Thanks.

[erosman]

This should be more robust even in case of **********.css.css

@homepage       ${item.updateURL.replace(/\.css(\?.*|$)/, '')}

[pintassilgo]

Oops, I just noticed a new issue regarding updating.

As you suggested, I have the style VK Plus disabled and I use my fork with @require VK Plus.

The problem is: FM downloaded VK Plus update a few hours ago, everything is fine in VK Plus code and info, but I noticed that the changes from the new version of the style weren't being applied to VK pages I loaded after the update. So I disabled my fork, enabled the original and reloaded the page. Then for the first time the updated version was applied. So I reverted to normal, disabling the original and enabling my fork, then finally everything worked as it should.

So the issue is: it seems that FM caches \@require'd style code and doesn't apply its update in the fork. FM only updates the cache of the style when I enable it. But since the style is required in a enabled item, it should instantly update the cache.

[erosman]

it seems that FM caches @require'd style code and doesn't apply its update in the fork.

Well, true but it is cached by Firefox API and not FM

FM only updates the cache of the style when I enable it. But since the style is required in a enabled item, it should instantly update the cache.

@require is applied at the time of registering a script/css and then passed to Firefox.

It is true that if the @require target is updated via auto-update or manual update, it currently has no effect on already registered script/CSS.

The problem with updating the already running script/CSS is that

• every update should go through all script/CSS to check if they have used @require on this item
• if so, unregister & register the parent script again
• then the page that has that script running will be without a script until tab is refreshed

:man_shrugging:

[pintassilgo]

then the page that has that script running will be without a script until tab is refreshed

This is #295, we must wait for Mozilla devs to help us.

As for now, I guess there's no other thing to do but to fix what I reported. While #295 is not fixed, the style being removed from the page when it updates is expected. What it's not expected is to need to enable a disabled style just to apply its update in the enabled style that requires it.

[erosman]

the style being removed from the page when it updates is expected

It is not only the styles but also scripts.

• User is on a page
• the @require updates in the background
• script & CSS are re-registered
• page soddenly goes back to its original state without userscript/userCSS
• if the script had set anything (often the case with some game pages) all will be lost, or if the script prevented anything (like ads) they come back

This will create a lot of complaints.

[pintassilgo]

Thanks for all your efforts, but unfortunately the issue wasn't solved yet in v2.20. As I tried to show in the commit I referenced in this issue and also in this quote from https://github.com/erosman/support/issues/297#issuecomment-803520956:

Well, the main issue preventing auto-update from working are the undefined name and updateURL in app.js. If you prefixed them with item., I think you can close this issue.

These are the cause of the issue, but you didn't fix them, you just fixed the cssURL → item.updateURL inside catch.

What is really preventing auto-update are the undefined name and updateURL.

https://github.com/pintassilgo/forked-firefox-extensions/commit/d64f31b10226b2c56e038e6790b4b59b629326ca#diff-707f2764e8492613b3f88e1b62a55a4bb30047b06806eaae68ef39e175008077L510-R511

[erosman]

Oops .... :man_facepalming:

I forgot to do that. I will do that now.

[erosman]

v2.21 uploaded