1625271 - getNewSideloads checks for system add-ons last modified time on the main thread at the end of startup

Status: NEW

(Toolkit :: Add-ons Manager, enhancement)

Description

[Florian Quèze [:florian]]

See this profile: https://bit.ly/2WGJr18

Accessing .lastModifiedTime on an nsIFile object causes a stat call on the main thread, which blocks the UI.

It's done at https://searchfox.org/mozilla-central/rev/b712398b7fae54ef377a558d6f16dede7a7f8530/toolkit/mozapps/extensions/internal/XPIProvider.jsm#583

The stack is:

promise callback []
AsyncFunctionNext [self-hosted:693:26]
InterpretGeneratorResume [self-hosted:1153:33]
_checkForSideloaded [resource:///modules/ExtensionsUI.jsm:75:27]
getNewSideloads [resource://gre/modules/AddonManager.jsm:3497:17]
getNewSideloads [resource://gre/modules/addons/XPIProvider.jsm:2987:23]
scanForChanges [resource://gre/modules/addons/XPIProvider.jsm:1413:16]
getModTime [resource://gre/modules/addons/XPIProvider.jsm:578:12]

getNewSideloads is already an async function, so we would need an async code path in scanForChanges and then getModTime could be done using OS.File.

Comment 1

[Andrew Swan [:aswan]]

A challenge here is that this code can be called at different times during startup. In a "regular" startup this is not called until after first paint. But in a new profile or after an application update, this code is called from XPIProvider.startup() -> XPIProvider.checkForChanges -> XPIProvider.scanForChanges(). If we switch this code to use OS.File then we're adding another user of osfile before first paint which we've been trying to avoid (bug 1522955)

Comment 2

[:Gijs (he/him)]

(In reply to Andrew Swan [:aswan] from comment #1)

If we switch this code to use OS.File then we're adding another user of osfile before first paint which we've been trying to avoid (bug 1522955)

Looks like this was meant to be bug 1522925.

Comment 3

[Beth Rennie [:beth] (she/her)]

Now that we have IOUtils support, we could use that instead of OS.File.

However, getModTime is also called in XPIState.syncWIthDB and XPIStateLocation.addFile, both of which have deep sync stacks. This would require asyncifying a lot of XPIDatabase and XPIProvider

Comment 4

[Bas Schouten (:bas.schouten)]

The Performance Impact Calculator has determined this bug's performance impact to be low. If you'd like to request re-triage, you can reset the Performance Impact flag to "?" or needinfo the triage sheriff.

Platforms: Windows
Impact on browser: Causes noticeable startup delay
Configuration: Specific but common
Websites affected: Rare
[x] Able to reproduce locally