Description

binki

opened on Jul 16, 2024

Issue body actions

I have a script which I updated on 2024-04 but one of my browsers says it last updated it on 2023-05-22. Automatic updates is on and everything.

When investigating, I looked at the code in the debugger. I think I found two issues.

Lack of updateNextAt value

greasemonkey/src/bg/updater.js

Line 104 in c0b3b54

if (!nextTime || !nextUuid || v < nextTime) {

I also found that only one of my scripts had a updateNextAt. value set in chrome.storage.local.get().

It seems that v < nextTime will evaluate to false when v is undefined and nextTime is some positive number:

> undefined < 10
false

I think that the intention is that any script without a updateNextAt. value automatically be scheduled for an immediate update check. What the actual current logic appears to be is that any script which had ever successfully updated in the past will update. It seems impossible for more than one script to ever have an updateNextAt. value set for it unless if the user debugs the extension and manually adds keys. So I think that either v === undefined should be added to the list of short-circuiting conditions or v should be coalesced to 0 when its value is not present in chrome.storage.local.

To explain in more detail: it seems that on one of my systems, only one script gets updated due to this issue. On another system, only 7 of 14 or so eligible scripts gets updated. The reason that sometimes multiple scripts actually can get their updateNextAt. value set depends on the order that script UUIDs are added to the updateNextAtKeys variable. As long as a newly added script which does not have an updateNextAt. value yet is the first key in updateNextAtKeys during at least one auto-update check, then it will, as the author intended, be considered eligible for updating and join the list of scripts which are capable of auto-updating (due to successfully having its updateNextAt. value set during that update check).

Lack of Error Tolerance

At

greasemonkey/src/bg/updater.js

Line 131 in c0b3b54

await checkForUpdate(nextUuid);

I see a call to a function which can throw (i.e., awaiting a Promise which can be rejected). In the above example, when checking the values on one of my computers, the one updateNextAt. value I saw in the system was set to a value in the past. Since this is running on a browser which might be left open for weeks on a computer which does sometimes enter a low power state, I suspect, but cannot prove, that the update checks of even that one script ended up paused due to updates never being rescheduled after a download failure. I think a try{}finally{} construct is in store here. This would enable the extension to continue checking for updates even if the scheduled update occurs when the system is offline.

Please let me know if PRs would be helpful.

Thanks.

Activity

1. arantius commented on Jul 17, 2024

   

   arantius

   on Jul 17, 2024

   Collaborator

   More actions

   Your diagnosis above sounds good. PRs would be happily reviewed and probably merged. Thank you! To make that easier:

   • If v being undefined is a/the problem, I'd probably prefer to change || v < nextTime to || !v || v < nextTime. With a comment about handling the case where v is undefined.
   • Agreed about catching exceptions! scheduleNextScriptAutoUpdate() should always happen so finally seems called for.

   Reacted by Nathan Phillip Brink
2. binki

   added 4 commits that reference this issue on Jul 17, 2024

   Fix issue preventing some scripts from auto-updating.

   7fc9137

   Add missing error recovery in auto-update logic.

   c587792

   Fix issue preventing some scripts from auto-updating.

   ac95ffa

   Fix issue preventing some scripts from auto-updating.

   a49fb7d
3. binki commented on Jul 17, 2024

   

   binki

   on Jul 17, 2024

   ContributorAuthor

   More actions

   I provided the PRs.

   I was wondering if the whole defaultCheckTime logic is intentional. That prevents updates from ever working if you never leave your browser open for at least 3 hours after the extension loads. All of the other update logic appears to handle short-lived browser sessions well. This is where I wonder if v could just be set to 0 when it is undefined.

   Anyway, I am looking forward to your reviews!
4. binki

   added a commit that references this issue on Jul 18, 2024

   Fix issue preventing some scripts from auto-updating.

   6d37edf
5. binki commented on Jul 18, 2024

   

   binki

   on Jul 18, 2024

   ContributorAuthor

   More actions

   The more I touched the code, I start thinking about more refactoring. For example, _pickNextScriptAutoUpdate is async but still uses the Promise() constructor. And the first check for updates always fails because it happens before the script registry is loaded. Also, I expect that if the script download fails (I have a script hosted in a private repository, so updates for that will only work if I am logged into Bitbucket) the update checker will get stuck on that script and no longer check updates on other scripts (so maybe its updateNextAt. should be updated, but by a smaller amount, even after a download failure). I don’t know if I will find the time, but I might try to provide additional PRs to address those (especially the download-failure-related issue).
6. binki

   added 3 commits that reference this issue on Jul 18, 2024

   Fix issue preventing some scripts from auto-updating.

   feba928

   Fix issue preventing some scripts from auto-updating.

   3f1455d

   Fix issue preventing some scripts from auto-updating.

   f3f0013
7. arantius

   added this to the 4.13 milestone on Jul 20, 2024
8. arantius commented on Jul 22, 2024

   

   arantius

   on Jul 22, 2024

   Collaborator

   More actions

   I believe #3191 and #3192 resolve this?

   Reacted by Nathan Phillip Brink
9. arantius

   closed this as completedon Jul 22, 2024
10. arantius

    added a commit that references this issue on Jul 24, 2024

    Test that `_pickNextScriptAutoUpdate()` picks never-updated scripts.

    5ed5f22

Issue actions