Closed Bug 1400467 Opened 7 years ago Closed 7 years ago

Make WeaveCrypto use promises.

Categories

(Firefox :: Sync, enhancement, P1)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

References

Details

Attachments

(2 files)

Bug 1400467 - Make WeaveCrypto use promises instead of spinning event loops
59 bytes, text/x-review-board-request
eoger
: review+
markh
: review+
Details
Bug 1400467 - Ensure services/common/logmanager.js awaits it's cleanup function
59 bytes, text/x-review-board-request
markh
: review+
Details
Follow up for bug 1210296, to a certain extent. Replaces promiseSpinningly in those files.
Comment on attachment 8908926 [details] Bug 1400467 - Make WeaveCrypto use promises instead of spinning event loops https://reviewboard.mozilla.org/r/180542/#review186044 Thanks. I see a few test failures on try, but the patch makes sense. r+ with test failure fixes.
Attachment #8908926 - Flags: review?(eoger) → review+
Priority: -- → P1
Comment on attachment 8908926 [details] Bug 1400467 - Make WeaveCrypto use promises instead of spinning event loops https://reviewboard.mozilla.org/r/180542/#review186332 LGTM, but let's wait until after the merge to land this. ::: services/sync/modules/bookmark_validator.js:835 (Diff revision 1) > collection.full = true; > let result = await collection.getBatched(); > if (!result.response.success) { > throw result.response; > } > - return result.records.map(record => { > + return await Promise.all(result.records.map(async record => { I'm slightly worried about this change - for no good reason other than suspicion :) But it's not immediately obvious that up to 1k of promises here will do the right thing (and later we are likely to bump this 1k limit). I think we should check this does perform OK (ie, doesn't jank) in a profile with ~1k bookmarks. ::: services/sync/modules/collection_validator.js:80 (Diff revision 1) > collection.full = true; > let result = await collection.getBatched(); > if (!result.response.success) { > throw result.response; > } > - return result.records.map(record => { > + return await Promise.all(result.records.map(async record => { ditto here.
Attachment #8908926 - Flags: review?(markh) → review+
Comment on attachment 8908926 [details] Bug 1400467 - Make WeaveCrypto use promises instead of spinning event loops https://reviewboard.mozilla.org/r/180542/#review188016 ::: toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js:336 (Diff revision 2) > })); > }); > } > > // Utility function to install a keyring at the start of a test. > - installKeyRing(fxaService, keysData, salts, etag, properties) { > + async installKeyRing(fxaService, keysData, salts, etag, properties) { I checked with :glasserc in IRC yesterday to ensure this promise was not being dropped unintentionally. Now that decryption is actually async, it seems that we lose the race consistently here. Easy fix once identified though. (I also think my changes to this file and ExtensionStorageSync.jsm are straightforward enough not to need to get another reviewer involved...).
Comment on attachment 8911273 [details] Bug 1400467 - Ensure services/common/logmanager.js awaits it's cleanup function https://reviewboard.mozilla.org/r/182766/#review188028 This really doesn't need to be its own patch now, but I had fiercly overcomplicated it earlier (it had been even worse earlier with versions that weren't pushed), while also making it likely behave worse in edge cases. ::: services/common/logmanager.js:323 (Diff revision 2) > + entry.name, ex); > } > }); > - iterator.close(); > + // Wait for this to close if we need to (but it might fail if OS.File has > + // shut down) > + await iterator.close().catch(e => { This isn't necessary for the tests passing, but I don't think any good can come of us failing to await it (which is async, see http://searchfox.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_async_front.jsm#1356 ).
Comment on attachment 8911273 [details] Bug 1400467 - Ensure services/common/logmanager.js awaits it's cleanup function https://reviewboard.mozilla.org/r/182766/#review188272 ::: services/common/logmanager.js:276 (Diff revision 2) > // (one theory is that only cleaning up on error makes it less > // likely old error logs would be removed, but that's not true if > // there are occasional errors - let's address this later!) > if (reason == this.ERROR_LOG_WRITTEN && !this._cleaningUpFileLogs) { > - this._log.trace("Scheduling cleanup."); > - // Note we don't return/await or otherwise wait on this promise - it > + this._log.trace("Running cleanup."); > + await this.cleanupLogs().catch(err => { this should probably be try/catch now? ::: services/common/logmanager.js:323 (Diff revision 2) > + entry.name, ex); > } > }); > - iterator.close(); > + // Wait for this to close if we need to (but it might fail if OS.File has > + // shut down) > + await iterator.close().catch(e => { ditto here.
Attachment #8911273 - Flags: review?(markh) → review+
Pushed by tchiovoloni@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8551f071adbd Make WeaveCrypto use promises instead of spinning event loops r=eoger,markh https://hg.mozilla.org/integration/autoland/rev/6d5418bb52b0 Ensure services/common/logmanager.js awaits it's cleanup function r=markh
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: