Page MenuHomePhabricator
Differential D12672

Implement utility functions to check if we have content or notif session with device
ClosedPublic
Actions

Authored by marcin on Jul 4 2024, 9:30 AM.
Tags
None
Referenced Files
F3193563: D12672.id42778.diff
Fri, Nov 8, 11:05 PM
Unknown Object (File)
Fri, Nov 8, 12:24 PM
Unknown Object (File)
Fri, Nov 8, 12:24 PM
Unknown Object (File)
Fri, Nov 8, 5:59 AM
Unknown Object (File)
Sat, Oct 26, 7:17 AM
Unknown Object (File)
Sat, Oct 26, 6:41 AM
Unknown Object (File)
Sat, Oct 26, 6:06 AM
Unknown Object (File)
Sat, Oct 26, 12:35 AM
View All Files
Subscribers
ashoat
kamil

Details

Reviewers
bartek
tomek
kamil
Commits
rCOMMf554afeb5b07: Implement utility functions to check if we have content or notif session with…
Summary

This differential implements simple utility functions to check if we have session for both content and/or notifs with particular device.

Test Plan

Tested in D12673

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Jul 4 2024, 9:30 AM
Herald added a subscriber: ashoat. · View Herald TranscriptJul 4 2024, 9:30 AM
marcin added a parent revision: D12671: Implement notification encryption on web.Jul 4 2024, 9:31 AM
marcin added a child revision: D12673: Utilities to idempotently create content and notif sessions.Jul 4 2024, 9:34 AM
marcin edited the test plan for this revision. (Show Details)Jul 4 2024, 9:44 AM
Harbormaster completed remote builds in B30151: Diff 42042.Jul 4 2024, 9:50 AM
marcin requested review of this revision.Jul 4 2024, 9:50 AM
tomek accepted this revision.Jul 10 2024, 4:56 AM
tomek added inline comments.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
1477 ↗(On Diff #42042)

Is it guaranteed that this code never fails? Should we add some error handling?

web/shared-worker/worker/worker-crypto.js
756 ↗(On Diff #42042)

We should never place the await keyword deep inside the statement.

This revision is now accepted and ready to land.Jul 10 2024, 4:56 AM
marcin added inline comments.Jul 15 2024, 6:13 AM
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
1477 ↗(On Diff #42042)

We should:

  1. Check for contentCryptoModule nullability.
  2. We can add try{} catch{}. Wouldn't hurt but those are only hash map operations.
marcin updated this revision to Diff 42292.Jul 15 2024, 7:14 AM

Add error handling to CommCoreModule::isContentSessionInitialized

Harbormaster completed remote builds in B30330: Diff 42292.Jul 15 2024, 7:30 AM
kamil accepted this revision.Jul 16 2024, 4:53 AM
kamil added a subscriber: kamil.
kamil added inline comments.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
1480 ↗(On Diff #42292)

do we need this check if we only care about content?

marcin added inline comments.Jul 24 2024, 3:43 AM
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
1480 ↗(On Diff #42292)

With or without the check for notifs this method will fail if someone forgets to call initializeCryptoAccount. On the other hand if someone calls initializeCryptoAccount it will ensure to initialize both accounts. Therefore removing this check would suggest that we allow the crypto state to be partially which is not true. If we get into state where content is initialized but notifs is not then it is a bug that has to be detected and fixed.

marcin updated this revision to Diff 42729.Jul 24 2024, 5:48 AM

Address review

Harbormaster failed remote builds in B30633: Diff 42729!Jul 24 2024, 6:00 AM
marcin updated this revision to Diff 42778.Jul 25 2024, 9:45 AM

Add additional method to check for presence of notifications session for multiple devices with one db query

Harbormaster failed remote builds in B30674: Diff 42778!Jul 25 2024, 9:51 AM
marcin updated this revision to Diff 42938.Jul 30 2024, 3:24 AM

Update phab

Harbormaster failed remote builds in B30793: Diff 42938!Jul 30 2024, 4:14 AM
marcin updated this revision to Diff 43075.Aug 5 2024, 4:09 AM

Rebase before landing

Harbormaster failed remote builds in B30895: Diff 43075!Aug 5 2024, 4:14 AM
marcin updated this revision to Diff 43092.Aug 5 2024, 5:19 AM

Fix CI

This revision was landed with ongoing or failed builds.Aug 5 2024, 6:39 AM
Closed by commit rCOMMf554afeb5b07: Implement utility functions to check if we have content or notif session with… (authored by marcin, committed by Marcin Wąsowicz <marcinwasowicz@MacBook-Pro-Marcin.local>). · Explain Why
This revision was automatically updated to reflect the committed changes.
Marcin Wąsowicz <marcinwasowicz@MacBook-Pro-Marcin.local> added a commit: rCOMMf554afeb5b07: Implement utility functions to check if we have content or notif session with….
Harbormaster completed remote builds in B30912: Diff 43092.Aug 5 2024, 6:57 AM

Revision Contents
Changeset List

PathSize
lib/
types/
7 lines
utils/
__mocks__/
3 lines
native/
cpp/
CommonCpp/
NativeModules/
8 lines
126 lines
Notifications/
BackgroundDataStorage/
3 lines
22 lines
_generated/
crypto/
5 lines
schema/
7 lines
web/
crypto/
7 lines
shared-worker/
worker/
38 lines

Diff 42778

View Options

lib/types/crypto-types.js

Loading...
View Options

lib/utils/__mocks__/config.js

Loading...
View Options

native/cpp/CommonCpp/NativeModules/CommCoreModule.h

Loading...
View Options

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp

Loading...
View Options

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.h

Loading...
View Options

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp

Loading...
View Options

native/cpp/CommonCpp/_generated/commJSI-generated.cpp

Loading...
View Options

native/cpp/CommonCpp/_generated/commJSI.h

Loading...
View Options

native/crypto/olm-api.js

Loading...
View Options

native/schema/CommCoreModuleSchema.js

Loading...
View Options

web/crypto/olm-api.js

Loading...
View Options

web/shared-worker/worker/worker-crypto.js

Loading...