1695797 - In background task mode, only process updates if we're the only instance running

Status: RESOLVED FIXED

(Toolkit :: Background Tasks, enhancement)

Description

[Nick Alexander :nalexander [he/him]]

Right now, firefox --backgroundtask ... will process any pending update at startup, just like Firefox does. We don't want to do this unless we're the only instance running: if we rename into place a staged update, then we'll update underneath another instance, causing Bug 1492023. This ticket tracks using the multi-instance lock added by Bug 1553982 to avoid pumping the update driver unless we're the lone instance.

Why is this not done without regard to the mode? Per the implementor (:mhowell), the multi-instance lock is designed to allow the user to break/ignore the lock via UI. At application startup, that's not possible. But for background task mode, we can make the decision to always respect the lock.

This is a choice for this specific use-case; it's similar to the choice we intend to pursue when launching the devtools as well: Bug 1120863.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1695797 - In background task mode, only process updates if we're the sole instance running. r?mhowell

This commit does three main things:

1. It allows to configure the global singleton nsUpdateSyncManager
   with an nsIFile rather than having it use the ambient XPCOM
   directory service. This allows to initialize the
   nsUpdateSyncManager very early: before processing updates and long
   before XPCOM is initialized. This in turn allows us to determine if
   other instances early enough to skip processing updates when
   appropriate.

When this initialization path is followed, i.e., in Firefox but not
xpcshell, the xpcom-startup notification will be received but no
action taken, since the singleton will already exist.

There is a classic time-of-check, time-of-use race window in this
implementation: an instance may be launched immediately after we check
for other instances. In practice this will result in behaviour that
is alreay possible: two independent instances both processing updates.
It is expected that the updater itself will exclude one of the
instances using its existing mutex.

2. It updates an existing background task test to use an explicit
   nsIFile rather than the existing directory service method. This
   exercises the newer API. There are other tests that might benefit,
   but there's no harm in remaining with the previous approach, since
   both are required.

3. It adds a new background task test to verify that update processing
   is skipped if we're not the sole instance running.

Depends on D106993

Comment 2

[Nick Alexander :nalexander [he/him]]

Try build is looking happy over at https://treeherder.mozilla.org/jobs?repo=try&test_paths=browser-toolbox&revision=08366517b94fbf6a76e0d0c3e0534bb7104720ee.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a6f82dc06a9
In background task mode, only process updates if we're the sole instance running. r=mhowell,application-update-reviewers,dthayer,bytesized

Comment 4

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 8a6f82dc06a9 (bug 1695797) for xpcshell failures in test_backgroundtask_update_sync_manager.js.
https://hg.mozilla.org/integration/autoland/rev/1646ae19a2625033ef7d7606cf806d9ed053568a

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=8a6f82dc06a98796026e47976a76bdd1b1e689df&selectedTaskRun=BzxwAcchTTuFodzpIbTizQ.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=332116836&repo=autoland&lineNumber=6747

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de11c0fbd901
In background task mode, only process updates if we're the sole instance running. r=mhowell,application-update-reviewers,dthayer,bytesized

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/de11c0fbd901

Comment 8

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 9207286 [details]
nsAppRunner.cpp

This doesn't seem relevant.

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1695797 - DO NOT LAND - Add logging to sync manager and multi-instance lock.

Comment 10

[Nick Alexander :nalexander [he/him]]

Clearing NI since I've relanded this. I needed to tweak the test a little to avoid a Windows issue, ticket is Bug 1696772. The DO NOT LAND patch that came later was for help debugging that ticket.