1875502 - Broken session store breaks updates

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Opening "About Nightly" on macOS with 2024-01-14. Firefox checks for updates and says "Nightly is up to date"

However, turning on app.update.log gives:

AUS:AUM AppUpdater:check - currentState=STATE_IDLE
AUS:AUM AppUpdater:check - starting update check
AUS:SVC CheckerService:checkForUpdates - checkType: 2
AUS:SVC CheckerService:checkForUpdates - Making new check request for check id 10.
AUS:SVC CheckerService:getUpdateURL - checkType: 2
AUS:SVC CheckerService:getUpdateURL - update URL: https://aus5.mozilla.org/update/6/Firefox/123.0a1/20240114093125/Darwin_x86_64-gcc3/en-US/nightly/Darwin%252019.6.0/ISET%3ASSE4_2%2CMEM%3A65536/default/default/update.xml?force=1
AUS:SVC CheckerService:#updateCheck - sending request to: https://aus5.mozilla.org/update/6/Firefox/123.0a1/20240114093125/Darwin_x86_64-gcc3/en-US/nightly/Darwin%252019.6.0/ISET%3ASSE4_2%2CMEM%3A65536/default/default/update.xml?force=1
AUS:SVC CheckerService:#updateCheck - request got 'load' event
AUS:SVC CheckerService:#updateCheck - request completed downloading document
AUS:SVC CheckerService:#updateCheck - number of updates available: 1
AUS:AUM AppUpdater:check - Update check succeeded
AUS:SVC UpdateService:selectUpdate - skipping update because no partial patch is available and an update has already been downloaded.
AUS:AUM AppUpdater:check - result: NO_UPDATES_FOUND 

which suggests that Firefox is not up to date.

Comment 1

[Robin Steuber (she/her) [:bytesized]]

Can you reproduce this consistently? If so, could you run this in the Browser Console and tell me the output?

XPCOMUtils.defineLazyServiceGetter(
  lazy,
  "UM",
  "@mozilla.org/updates/update-manager;1",
  "nsIUpdateManager"
);
lazy.UM.readyUpdate.state

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I get "applied"

Comment 3

[Jeff Muizelaar [:jrmuizel]]

My computer rebooted and so Firefox restarted. It's still stuck on 2024-01-14

Comment 4

[Robin Steuber (she/her) [:bytesized]]

Could you collect another browser console log like the one you provided in Comment 0? I'd like to see the logs of the Update Manager initializing, so it should be collected soon after startup. Which is to say, you should start the browser, open the console, check for an update, and then collect the log.

Thank you!

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Here's what I get:

1706108029437	addons.xpi	WARN	Checking /Applications/FirefoxNightly.app/Contents/Resources/distribution/extensions for addons
UTM:SVC TimerManager:registerTimer - timerID: xpi-signature-verification interval: 86400 skipFirst: false
AUS:STB UpdateServiceStub - Begin.
AUS:SVC RestartOnLastWindowClosed.#maybeEnableOrDisable - Enabling
AUS:SVC RestartOnLastWindowClosed.#onLastWindowClose - Last window closed. Starting restart timer
AUS:SVC Creating UpdateService
AUS:SVC Logging current UpdateService status:
AUS:SVC UpdateService.canUsuallyCheckForUpdates - able to check for updates
AUS:SVC UpdateService.canCheckForUpdates - able to check for updates
AUS:SVC getCanApplyUpdates - testing write access /Users/jrmuizel/Library/Caches/Mozilla/updates/Applications/FirefoxNightly/update.test
AUS:SVC getCanApplyUpdates - bypass the write since elevation can be used on Mac OS X
AUS:SVC getElevationRequired - recursively testing write access on /Applications/FirefoxNightly.app
AUS:SVC getElevationRequired - able to write to application bundle, elevation not required
AUS:SVC gCanStageUpdatesSession - testing write access /Users/jrmuizel/Library/Caches/Mozilla/updates/Applications/FirefoxNightly/update.test
AUS:SVC gCanStageUpdatesSession - able to stage updates
AUS:SVC getElevationRequired - recursively testing write access on /Applications/FirefoxNightly.app
AUS:SVC getElevationRequired - able to write to application bundle, elevation not required
AUS:SVC Elevation required: false
AUS:SVC Other instance of the application currently running: false
AUS:SVC Downloading: false
AUS:SVC End of UpdateService status
AUS:SVC RestartOnLastWindowClosed.#onWindowOpen - Window opened. Cancelling restart timer.
AUS:SVC readStatusFile - status: succeeded, path: /Users/jrmuizel/Library/Caches/Mozilla/updates/Applications/FirefoxNightly/updates/0/update.status
AUS:SVC UpdateManager:UpdateManager - status = "succeeded"
AUS:SVC UpdateManager:UpdateManager - Initialized downloadingUpdate to null
AUS:SVC UpdateManager:UpdateManager - Initialized readyUpdate to [object Object]
AUS:SVC UpdateManager:UpdateManager - Initialized readyUpdate state to applied
AUS:SVC getCanApplyUpdates - testing write access /Users/jrmuizel/Library/Caches/Mozilla/updates/Applications/FirefoxNightly/update.test
AUS:SVC getCanApplyUpdates - bypass the write since elevation can be used on Mac OS X
AUS:SVC CheckerService:getUpdateURL - checkType: 1
UTM:SVC TimerManager:registerTimer - timerID: browser-cleanup-thumbnails interval: 3600 skipFirst: false
AUS:SVC CheckerService:getUpdateURL - update URL: https://aus5.mozilla.org/update/6/Firefox/123.0a1/20240114093125/Darwin_x86_64-gcc3/en-US/nightly/Darwin%252019.6.0/ISET%3ASSE4_2%2CMEM%3A65536/default/default/update.xml
AUS:SVC UpdateService.canUsuallyCheckForUpdates - able to check for updates
AUS:SVC UpdateService.canCheckForUpdates - able to check for updates
AUS:SVC CheckerService:checkForUpdates - checkType: 1
AUS:SVC UpdateService.canUsuallyCheckForUpdates - able to check for updates
AUS:SVC UpdateService.canCheckForUpdates - able to check for updates
AUS:SVC CheckerService:checkForUpdates - Making new check request for check id 1.
AUS:SVC waitForOtherInstances - beginning polling
AUS:SVC waitForOtherInstances - no other instances found, exiting
AUS:SVC CheckerService:getUpdateURL - checkType: 1
AUS:SVC CheckerService:getUpdateURL - update URL: https://aus5.mozilla.org/update/6/Firefox/123.0a1/20240114093125/Darwin_x86_64-gcc3/en-US/nightly/Darwin%252019.6.0/ISET%3ASSE4_2%2CMEM%3A65536/default/default/update.xml
AUS:SVC CheckerService:#updateCheck - sending request to: https://aus5.mozilla.org/update/6/Firefox/123.0a1/20240114093125/Darwin_x86_64-gcc3/en-US/nightly/Darwin%252019.6.0/ISET%3ASSE4_2%2CMEM%3A65536/default/default/update.xml
NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAboutModule.getURIFlags] E10SUtils.sys.mjs:435
UTM:SVC TimerManager:registerTimer - timerID: region-update-timer interval: 604800 skipFirst: false
AUS:SVC CheckerService:#updateCheck - request got 'load' event
AUS:SVC CheckerService:#updateCheck - request completed downloading document
AUS:SVC CheckerService:#updateCheck - number of updates available: 1
AUS:SVC UpdateService:selectUpdate - skipping update because no partial patch is available and an update has already been downloaded.
XPCOMUtils.defineLazyServiceGetter(
  lazy,
  "UM",
  "@mozilla.org/updates/update-manager;1",
  "nsIUpdateManager"…
"applied"
QM_TRY failure (WARNING): 'Unavailable, context dom::quota::FirstInitializationAttempt::TemporaryStorage', file dom/quota/ActorsParent.cpp:2769

Comment 6

[Robin Steuber (she/her) [:bytesized]]

Huh. I'm very confused by this log.

From these lines

AUS:SVC UpdateManager:UpdateManager - status = "succeeded"
AUS:SVC UpdateManager:UpdateManager - Initialized readyUpdate state to applied

we can see that the state, at startup, is that the update.status file shows succeeded and the active update XML shows the update as having the status applied. The updater binary modifies update.status but not the XML, so it is the normal, expected state of things that these two statuses not match after an update is installed. So this looks fine so far.

But normally in this case _postUpdateProcessing would move the completed update to the update history, here, allowing another update to be installed. But we clearly aren't getting to that line, since the log doesn't contain

AUS:SVC UpdateService:_postUpdateProcessing - Cleaning up successful ready update.

We do see AUS:STB UpdateServiceStub - Begin., which is in the path that would normally lead to this happening. But nearly all routes from there should have produced a UpdateService:_postUpdateProcessing log line. I can only find three that do not:

• The Stub sees no update status file, here. This doesn't seem right, since we successfully read succeeded from that file.
• If update was disabled, we would return here. But that can't be right either, because we would never have logged Logging current UpdateService status in that case.
• An exception being thrown somewhere.

Is the log that you provided filtered in any way? Often update logs are collected with the AUS: filter. But I see lines in your log that don't seem to match that filter.

My best guess here is that an exception is being thrown, but it happens very quickly - before the console can be opened. I have experienced before that some things that happen before the console is opened do not reliably end up in the console, so this is my best guess as to what is going on.

As for how to move forward troubleshooting this, I'm not really sure. I'm going to have to think about it a bit.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

It's worth noting that I currently hit bug 1873912 which is an exception that's being thrown during browser startup. So it could be that my browser is in a sort of broken state.

Comment 8

[Nick Alexander :nalexander [he/him]]

Jeff -- could you run with MOZ_LOG=Dump:5? That should send all console output to stdout, and might witness an early exception or other anomaly. (That was added in https://bugzilla.mozilla.org/show_bug.cgi?id=1687553, if you want to dig deeper into how it works.)

Comment 9

[Jeff Muizelaar [:jrmuizel]]

I don't get any output in my terminal when running:

MOZ_LOG=Dump:5 /Applications/FirefoxNightly.app/Contents/MacOS/firefox

Comment 10

[Robin Steuber (she/her) [:bytesized]]

Can you try manually invoking the stub and see if it spits out any exceptions or any messages we haven't seen yet? I'd like to see a complete log, like the one from comment 5. But at the end, run Cc["@mozilla.org/updates/update-service-stub;1"].createInstance(Ci.nsISupports);

Comment 11

[Jeff Muizelaar [:jrmuizel]]

1706559069219	addons.xpi	WARN	Checking /Applications/FirefoxNightly.app/Contents/Resources/distribution/extensions for addons
UTM:SVC TimerManager:registerTimer - timerID: xpi-signature-verification interval: 86400 skipFirst: false
AUS:STB UpdateServiceStub - Begin.
AUS:SVC RestartOnLastWindowClosed.#maybeEnableOrDisable - Enabling
AUS:SVC RestartOnLastWindowClosed.#onLastWindowClose - Last window closed. Starting restart timer
AUS:SVC Creating UpdateService
AUS:SVC Logging current UpdateService status:
AUS:SVC UpdateService.canUsuallyCheckForUpdates - able to check for updates
AUS:SVC UpdateService.canCheckForUpdates - able to check for updates
AUS:SVC getCanApplyUpdates - testing write access /Users/jrmuizel/Library/Caches/Mozilla/updates/Applications/FirefoxNightly/update.test
AUS:SVC getCanApplyUpdates - bypass the write since elevation can be used on Mac OS X
AUS:SVC getElevationRequired - recursively testing write access on /Applications/FirefoxNightly.app
AUS:SVC getElevationRequired - able to write to application bundle, elevation not required
AUS:SVC gCanStageUpdatesSession - testing write access /Users/jrmuizel/Library/Caches/Mozilla/updates/Applications/FirefoxNightly/update.test
AUS:SVC gCanStageUpdatesSession - able to stage updates
AUS:SVC getElevationRequired - recursively testing write access on /Applications/FirefoxNightly.app
AUS:SVC getElevationRequired - able to write to application bundle, elevation not required
AUS:SVC Elevation required: false
AUS:SVC Other instance of the application currently running: false
AUS:SVC Downloading: false
AUS:SVC End of UpdateService status
AUS:SVC RestartOnLastWindowClosed.#onWindowOpen - Window opened. Cancelling restart timer.
AUS:SVC readStatusFile - status: succeeded, path: /Users/jrmuizel/Library/Caches/Mozilla/updates/Applications/FirefoxNightly/updates/0/update.status
AUS:SVC UpdateManager:UpdateManager - status = "succeeded"
AUS:SVC UpdateManager:UpdateManager - Initialized downloadingUpdate to null
AUS:SVC UpdateManager:UpdateManager - Initialized readyUpdate to [object Object]
AUS:SVC UpdateManager:UpdateManager - Initialized readyUpdate state to applied
AUS:SVC getCanApplyUpdates - testing write access /Users/jrmuizel/Library/Caches/Mozilla/updates/Applications/FirefoxNightly/update.test
AUS:SVC getCanApplyUpdates - bypass the write since elevation can be used on Mac OS X
AUS:SVC CheckerService:getUpdateURL - checkType: 1
UTM:SVC TimerManager:registerTimer - timerID: browser-cleanup-thumbnails interval: 3600 skipFirst: false
AUS:SVC CheckerService:getUpdateURL - update URL: https://aus5.mozilla.org/update/6/Firefox/123.0a1/20240114093125/Darwin_x86_64-gcc3/en-US/nightly/Darwin%252019.6.0/ISET%3ASSE4_2%2CMEM%3A65536/default/default/update.xml
AUS:SVC UpdateService.canUsuallyCheckForUpdates - able to check for updates
AUS:SVC UpdateService.canCheckForUpdates - able to check for updates
AUS:SVC CheckerService:checkForUpdates - checkType: 1
AUS:SVC UpdateService.canUsuallyCheckForUpdates - able to check for updates
AUS:SVC UpdateService.canCheckForUpdates - able to check for updates
AUS:SVC CheckerService:checkForUpdates - Making new check request for check id 1.
AUS:SVC waitForOtherInstances - beginning polling
AUS:SVC waitForOtherInstances - no other instances found, exiting
AUS:SVC CheckerService:getUpdateURL - checkType: 1
AUS:SVC CheckerService:getUpdateURL - update URL: https://aus5.mozilla.org/update/6/Firefox/123.0a1/20240114093125/Darwin_x86_64-gcc3/en-US/nightly/Darwin%252019.6.0/ISET%3ASSE4_2%2CMEM%3A65536/default/default/update.xml
AUS:SVC CheckerService:#updateCheck - sending request to: https://aus5.mozilla.org/update/6/Firefox/123.0a1/20240114093125/Darwin_x86_64-gcc3/en-US/nightly/Darwin%252019.6.0/ISET%3ASSE4_2%2CMEM%3A65536/default/default/update.xml
NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAboutModule.getURIFlags] E10SUtils.sys.mjs:435
UTM:SVC TimerManager:registerTimer - timerID: region-update-timer interval: 604800 skipFirst: false
AUS:SVC CheckerService:#updateCheck - request got 'load' event
AUS:SVC CheckerService:#updateCheck - request completed downloading document
AUS:SVC CheckerService:#updateCheck - number of updates available: 1
AUS:SVC UpdateService:selectUpdate - skipping update because no partial patch is available and an update has already been downloaded.

Cc["@mozilla.org/updates/update-service-stub;1"].createInstance(Ci.nsISupports);
AUS:STB UpdateServiceStub - Begin.
XPCWrappedNative_NoHelper { QueryInterface: QueryInterface() }

Comment 12

[Robin Steuber (she/her) [:bytesized]]

Huh. I'm pretty lost here. I don't see any exceptions being thrown. Yet, as I covered in Comment 6, I don't understand how else this could be happening.

I am going to assume for now that the other issue mentioned in Comment 7 is affecting us here and set this to a low severity. I'm not very happy allowing other components to cause problems for update this way, since they rely on the update system to deliver the fixes to other systems. So I'll keep trying to think of ways to move forward with this bug. But at the moment I can't really think of anything.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

I manually removed the offending about:firefoxview-next entry from my sessionstore.jsonlz4 and that made updates start working again. It does seem pretty bad that a bad sessionstore file can break updates. Should I file a new bug about that or do you want to repurpose this one for that?

Comment 14

[Robin Steuber (she/her) [:bytesized]]

Using this bug seems reasonable.

Comment 15

[:Gijs (he/him)]

Do we understand why/how the session restore exception breaks updates? I wouldn't have thought the update service was in any way dependent on code run as part of initial window opening to be successful.

Comment 16

[Robin Steuber (she/her) [:bytesized]]

(In reply to :Gijs (he/him) from comment #15)

Do we understand why/how the session restore exception breaks updates?

Nope. Comment 6 explains the ways that I believe this could be failing, which basically boils down to "I think an exception is being thrown somewhere". But either my analysis is incorrect or the exception just refuses to show up in the console, for some reason.

Comment 17

[Masatoshi Kimura [:emk]]

When UpdateService observes "post-update-processing" topic, it will add an observer for "sessionstore-windows-restored" topic instead of calling _postUpdateProcessing directly (See here). So if the session store module fails to notify of "sessionstore-windows-restored" topic for some reason, _postUpdateProcessing will not be called.

Does it explain what is going on?

Comment 18

[Robin Steuber (she/her) [:bytesized]]

Ohhhhhh, that does seem likely to explain it. I completely forgot about that detail and I guess I missed it when I was tracing through this code path.

I would definitely like to fix this, but it's not immediately clear to me what the right fix is. I'm going to discuss this with my team and try to find a good answer.

Leaving the needinfo until I have one.

Comment 19

[Robin Steuber (she/her) [:bytesized]]

Alright, I think the solution here is simply to remove the wait for sessionstore-windows-restored. It appears that the reason for doing that no longer exists.

However, I would kind of like to go a bit further than this. It has always been my goal for the update system to be as bulletproof as possible, since failures in it are inherently hard to fix and also make it hard to fix any other problem that version might have. I'm not sure how difficult these changes will be, but I'm going to look into them and implement them if they don't look too onerous.

1. As seen in this bug, _postUpdateProcessing failing in certain ways can prevent us from installing more updates. I want to try to harden that function a bit to try to make sure that any other unexpected failures that might occur don't cause problems like this. Anything unrecoverable (like inability to clean up certain parts of the update directory) should probably result in an "update failed - reinstall" prompt.
2. I think that it might be wise to have a more standardized way of entering the update system that involves making sure that _postUpdateProcessing was called, if necessary.

I've kind of got a lot on my plate right now, but I think this is important and I want to spend some time on it. So I'm going to assign myself and try to work on it asap.

Comment 20

[Robin Steuber (she/her) [:bytesized]]

I'm actually going to spin the additional work that I mentioned in comment 19 off into its own bug (Bug 1884260) so that I can land the fix for this bug as quickly as possible. I should have a patch up for this very shortly.

Comment 21

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Don't wait for sessionstore-windows-restored to do _postUpdateProcessing r=nalexander!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ca2467996c3
Don't wait for sessionstore-windows-restored to do _postUpdateProcessing r=nalexander,application-update-reviewers

Comment 23

[pstanciu]

Backed out for causing marionette failures on test_no_window_update_restart.py

• Backout link
• Push with failures
• Failure Logs:
  TEST-UNEXPECTED-ERROR | toolkit/mozapps/update/tests/marionette/test_no_window_update_restart.py TestNoWindowUpdateRestart.test_update_on_last_window_close
  TEST-UNEXPECTED-FAIL | browser/components/tests/browser/whats_new_page/browser_whats_new_page.js | Uncaught exception in test bound whats_new_page
  TEST-UNEXPECTED-FAIL | browser/components/urlbar/tests/browser-tips/suppress-tips/browser_suppressTips.js

Comment 24

[Robin Steuber (she/her) [:bytesized]]

I'll investigate.

Comment 25

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Fix tests broken by the previous patch stack that standardizes update initialization r=nalexander!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a5f4e6e4368
Don't wait for sessionstore-windows-restored to do _postUpdateProcessing r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/80a009c6aacd
Fix test_no_window_update_restart to work whether post update processing ran or not r=nalexander,application-update-reviewers

Comment 27

[Sandor Molnar[:smolnar]]

Backed out for causing bc failures @ browser_suppressTips.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/eeeccc3d40a5983917f96cdbe50f65e6071cf30f

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | browser/components/urlbar/tests/browser-tips/suppress-tips/browser_suppressTips.js | updateAppliedTime pref should be updated when booting this test

Failure log ->TEST-UNEXPECTED-FAIL | browser/components/tests/browser/whats_new_page/browser_whats_new_page.js | Uncaught exception in test bound whats_new_page - Waiting for the expected page to reopen

Comment 28

[Robin Steuber (she/her) [:bytesized]]

This is proving harder to merge than I hoped. I really need to prioritize Bug 1883089 over this, and I am now unblocked on it. I'll try to come back to this as soon as I can.

Comment 29

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:bytesized, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 30

[Robin Steuber (she/her) [:bytesized]]

Still need to find time to fix these test failures.

Comment 31

[Robin Steuber (she/her) [:bytesized]]

I think I've discovered a problem that may force me to fold the work I spun off into Bug 1884260 back into this work.

It has to do with the way that we show the What's New Page (WNP). The issue is that unless UpdateManager.readyUpdate has the update that we just installed in it, we do not show the WNP (here). This causes the related Bug 1889785, which I filed as a result of looking into this. But it also just generally relies on race conditions to function properly. If _postUpdateProcessing runs before this does, that update simply won't be present in readyUpdate and the WNP will never be shown. Until now, _postUpdateProcessing was delayed long enough that it was probably impossible for this to happen. But I'm really not happy relying on timing here to make sure that the update that we want is still in readyUpdate.

I was originally going to just have the UpdateManager do this during initialization, but this has two problems. First, since it's not especially well-defined that this initialization will happen before _postUpdateProcessing, this doesn't really resolve the race condition. In practice, _postUpdateProcessing uses UpdateManager pretty early on. But I don't really love relying on that. The second problem is that we really shouldn't be assuming that an update was installed unless we can check some things like the state (i.e. we didn't install it if the update failed). But this is the one time in the update process when readyUpdate.state doesn't match update.status, since the updater binary doesn't touch active-update.xml. And I don't really want to add an update.status read to the UpdateManager initialization.

With a centralized update entry point, we can ensure that we have a standard time and place to check if the previous value of readyUpdate is an update that was just installed. Then we can store it in a designated place and transfer it out of that value (as part of _postUpdateProcessing). Note that this will pretty explicitly maintain Bug 1889785 behavior rather than fixing it, but that's an issue for another day.

Comment 32

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Convert UpdateService.sys.mjs to use classes r=nalexander!

There isn't an especially good reason why this needs to be done, but the syntax is a bit nicer, it allows us to use some modern language features, and it's a pretty low efffort and low risk change.

Also changes generateQI calls to use XPCOM id objects instead of strings.

Comment 33

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Add separate XPCOM interface for internal use of the update manager r=nalexander!

Looking forward, a goal of this patch stack is to have external interfaces automatically initialize the update system before doing anything that requires that. However, during that initialization, we need to make sure that we don't accidentally cause recursion by invoking an external interface that also does initialization. To do this, we will require special, privileged access to this functionality that doesn't do the initialization.

An appealing way of implementing this is to have UpdateManager implement both nsIUpdateManager and nsIUpdateManagerInternal. However, by experimentation, this seems to allow us to use the internal functions directly on the update manager object, even without ever QIing it to nsIUpdateManagerInternal. This is undesirable both because it allows consumers to use internal interfaces sort of implicitly and because it means that if we move functionality from the public interface to the private one, existing consumers of that interface could more easily be missed since the method is still available on the update manager object that it's already using.

This commit doesn't actually add any attributes or methods to the new interface. That will be done later in the patch stack.

Comment 34

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Move update manager reload functionality into nsIUpdateManagerInternal r=nalexander!

Comment 35

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Make AUS._checkForBackgroundUpdates async r=nalexander!

I want the shouldEnterUpdate function that I'm going to add later in the patch stack to be asynchronous, so the update entry points should be asynchronous functions.

Comment 36

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Make update cleanup functions async r=nalexander!

Comment 37

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Make UpdateManager's history functions async r=nalexander!

Comment 38

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Make AUS's selectUpdate async r=nalexander!

Comment 39

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Make AUS's readyUpdate async r=nalexander!

Aside from tests (which will be in the next patch in the stack), this specifically leaves two instances of readyUpdate unchanged - the one in BrowserContentHandler's getArgs and the one in UpdatePing's _getActiveUpdate. In both of these cases what is actually wanted is the update that was just installed. It's only because of the (fairly arbitrary) order that things are started up in that readyUpdate still happens to contain this value when these things run. Later in this patch stack we will add something more appropriate for this. But in the meantime, it doesn't make sense to convert functions to be asynchronous that will not ultimately need to be asynchronous.

Comment 40

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Update tests to accomodate async readyUpdate r=nalexander!

Comment 41

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Make AUS's downloadingUpdate async r=nalexander!

This does not include testing changes. Those will be in the next patch in this stack.

Comment 42

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Update tests to accomodate async downloadingUpdate r=nalexander!

Comment 43

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Make testPostUpdateProcessing async r=nalexander!

Comment 44

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Make initUpdateServiceStub async r=nalexander!

Comment 45

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Split AUS.downloadUpdate into an internal version and an external version and make result more specific r=nalexander!

Comment 46

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Split AUS.stopDownload into an internal version and an external version r=nalexander!

Comment 47

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Split Update Manager's refreshUpdateStatus into an internal version and an external version r=nalexander!

Comment 48

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Split the update checker service's checkForUpdates into an internal version and an external version r=nalexander!

The internal/external state needs to be piped through to #updateCheck, where the asynchrous initialization will need to be done.

Comment 49

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Create standardized update init flow r=nalexander!

A number of changes are made as part of this patch:

• A consistent way of initializing update is created. This is automatically run when methods that need it are invoked.
• The "post-update-processing" notification has been removed. Post update processing is now done through the new nsIApplicationUpdateService.init or nsIApplicationUpdateService.internal.postUpdateProcessing.
• Post update processing no longer waits for the sessionstore-windows-restored observer service notification
• Post update processing is no longer invoked only when the update.status file exists. It is now run unconditionally.
• Explicitly initialize before we potentially clean up updates from the update UI.

Note that the update service stub and a few consumers of it ought to be able to wait for post update processing to be done but they currently do not. That will be done later in this patch stack when we rework the stub.

Comment 50

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Add UpdateManager.updateInstalledAtStartup r=nalexander!

Comment 51

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Update UpdateServiceStub consumers r=nalexander!

The earlier patches in this stack (a) made it possible to await on the stub to complete and (b) ensured that the update system initiates properly regardless of when and if the update service stub is invoked. This allows us to remove explict stub invocations in some cases and to have the await on the results in others.

Comment 52

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Disable update by default in all tests r=nalexander!

I really thought this was already the case. It was a bit of a shock to debug a non-update-related test and discover that the failure was because update was just attempting to run normally while the test did something unrelated. In general, we shouldn't have the updater running during tests that aren't testing updater functionality. It cause test failures by contacting the real update url or by throwing errors when attempting to use features that aren't available during that test.

Comment 53

[Robin Steuber (she/her) [:bytesized]]

Attachment: WIP: Bug 1875502 - Fix tests broken by disabling update by default r=nalexander!

Comment 54

[Phabricator Automation]

Comment on attachment 9399548 [details]
Bug 1875502 - Pre: Convert UpdateService.sys.mjs to use classes r=nalexander!

Revision D209105 was moved to bug 1894979. Setting attachment 9399548 [details] to obsolete.

Comment 55

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Pre: Switch incorrect `standardInit` uses to `testPostUpdateProcessing` r=nalexander!

We currently seem to use standardInit and testPostUpdateProcessing interchangeably. In the future, we should not do this. standardInit will do the standard initialization process, which only ever runs once. If we want to run postUpdateProcessing again, even after initialization has already run, we should call into testPostUpdateProcessing. I believe that this makes sense considering the naming, but existing uses don't always match this expectation.

There are many places where we do things like

await setupUpdaterTest(...);
runUpdate(...);
await standardInit();

This doesn't make sense. We will already have initialized update by that point. This patch replaces uses of standardInit that are used this way with testPostUpdateProcessing.

Comment 56

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1875502 - Fix tests broken by disabling update by default r=nalexander!

Comment 57

[Nick Alexander :nalexander [he/him]]

bytesized: this stack makes sense to me. Start peeling off the bits and landing them so that we can bisect any issues, and hopefully we can get the meat landed early in the cycle so that Nightly shakes out unexpected interactions. Great work!

Comment 58

[Robin Steuber (she/her) [:bytesized]]

(In reply to Nick Alexander :nalexander [he/him] from comment #57)

Start peeling off the bits and landing them so that we can bisect any issues

That sounds quite difficult. For example, we'd have to split up D204129 to figure out which exact patch in the stack causes each of those test failures. Some patches really can't be split apart, like D209115 whose commit message specifically mentions leaving some instances of readyUpdate unfixed until D209127, 12 patches later in the stack.

It seems like splitting this is likely to introduce bugs. I suppose it would make it easier to track down where the bug is coming from with less code to analyze. But I think I would rather have to find fewer bugs amongst more code than to potentially introduce more bugs by trying to split off pieces of this that weren't meant to stand alone.

Comment 59

[Nick Alexander :nalexander [he/him]]

(In reply to Robin Steuber (they/them) [:bytesized] from comment #58)

(In reply to Nick Alexander :nalexander [he/him] from comment #57)

Start peeling off the bits and landing them so that we can bisect any issues

That sounds quite difficult. For example, we'd have to split up D204129 to figure out which exact patch in the stack causes each of those test failures. Some patches really can't be split apart, like D209115 whose commit message specifically mentions leaving some instances of readyUpdate unfixed until D209127, 12 patches later in the stack.

It seems like splitting this is likely to introduce bugs. I suppose it would make it easier to track down where the bug is coming from with less code to analyze. But I think I would rather have to find fewer bugs amongst more code than to potentially introduce more bugs by trying to split off pieces of this that weren't meant to stand alone.

I trust you to land as you see fit. When I see a lot of Pre: commits, I assume they are roughly independent -- and many of them, the ones that make interfaces async, surely can be landed before the meat.

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1950b330d253
Pre: Add separate XPCOM interface for internal use of the update manager r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/fcb327ff8717
Pre: Move update manager reload functionality into nsIUpdateManagerInternal r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/6e60ddb35c98
Pre: Make AUS._checkForBackgroundUpdates async r=nalexander,application-update-reviewers,firefox-desktop-core-reviewers
https://hg.mozilla.org/integration/autoland/rev/5736dca8c05b
Pre: Make update cleanup functions async r=nalexander,settings-reviewers,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/aa0eee306544
Pre: Make UpdateManager's history functions async r=nalexander,settings-reviewers,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/05b53ed47055
Pre: Make AUS's selectUpdate async r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/a013811a156b
Pre: Make AUS's readyUpdate async r=nalexander,settings-reviewers,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/b57983b426ba
Pre: Update tests to accomodate async readyUpdate r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/a33c98ac118c
Pre: Make AUS's downloadingUpdate async r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/d47c42d117e3
Pre: Update tests to accomodate async downloadingUpdate r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/b5f4c67a548b
Pre: Make testPostUpdateProcessing async r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/42ad3c8fe41e
Pre: Make initUpdateServiceStub async r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/afa0353ca6a2
Pre: Switch incorrect `standardInit` uses to `testPostUpdateProcessing` r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/917a4aed3b4b
Pre: Split AUS.downloadUpdate into an internal version and an external version and make result more specific r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/6bc2601a69bd
Pre: Split AUS.stopDownload into an internal version and an external version r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/54d53947927a
Pre: Split Update Manager's refreshUpdateStatus into an internal version and an external version r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/47bc52e77563
Pre: Split the update checker service's checkForUpdates into an internal version and an external version r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/a6c1ee51eadb
Create standardized update init flow r=nalexander,settings-reviewers,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/c538de825468
Add UpdateManager.updateInstalledAtStartup r=nalexander,application-update-reviewers,firefox-desktop-core-reviewers
https://hg.mozilla.org/integration/autoland/rev/924b7230a45e
Update UpdateServiceStub consumers r=nalexander,application-update-reviewers,firefox-desktop-core-reviewers
https://hg.mozilla.org/integration/autoland/rev/9157c611617d
Fix tests broken by the previous patch stack that standardizes update initialization r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/fb53b5f266e2
Disable update by default in all tests r=perftest-reviewers,nalexander,afinder,whimboo,sparky

Comment 61

[Sandor Molnar[:smolnar]]

Backed out for causing xpc failures @ toolkit/components/extensions/test/xpcshell/test_ext_storage_idb_data_migration.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/c80c038ba9d658e332608a6ee3ddda1db590493c

Push with failures

Failure log ->

Comment 62

[Robin Steuber (she/her) [:bytesized]]

Investigating.

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59324b119f12
Pre: Add separate XPCOM interface for internal use of the update manager r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/37d8c532f7ac
Pre: Move update manager reload functionality into nsIUpdateManagerInternal r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/ced6fb446f22
Pre: Make AUS._checkForBackgroundUpdates async r=nalexander,application-update-reviewers,firefox-desktop-core-reviewers
https://hg.mozilla.org/integration/autoland/rev/dbd54931ca08
Pre: Make update cleanup functions async r=nalexander,settings-reviewers,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/b97c545f53e6
Pre: Make UpdateManager's history functions async r=nalexander,settings-reviewers,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/a35071bd4af0
Pre: Make AUS's selectUpdate async r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/81e68b60bf38
Pre: Make AUS's readyUpdate async r=nalexander,settings-reviewers,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/f6651b3e372d
Pre: Update tests to accomodate async readyUpdate r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/722d4d09b814
Pre: Make AUS's downloadingUpdate async r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/42776c22877f
Pre: Update tests to accomodate async downloadingUpdate r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/670c095252b5
Pre: Make testPostUpdateProcessing async r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/312f910e469c
Pre: Make initUpdateServiceStub async r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/eb73f4686dfb
Pre: Switch incorrect `standardInit` uses to `testPostUpdateProcessing` r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/eb1d4d40329b
Pre: Split AUS.downloadUpdate into an internal version and an external version and make result more specific r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/1ad024c032eb
Pre: Split AUS.stopDownload into an internal version and an external version r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/776f57868702
Pre: Split Update Manager's refreshUpdateStatus into an internal version and an external version r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/0dee3dad78c8
Pre: Split the update checker service's checkForUpdates into an internal version and an external version r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/3b67ee286656
Create standardized update init flow r=nalexander,settings-reviewers,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/6b66ef4d5021
Add UpdateManager.updateInstalledAtStartup r=nalexander,application-update-reviewers,firefox-desktop-core-reviewers
https://hg.mozilla.org/integration/autoland/rev/74f095ac83d9
Update UpdateServiceStub consumers r=nalexander,application-update-reviewers,firefox-desktop-core-reviewers
https://hg.mozilla.org/integration/autoland/rev/c217ecc464d3
Fix tests broken by the previous patch stack that standardizes update initialization r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/ec4734833a47
Disable update by default in all tests r=perftest-reviewers,nalexander,afinder,whimboo,sparky

Comment 64

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/59324b119f12
https://hg.mozilla.org/mozilla-central/rev/37d8c532f7ac
https://hg.mozilla.org/mozilla-central/rev/ced6fb446f22
https://hg.mozilla.org/mozilla-central/rev/dbd54931ca08
https://hg.mozilla.org/mozilla-central/rev/b97c545f53e6
https://hg.mozilla.org/mozilla-central/rev/a35071bd4af0
https://hg.mozilla.org/mozilla-central/rev/81e68b60bf38
https://hg.mozilla.org/mozilla-central/rev/f6651b3e372d
https://hg.mozilla.org/mozilla-central/rev/722d4d09b814
https://hg.mozilla.org/mozilla-central/rev/42776c22877f
https://hg.mozilla.org/mozilla-central/rev/670c095252b5
https://hg.mozilla.org/mozilla-central/rev/312f910e469c
https://hg.mozilla.org/mozilla-central/rev/eb73f4686dfb
https://hg.mozilla.org/mozilla-central/rev/eb1d4d40329b
https://hg.mozilla.org/mozilla-central/rev/1ad024c032eb
https://hg.mozilla.org/mozilla-central/rev/776f57868702
https://hg.mozilla.org/mozilla-central/rev/0dee3dad78c8
https://hg.mozilla.org/mozilla-central/rev/3b67ee286656
https://hg.mozilla.org/mozilla-central/rev/6b66ef4d5021
https://hg.mozilla.org/mozilla-central/rev/74f095ac83d9
https://hg.mozilla.org/mozilla-central/rev/c217ecc464d3
https://hg.mozilla.org/mozilla-central/rev/ec4734833a47

Comment 67

[Ina Popescu, Desktop QA]

I've replicated this issue using Nightly 123.0a1(2024-01-14) on macOS 13.
Verified as fixed while updating to the latest Nightly 129.0a1 (2024-07-01) and Firefox 128.0b9 versions under the same configuration, as the issue no longer occurs.