The password manager code triggers main thread sqlite disk I/O off of the gather-telemetry notification
Categories
(Toolkit :: Password Manager, defect, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox85 | --- | fixed |
People
(Reporter: florian, Assigned: dimi)
References
Details
(Keywords: perf, Whiteboard: [fxperf:p1][bhr:gather-telemetry])
Attachments
(2 files)
When looking at BHR data for hangs from the gather-telemetry notification http://queze.net/bhr/test/#row=0&filter=gather-telemetry, it seems (almost?) all the stacks involve:
gre/modules/crypto-SDR.js:172
gre/modules/storage-json.js:377
gre/modules/LoginManager.jsm:124
nsObserverService::NotifyObservers gather-telemetry
gre/modules/TelemetrySession.jsm:1176
Most stacks contain:
PK11SDR_Decrypt(SECItemStr*, SECItemStr*, void*) nss3
SecretDecoderRing::Decrypt
and sqlite main thread I/O.
This is triggered from https://searchfox.org/mozilla-central/rev/ff82c973f8ccb0475ec32439e9ec07014b3a681f/toolkit/components/passwordmgr/LoginManager.jsm#138,141,203
Given that this data collection happens during idle-daily https://searchfox.org/mozilla-central/rev/ff82c973f8ccb0475ec32439e9ec07014b3a681f/toolkit/components/telemetry/pings/TelemetrySession.jsm#1169-1178 and that nothing seems to be waiting synchronously for the result, I wonder if the fix could be as easy as making the _gatherTelemetry method of LoginManager.jsm async, and replacing let logins = this.getAllLogins(); with let logins = await this.getAllLoginsAsync();.
These hangs represent about 0.3% of the total hang time reported on Nightly.
|
||
Comment 1•4 years ago
|
||
We should be able to use getAllLoginsAsync here.
|
Assignee | |
Updated•3 years ago
|
|
|
Assignee | |
Comment 2•3 years ago
|
||
While receiving "gather-telemetry" event, we use synchronous getAllLogins() to
retrieve login information, which might cause Firefox to hang because it requires I/O access.
Since no one is blocked by _gatherTelemetry() method, this patch simply replaces
getAllLogins() with getAllLoginsAsync() to avoid blocking the main-thread.
|
|
Assignee | |
Comment 3•3 years ago
|
||
Depends on D98292
|
||
Updated•3 years ago
|
Pushed by dlee@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1f02610cc1a7 P1. Use asynchronous getAllLogins when processing gather-telemetry event r=sfoster https://hg.mozilla.org/integration/autoland/rev/fc53bbf24599 P2. Update test_telemetry test to wait until gather telemetry is complete r=sfoster
|
||
Comment 5•3 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/1f02610cc1a7
https://hg.mozilla.org/mozilla-central/rev/fc53bbf24599
Description
•