See this profile for what the code currently does while creating the first browser window: https://perfht.ml/2qIfZVz Things that should be changed: - the imports in browser.js, nsContextMenu.js and utilityOverlay.js should be lazy - ContextualIdentityService.load() should be called off an idle dispatch callback from nsBrowserGlue.js' _onWindowsRestored method. - this._saver = new DeferredTask(... should be done the first time we call saveSoon - the AsyncShutdown blocker should only be set when there's pending data to write, to avoid needlessly doing work during shutdown when there's nothing to save.

Florin, in terms of priority, should we get this fixed for the fx57 proton/photon release? Currently, most the work is going into the test pilot experiment [1] and the shield study [2]. [1] https://github.com/mozilla/testpilot-containers/ [2] https://github.com/mozilla/testpilot-containers/labels/shield

(In reply to Kamil Jozwiak [:kjozwiak] from comment #1) > Florin, in terms of priority, should we get this fixed for the fx57 > proton/photon release? Yes. Unless I missed something, it's going to be easy (I explained almost everything that should be done in comment 0).

baku, mind taking a look?

Comment on attachment 8874085 [details] [diff] [review] contextual.patch Review of attachment 8874085 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for looking at this! ::: browser/components/nsBrowserGlue.js @@ +1181,5 @@ > } > } > > + // Let's load the contextual identities. > + ContextualIdentityService.load(); In comment 0 I suggested doing this off an idle dispatch, ie. Services.tm.mainThread.idleDispatch(() => { ContextualIdentityService.load(); }); Did you just miss this suggestion, or is there a reason why it's not a good idea? ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm @@ +155,5 @@ > + AsyncShutdown.profileBeforeChange.addBlocker( > + "ContextualIdentityService: writing data", > + () => { > + if (this._saverCount) { > + this._saver.finalize(); you forgot a 'return' keyword here. @@ +160,5 @@ > + } > + }); > + } > + > + ++this._saverCount; I don't understand what this _saverCount int is for. If you are concerned about something changing the data while we are already in the process of saving to disk (ie. after the timer has fired), you can call .disarm() and then .arm() again on the deferred task. @@ +175,5 @@ > }; > > let bytes = gTextEncoder.encode(JSON.stringify(object)); > return OS.File.writeAtomic(this._path, bytes, > { tmpPath: this._path + ".tmp" }); Can we do: let promise = OS.File.writeAtomic(...) return promise.then(() => { this._saver = null; AsyncShutdown.profileBeforeChange.removeBlocker(promise); }); I'm suggesting this because I would prefer if no ContextualIdentityService.jsm code at all ran during shutdown, unless we have something that needs to be saved to disk.

Comment on attachment 8876777 [details] [diff] [review] contextual.patch Review of attachment 8876777 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I see I made a mistake in comment 5 that was likely confusing :-(. ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm @@ +152,5 @@ > + if (!this._saver) { > + this._saver = new DeferredTask(() => this.save(), SAVE_DELAY_MS); > + AsyncShutdown.profileBeforeChange.addBlocker( > + "ContextualIdentityService: writing data", > + () => this._saver.finalize()); We'll need to save a reference to this callback, to be able to remove the shutdown blocker later. @@ +171,5 @@ > let bytes = gTextEncoder.encode(JSON.stringify(object)); > + let promise = OS.File.writeAtomic(this._path, bytes, > + { tmpPath: this._path + ".tmp" }); > + > + AsyncShutdown.profileBeforeChange.addBlocker( OS File already handles for you adding a shutdown blocker, see http://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3cfb6b410d2/toolkit/components/osfile/modules/osfile_async_front.jsm#1496 @@ +175,5 @@ > + AsyncShutdown.profileBeforeChange.addBlocker( > + "ContextualIdentityService: writing data", promise); > + > + return promise.then(() => { > + AsyncShutdown.profileBeforeChange.removeBlocker(promise); The shutdown blocker I would like us to remove from the save method is the blocker added in the saveSoon method. And I would like us to set this._saver to null too, so that we re-add the blocker the next time a save is needed.

Comment on attachment 8877040 [details] [diff] [review] contextual.patch Review of attachment 8877040 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good to me now, with one last comment that needs fixing. Also, I would appreciate if you could include test coverage, which is very easy to do now: you just need to blacklist the "resource://gre/modules/ContextualIdentityService.jsm" module in http://searchfox.org/mozilla-central/source/browser/base/content/test/performance/browser_startup.js To verify this bug is fixed, you need to black list it at least for the "before first paint" startup phase... but given that you used idleDispatch, you can likely blacklist it for all of startup (ie. in the "before handling user events" phase) if it passes try :-). ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm @@ +149,5 @@ > }, > > saveSoon() { > + if (!this._saver) { > + this._saverCallback = () => { this._saver.finalize(); } You need the shutdown blocker to return the promise returned by .finalize(). So this line should either become: this._saverCallback = () => this._saver.finalize(); or if you want to be very explicit: this._saverCallback = () => { return this._saver.finalize(); } I would personally go with the first solution, but either is fine.

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/28ae65a052aa ContextualIdentityService should not be initialized before first paint, r=florian