I don't think so. Not sure what the goal was, but:

1. If the goal was to set this value in one thread and then read in multiple, that would be probably incorrect, as only the result of one call of threadSafeContentHandlerCall would result in calling contentHandlerCalled - so we would ignore all but one notifications.
2. If the goal is to set this value in one thread and read in the same, then it should be guaranteed that this value is visible later.

So I don't think we need to synchronize, but I also don't know why it is needed at all.

We probably don't need this comment anymore.

Shouldn't we unregister every time we return from this method? If that's the case, we should create a separate method that wraps this one so that we can unregister in one place, instead of multiple ones.

What do we try to achieve here? If we assume that an instance of this class can be used by multiple threads, this code won't solve that - it would be still possible for two threads to call self.contentHandlerCalled = NO; and then to call threadSafeContentHandlerCall, which would make the second call invalid. If we are sure that the instance is used by one thread, then synchronizing the block is not necessary - the whole method is executed on one thread and we have a happen-before relationship.

I hope that the answer to the last @tomek 's comment answers this as well

That is correct - I forgot to add unregister calls before all returns.

There is more to the picture here and the question "can this class be accessed from multiple threads or not" is not exhaustive enough. There are two "types" of threads here, The first is the thread that iOS creates to call didReceive... callback on it. The second is the thread that we introduce by calling registerForMemoryEvents method that will be calling completionHandler of this instance (technically we are not creating additional thread - we just let the already existing main thread of the process call lambda that access this instance).
That said we might have two cases:

1. The iOS always creates new instance of NSE class for each thread that processes notification or reuse the instance but sequentially.
2. The iOS might let two different threads access the same NSE class instance simultaneously.

In case 1 is true then this synchronization is necessary and is correct since we have two threads that can call completion handler - the thread iOS created for this NSE class and the thread that will respond to memory events. And the line at 55 does not need synchronization since it will be called only from the thread that iOS created for this NSE instance.

In case 2 is not true then this synchronization is not correct and @ashoat is right that this diff may introduce regressions. However if 2 is correct our code was, in fact, invalid from the start even before we introduced e2e notifs. e2e notifs only revealed the issue as we are actually mutatin notification content (previously we only used to persist it in a file).

Why did I think that 1 is correct? An example from Apple docs: https://developer.apple.com/documentation/usernotifications/modifying_content_in_newly_delivered_notifications?language=objc shows usage of NSE that is not thread-safe in case of multiple threads accessing the same NSE instance.

What makes me think that 2 might actually be correct?
Signal's NSE code preambule: https://github.com/signalapp/Signal-iOS/blob/main/SignalNSE/NotificationService.swift#L10-L30
and the fact that they use atomic variables for contentHandler instead of directly accessing self: https://github.com/signalapp/Signal-iOS/blob/main/SignalNSE/NotificationService.swift#L37

Additionally element app keeps a hash table of contents and handlers that correspond to each other: https://github.com/vector-im/element-ios/blob/develop/RiotNSE/NotificationService.swift#L135-L136

I hope that the above answers @ashoat's questions:

Provide more detail (including a link to the code) about Signal's beliefs?

I linked the code.

If you are confident that this diff won't introduce additional regressions in the case that Signal is right, can you please talk through the various "race" scenarios here, and explain why the behavior will be unchanged?

If Signal's is right then this diff might introduce some regressions.

In summary:
I wrote this differential prioritising Apple docs more than Signal/Telegram/Element and believing that 1 is true. I hope that I justified that this diff is correct in case 1 is true and we trust the docs. However yesterday during our 1:1 @ashoat advised me to prioritise the code more than the docs.

That said this differential needs refactor to adopt the threading patterns of Signal's and Telegram

After discussion with @tomek we agreed that this code might be our primary suspect. One thread/process creates a temporary file and starts writing to it. Then another thread/process starts to execute this code and removes temporary file the first thread is writing to which causes the first thread to crash.

I we assume that it is possible for the same NSE instance to be used by many thread at once to process different notifications then we have to keep a collection on handlers and contents and accesses to this collection must be synchronized.

I considered folly's implementation of ConcurrentHashMap: https://github.com/facebook/folly/blob/main/folly/concurrency/ConcurrentHashMap.h, however it was not sufficient for our case. In serviceExtensionTimeWillExpire we must atomically retrieve all contents and handlers and remove them from hash maps to prevent other threads from accessing them. ConcurrentHashMap does not offer the getAndDeleteAll- like functionality. Therefore this code uses custom mutex-based locking scheme to ensure atomicity of any block of code.

I want to make sure that whatever occurs inside @synchronized block is as fast as possible. That said the only operations that take place in @synchronized blocks are hash map keys additions/deletions. Content handler blocks are provided by the system so they might be potentially costly to call so I want to avoid calling them in @synchronized blocks.

Memory events are delivered at process level. Therefore if we decide to create separate memorySource for each thread/NSE instance we could see redundant messages about memory issue in case one happens. That said I decided that memorySource will be set once per process. In case memory event occurs the event handler will substitute a string value that is initially null. Each NSE instance belonging to the process will check for this value before displaying notification. The first one that sees non-null value will replace the notification content it was about to display with memory error notification.

Making sure each thread/process uses a distinct file with a random suffix

The reason to introduce temporary file here is to have atomic reads and writes to main olm session file. The content of main olm file is read into memory, in-memory version is modified, modified content is written to temporary file and finally temporary file is renamed with the main olm session file. That said there is no code reading this file and there is no code accessing this file outside of this method. Did I correctly understand your question?

Removing the line that deletes the file:

Temporary file is removed each time an error occurs and is deleted at the end of this method (it is not currently deleted but I will add this line - forgot to do it thanks for catching). Is there any concern then? Although unlikely some uncaught error (or sth like power-off) might occur just before we remove the temporary file. It will not grow since any subsequent call to this method will create new temporary file but it may just stafy there undeleted. To be honest I don't have a better idea on how to solve this problem than some scheduled cleanup. Android and iOS have tools for this. Perhaps we could do it together with downloaded blobs cleanup? Alternatively we could use directory search API to check for files that are older that some threshold and delete them. Such code could be executed at the beginning of this function or in the NSE code just before decrypted and persisted notification is displayed.

This is valid observation. I will check since I am not sure.

I couldn't find any resources for this so I think it is better to remove potential spaces.

I would be scared to override init for this class. Apple docs are rather strict that we should no aim to instantiate NSE ourselves and didReceive... with serviceExtensionTimeWillExpire are the only API we should use. Additionally signal does global setup in didReceive.. callback: https://github.com/signalapp/Signal-iOS/blob/6cc18788850558251db5f772ca62bce6b576dfc4/SignalNSE/NotificationService.swift#L111. This is the particular reason I made this method idempotent.

Unfortunately there is nothing we can do about it since it was what the formatter gives us.

Actually this implementation is our own invention. I examined Signal and Element code.

1. Signal just silences current content: https://github.com/signalapp/Signal-iOS/blob/6cc18788850558251db5f772ca62bce6b576dfc4/SignalNSE/NotificationService.swift#L166. This suggests that they don't believe the same NSE can be used by two threads at the same time. On the other hand they keep contentHandler of current NSE as an atomic variable: https://github.com/signalapp/Signal-iOS/blob/6cc18788850558251db5f772ca62bce6b576dfc4/SignalNSE/NotificationService.swift#L37.
2. Element on the other hand does a similar thing to us (they keep a collection of handlers): https://github.com/vector-im/element-ios/blob/develop/RiotNSE/NotificationService.swift#L38 but their way of handling NSE timeout is just to give up: https://github.com/vector-im/element-ios/blob/develop/RiotNSE/NotificationService.swift#L147

After discussion with @tomek we agreed that it looks like those apps were aware of the possibility of two threads accessing the same NSE instance at a time but didn't have a good idea on how to handle it in case serviceExtensionTimeWillExpire since we don't know which notification is it called for.

That said I concluded that since we don't know which notification serviceExtensionTimeWillExpire is called for we will try to handle all notifications that are currently being processed.

We probably don't know if Apple would penalize us for not calling a handler...

It is not penalized - notification is just displayed unchanged.

setUpNSEInstance is synchronized on self so it is a minor optimization to use different lock.

Might be worth considering having a single array of pair-like structures.

This line doesn't do anything. To have the same effect as updating self.bestAttemptContent previously, you would need to call your putContent method again

You are right that this line is not necessary. However there is no need to call putContent here. Unless we exceed time limit the actual content displayed to the user is going to be publicUserContent variable. Contents that we store in the synchronized dictionary (that are stored there by putContent) are only used in serviceExtensionTimeWillExpire to know what the notification type was to display appropriate timeout error message. In serviceExtensionTimeWillExpire if we detect that content was badgeOnly content we will construct badgeOnlyNotif again. Therefore we only have to delete this line.