Closed Bug 1400467 Opened 3 years ago Closed 3 years ago

Make WeaveCrypto use promises.

Categories

(Firefox :: Sync, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

References

Details

Attachments

(2 files)

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
https://hg.mozilla.org/mozilla-central/rev/8551f071adbd
https://hg.mozilla.org/mozilla-central/rev/6d5418bb52b0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.